Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Re-import to bookmark toolbar will mess up the order #7194

Closed
darkdh opened this issue Feb 11, 2017 · 2 comments · Fixed by #8402
Closed

Re-import to bookmark toolbar will mess up the order #7194

darkdh opened this issue Feb 11, 2017 · 2 comments · Fixed by #8402
Assignees
@darkdh
Labels
feature/bookmarks feature/importer QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Milestone
0.15.300 (Frozen,...

Comments

@darkdh
Copy link
Member

darkdh commented Feb 11, 2017 •
edited by bsclifton
Loading

Test Plan:

a. getNextFolderName covered by unittest

b. Import bookmark:

  1. Import bookmarks from other browser or html
  2. There will always a folder contains the imported bookmarks
  3. If the folder name already exist,
    it will be "Imported from XXX (1)", "Imported from XXX (2)" ... etc

Original issue description

STR:

  1. Re-import bookmarks twice by choosing merge into bookmark toolbar
  2. The second import will overwrite the original bookmark by arbitrary order

Proposal:
To make it simple, I suggest we follow the chrome behavior to eliminate this option

  1. Only write to bookmark toolbar for the first time (No bookmarks)
  2. If you have bookmarks, always create a folder for new imported bookmarks

cc @bradleyrichter , @bbondy
@darkdh darkdh mentioned this issue Feb 11, 2017
Closed
@darkdh
Copy link
Member Author

darkdh commented Feb 11, 2017

Related #6564

@bradleyrichter
Copy link
Contributor

@darkdh Your proposal sounds like the least amount of user friction. WFM.

@darkdh darkdh self-assigned this Feb 14, 2017
@darkdh darkdh mentioned this issue Feb 14, 2017
Closed
@darkdh darkdh added this to the 0.15.1 milestone Apr 18, 2017
darkdh added a commit that referenced this issue Apr 19, 2017
@darkdh
Remove merge into toolbar option from importer
c684f48 
fix #7194

Auditors: @bsclifton, @bbondy

Test Plan:

a. getNextFolderName covered by unittest

b. Import bookmark:
1. Import bookmarks from other browser or html
2. There will always a folder contains the imported bookmarks
3. If the folder name already exist,
  it will be "Imported from XXX (1)", "Imported from XXX (2)" ... etc
@darkdh darkdh mentioned this issue Apr 19, 2017
Merged
4 tasks
darkdh added a commit that referenced this issue Apr 27, 2017
@darkdh
Remove merge into toolbar option from importer
46e3919 
fix #7194

Auditors: @bsclifton, @bbondy

Test Plan:

a. getNextFolderName covered by unittest

b. Import bookmark:
1. Import bookmarks from other browser or html
2. There will always a folder contains the imported bookmarks
3. If the folder name already exist,
  it will be "Imported from XXX (1)", "Imported from XXX (2)" ... etc
This was referenced May 5, 2017
Closed
Closed
Closed
Closed
@luixxiul luixxiul mentioned this issue May 28, 2017
Closed
@darkdh darkdh mentioned this issue Aug 14, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@darkdh darkdh

Labels
feature/bookmarks feature/importer QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.15.300 (Frozen, only critical adds ...
Development

Successfully merging a pull request may close this issue.

Remove merge into toolbar option from importer brave/browser-laptop
6 participants
@alexwykoff @luixxiul @bsclifton @darkdh @bradleyrichter @srirambv