Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bookmarks management #125

Merged
merged 79 commits into from
Jun 21, 2021
Merged

Bookmarks management #125

merged 79 commits into from
Jun 21, 2021

Conversation

samsymons
Copy link
Collaborator

@samsymons samsymons commented May 31, 2021 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/72649045549333/1200025962848493/f
Tech Design URL:
CC: @brindy

Description

This PR adds the final part of the bookmarks management feature. Specifically, it adds:

  • A new management screen which lists all bookmarks and folders
  • A bookmark list popover for quick access to your bookmarks

Steps to test this PR

  1. Test the regular bookmark functionality works as expected
  2. Test that the new menu buttons work, specifically the Manage Bookmarks button in the main menu, the Bookmarks option in the options menu, and the new toolbar button
  3. Test that dragging bookmarks around in the management view works
  4. Test that the list view renders bookmarks correctly and the interactions work as expected
  5. Test that the right click menu works on all bookmarks and folders
  6. Test that folders appear in the bookmarks menu in the menu bar
Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

samsymons and others added 11 commits May 13, 2021 11:41
@samsymons @RendijsSmukulis
@brindy @mallexxx @tomasstrba
Bookmarks management part 1 (#107)
1c296cc 
* Consolidate Core Data testing utilities.

* Add the updated data model, along with migration and tests.

* Add BookmarkManagedObject validation & tests.

* Update the bookmark store and manager.

* Add the Node and TreeController code.

* Properly copy over the Bookmark data model changes.

* Rename Folder to BookmarkFolder for clarity.

* Update BookmarkManagedObject validation function names.

* Fix the Bookmark data model path.

* Rename Node to BookmarkNode.

* Fixed up filenames in file headers (#109)

* check file headers programatically (#110)

* Zoom menu items; Menu Items validation (#105)

* Zoom menu items; Menu Items validation in MainMenuActions

* Update navigational menu items on WebView state change

* Update to BrowserServicesKit 1.1.1 (#111)

* Use a specific version of BrowserServicesKit.

* Use BrowserServicesKit 1.1.1.

* Force the content blocking rules publisher onto the main thread. (#112)

* Version 0.6.16

* Address SwiftLint warnings.

* Add the bookmark checks back.

* Fix some incorrect file headers comments.

* Fix remaining SwiftLint warnings.

Co-authored-by: Rendijs Smukulis <rendijs.smukulis@gmail.com>
Co-authored-by: Christopher Brind <brindy@duckduckgo.com>
Co-authored-by: Alexey Martemyanov <mallexxx@gmail.com>
Co-authored-by: Tomas Strba <tom@duckduckgo.com>
@samsymons
Bookmarks management part 2 (#115)
85105db 
* Add NSOutlineView extensions.

* Add PasteboardFolder and PasteboardBookmark.

* Update the NSOutlineView extension.

* Add the bookmark sidebar tree controller.

* Add the BookmarksOutlineView subclass.

* Rename a test file.

* Add the bookmarks outline view data source and views.

* Clean up a few comments.

* Fix another misnamed header.

* Fix failed tests.

* Fix PR comments.
@samsymons
Merge branch 'develop' into feature/sam/bookmarks-management
1a77776 
# By Tomas Strba (4) and others
# Via GitHub
* develop:
  Update the Chrome user agent (#122)
  TabViewModel released immediately from TabCollectionViewModel after its tab is closed (#118)
  User agent updated (#117)
  History Storage - Please do not merge (#108)
  use the temporary unprotected sites list and update the frequency to 30 minutes (#114)
  update email autofill (#113)
  Version 0.6.16
  Force the content blocking rules publisher onto the main thread. (#112)
  Update to BrowserServicesKit 1.1.1 (#111)
  Zoom menu items; Menu Items validation (#105)
  check file headers programatically (#110)
  Fixed up filenames in file headers (#109)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/Bookmarks/Services/Bookmark.xcdatamodeld/Bookmark.xcdatamodel/contents
#	Unit Tests/Bookmarks/Services/LocalBookmarkStoreTests.swift
#	Unit Tests/FileSystem/CoreDataEncryptionTests.swift
@samsymons
Repair merge conflicts in the test suite.
3aef4b5
@samsymons
Fix a warning related to header comment naming.
19c8f2c
@samsymons
Bring over bookmark UI code.
4325ed2
@samsymons
Remove extra print statements.
8a44502
@samsymons
Fix dark mode colors when editing bookmarks.
635dab4
@samsymons
Fix some logic around cell states.
529f70e
@samsymons
Sort displayable tab cases alphabetically.
58c9761
@samsymons
Verify that the tab switcher button has the right item selected.
bd6a41b
@samsymons
Rename a file header comment.
1f091b0
@samsymons
Move bookmarks colors into the color extension.
3292143
@samsymons
Push tab chooser logic into the view controllers.
fd5dc37
@samsymons
Store the UUID when setting the editing state.
599912a
@samsymons
Merge branch 'develop' into feature/sam/bookmarks-management
c8aae1a 
# By Tomas Strba (2) and others
# Via Tomas Strba
* develop:
  Version 0.6.17
  History in Autocomplete Suggestions (please don’t merge until BSK branch is released) (#121)
  Save As (Downloads Refactoring) (#119)
  update dependency on BSK for email (#116)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@samsymons
Fix test suite merge conflicts.
885db90
@samsymons
Merge branch 'feature/sam/bookmarks-management' into feature/sam/book…
2cc5527 
…marks-management-part-3

# By Tomas Strba (2) and others
* feature/sam/bookmarks-management:
  Fix test suite merge conflicts.
  Version 0.6.17
  History in Autocomplete Suggestions (please don’t merge until BSK branch is released) (#121)
  Save As (Downloads Refactoring) (#119)
  update dependency on BSK for email (#116)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
@samsymons
Bring over menu changes to show folders in menus.
390a692
@samsymons
Remove an assertion that doesn’t need to be there.
f365cd6 
This is a legitimate case being checked, it doesn’t need to be asserted.
@brindy brindy self-assigned this Jun 16, 2021
@samsymons samsymons changed the title Bookmarks management part 3 Bookmarks management Jun 17, 2021
@samsymons samsymons changed the base branch from feature/sam/bookmarks-management to develop June 17, 2021 04:24
@samsymons
Copy link
Collaborator Author

@brindy I've done a pass over this today to just fix a few little things and clean up some older code, and have retargeted this branch against develop instead of merging it into the staging branch and having to open that as a PR.

I just want to read through it all once more tomorrow morning and then merge if that's okay, as it's been a while since I've compared the entire thing to `develop.

@brindy
Copy link
Contributor

brindy commented Jun 17, 2021

@samsymons one thing I've noticed when switching between this branch and others off develop is that I can get core data errors. I think upgrading to this branch is OK (the problem seems to be going the other way, which is reasonable), but can you just check that to be sure, please?

@samsymons
Copy link
Collaborator Author

Tested bookmark migration earlier today by running the latest develop branch with a fully clean slate, bookmarking some websites, and then running this branch - didn't run into any issues, so I'll merge this tomorrow after tweaking some design things that Bryan reported.

@samsymons
Copy link
Collaborator Author

samsymons commented Jun 19, 2021 •
edited
Loading

Bryan had a couple final requests yesterday afternoon:

  • Show the bookmarks panel when clicking the Bookmarks menu item in the options menu
  • Add a Manage Bookmarks menu item to the main Bookmarks menu in the menu bar
  • Keep favorites open in the list view when making changes to the data model

All of these are super quick jobs.

@samsymons
Add a button to the Bookmarks menu to show the manage bookmarks UI.
3f5a398
@samsymons
Remember the expanded state of pseudofolders.
222fbdc
@samsymons
Fix test suite compilation failures from an earlier refactor.
56b305b
@samsymons
Remove an unused variable.
1fe8d4b
@samsymons
Show the bookmark list popover from the options menu.
8418efb
@samsymons
Merge branch 'develop' into feature/sam/bookmarks-management-part-3
b0ae352 
* develop:
  Version 0.7.1
  Directory for the data persistance set to original (#136)
  Version 0.7.0
  Use NavigationAction.shouldDownload for data links (#134)
  Sparkle framework & Automatic Updates (#132)
  Copyright year set to 2021 (#135)
  Fix Gmail (#131)
@samsymons
Hide the bookmark edit option in the list view.
318245c
@samsymons
Use the correct app appearance when setting the detail view color.
d4fc4f5
@samsymons
Track when bookmarks are clicked from the new UI.
ddd54e3
@samsymons
Update a bookmark pixel.
ac5e51a
@samsymons
Fire a pixel when showing the bookmarks list.
7eb8bf2
@samsymons
Copy link
Collaborator Author

Happy with the migration flow here. I tested this from a develop debug build and then shifted to this branch with no issues.

I also installed the latest release from Asana, and then ran an archive built off of this branch, no issues either.

@samsymons samsymons merged commit ed949d8 into develop Jun 21, 2021
@samsymons samsymons deleted the feature/sam/bookmarks-management-part-3 branch June 21, 2021 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brindy brindy brindy approved these changes

Assignees

@brindy brindy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samsymons @brindy