Alessandro/address bookmarks feedback bookmark logic #2231

alessandroboron · 2024-02-21T10:55:58Z

Task/Issue URL: https://app.asana.com/0/0/1206590790808718/f
CC: @samsymons

Description:

In this PR I address the following:

Implementation of add/edit bookmark dialog logic + tests.
Extracted view to be reused by AddBookmarkPopoverView & AddEditBookmarkDialogView

NOTE

I tried different approaches to present the “Add Folder” dialog when the user taps on the “Add Folder” button in the “Add/Edit Bookmark” dialog. The main issue encountered was to "sync” the folder selected by the user via the picker in the “Add Bookmar” dialog to the one in the “Add Folder” dialog and viceversa. Scenarios explained below:

Scenario 1:

User adds/edit a bookmark.
User select another folder using the folders picker.
User taps “Add Folder” button.
Expected result: The picker in the “Add Folder” dialog should have selected the folder previously selected by the user.

Scenario 2:

User taps “Add Folder” button.
User select another folder using the folders picker.
The user confirm the folder creation.
Expected result: On landing back to the “Add/Edit Bookmark” dialog the picker should have selected the new folder created.

Approach 1.

Use of the @EnvironmentObject as previously discussed.

This approach was simple altough I found that to synchronise the two folders I had to do some logic in the view and I could not test that.

For scenario 1 I would assign the value from one VM to the other one in onAppear as per commit 45185fb#diff-c0a84739fdc5e3437d79a0df01b5ffa4c9464e048036422022d72a6d1cfb2a0b

For scenario 2 I had a callback that fired when the folder was created. The issue with that was that the VM was created beforehand and injected so the closure behaviour would be de at the caller site. That means that this logic would be duplicated at every call site. While this could be fixed via a factory I opted not to have logic in the factory.

Approach 2.

Shared mutable state.

The title says all 🗡️ . I extracted the logic to store the folders and the selected one in its own class. This was then injected in the VMs for both dialogs. It worked fine but shared mutable state is difficult to reason about and always introduce bugs in the long term.

Approach 3.

A wrapper view model (called CoordinatorViewModel) that orchestrates the events between the view model for AddEdit BookmarkDialogView & the view model for AddEdit BookmarkFolderDialogView.

I don’t think it’s perfect. It’s not really a coordinator….It hijacks the addFolderAction which is called when the user taps on the “Add Folder” button. This approach let me test every path.

I’m happy to hear your thoughts and I’m available ti have a sync conversation Tomorrow to walk through the PR.

—

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

github-actions · 2024-02-21T10:56:25Z

🚫 The Asana task linked in the PR description is not added to macOS App Board project.

Verify that the correct task is linked in the PR.
- ⚠️ Please use the actual implementation task, rather than the Code Review subtask.
Verify that the task is added to macOS App Board project.
When ready, remove the bot: not in app board label to retrigger the check.

github-actions · 2024-02-21T10:57:44Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 9a5d181

…alogView

…popover and dialog

SabrinaTardio

Works as expected from what I see. In general looks good but I feels like architecture is a little bit too complicated for a simple feature.
It feels like there are too many layers but this is probably more of a personal choice.
I have left a few comments we can discuss some although in general I am happy to approve it.

SabrinaTardio · 2024-02-23T10:16:02Z

DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkFolderDialogViewModel.swift

-
-    func cancel()
-    func addOrSave()
+protocol AddEditBookmarkFolderDialogViewModelProtocol: BookmarksDialogViewModel {


I don’t like the name protocol to a protocol… Maybe we can chose a name that describes what it is useful for...
Maybe could be BookmarkFolderEditing?
Also I would apply BookmarksDialogViewModel protocol to the actual view model directly so that this can be separate although I suppose it complicates the coordinator

No problem for the name, not a fan of the word protocol either.

About "Also I would apply BookmarksDialogViewModel protocol to the actual view model directly so that this can be separate although I suppose it complicates the coordinator”, the Coordinator still needs to accept a AddEditBookmarkFolderDialogViewModelProtocol so if I don’t use this approach I cannot inject the VM.

I could use composition via typealias and have something like typalias BookmarkFolderDialogViewModel = <protocol_a> & <protocol_b> but there are no scenarios where I would inject only protocol_a or only protocol_b.

Happy to have a chat about this if I misunderstood what you meant.

SabrinaTardio · 2024-02-23T10:38:50Z

DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkDialogViewModel.swift

+import Combine
+
+@MainActor
+protocol AddEditBookmarkDialogViewModelProtocol: BookmarksDialogViewModel {


Again we could find a protocol name based on what it does maybe BookmarkDialogModeling?
Also I would apply BookmarksDialogViewModel protocol to the actual view model directly so that this can be separate although I suppose it complicates the coordinator

SabrinaTardio · 2024-02-23T11:18:53Z

DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkDialogViewModel.swift

+        if bookmark.url != url.absoluteString {
+            bookmark = bookmarkManager.updateUrl(of: bookmark, to: url) ?? bookmark
+        }
+        //


I guess this can be deleted

SabrinaTardio · 2024-02-23T11:27:32Z

DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkDialogCoordinatorViewModel.swift

+import SwiftUI
+import Combine
+
+final class AddEditBookmarkDialogCoordinatorViewModel<BookmarkViewModel: AddEditBookmarkDialogViewModelProtocol, AddFolderViewModel: AddEditBookmarkFolderDialogViewModelProtocol>: ObservableObject {


It feels like we have to fight the system doesn’t it?!

I think this approach is ok, I was thinking another approach that might be more streamlined could be to have a unique view model for edit folders and bookmarks since they are closely related day they are very related and have the same dependency.
Just thinking out loud do not need to change.

I thought about that, my worry was that the VM would increase in complexity to handle all the different cases/operations.

SabrinaTardio · 2024-02-23T11:51:54Z

DuckDuckGo/Bookmarks/View/Dialog/AddEditBookmarkDialogView.swift

-            return UserText.save
-        }
+    private var addFolderView: some View {
+        AddEditBookmarkFolderView(


For both isn’t it better to pass the view model directly? I assume you will want to use the view from all the other places and don’t want to do the list every time

I get your point, the reason I’m not currently passing the VM is because the “Popover” we show from the address bar re-use the same views but have a different VM due to the live updates. Having said, that I should be able (🤞) to re-use the protocols I made for the new VMs and inject that one to the view. The thing is as soon as I pass the protocol I have to make the Views generic over the ViewModel due to any <viewModelProtocol> and not being able to use property wrappers.

DuckDuckGo/Bookmarks/View/AddBookmarkPopoverView.swift

Task/Issue URL: https://app.asana.com/0/0/1206590790808718/f **Description**: 1. Implementation of add/edit bookmark dialog logic + tests. 2. Extracted view to be reused by AddBookmarkPopoverView & AddEditBookmarkDialogView

github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Feb 21, 2024

Base automatically changed from alessandro/address-bookmarks-feedback-folder-logic to alessandro/address-bookmarks-feedback February 23, 2024 01:00

alessandroboron added 8 commits February 23, 2024 12:02

Initial logic

c8c5978

Inject selected folder from BookmarkDialogView to AddBookmarkFolderDi…

02146f6

…alogView

Fixed Previews

06d1400

Refactor logic to use CoordinatorViewModel

74bc8a9

Add unit tests for AddEditBookmarkDialogViewModel

fe2dfbe

Add unit tests for CoordinatorViewModel

fbd26dc

Refactor view logic and extract AddEditBookmarkView to be re-used in …

c5056a7

…popover and dialog

Fix lint warning

00b1f99

alessandroboron force-pushed the alessandro/address-bookmarks-feedback-bookmark-logic branch from d2a954d to 00b1f99 Compare February 23, 2024 01:02

SabrinaTardio reviewed Feb 23, 2024

View reviewed changes

Fix PR comments

9a5d181

alessandroboron requested a review from SabrinaTardio February 26, 2024 08:27

SabrinaTardio approved these changes Feb 26, 2024

View reviewed changes

alessandroboron merged commit c8c6737 into alessandro/address-bookmarks-feedback Feb 26, 2024
16 of 17 checks passed

alessandroboron deleted the alessandro/address-bookmarks-feedback-bookmark-logic branch February 26, 2024 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alessandro/address bookmarks feedback bookmark logic #2231

Alessandro/address bookmarks feedback bookmark logic #2231

alessandroboron commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024 •

edited

Loading

SabrinaTardio left a comment

SabrinaTardio Feb 23, 2024

alessandroboron Feb 26, 2024 •

edited

Loading

SabrinaTardio Feb 23, 2024

SabrinaTardio Feb 23, 2024

SabrinaTardio Feb 23, 2024

SabrinaTardio Feb 23, 2024

alessandroboron Feb 26, 2024

SabrinaTardio Feb 23, 2024

alessandroboron Feb 26, 2024 •

edited

Loading

Alessandro/address bookmarks feedback bookmark logic #2231

Alessandro/address bookmarks feedback bookmark logic #2231

Conversation

alessandroboron commented Feb 21, 2024

Internal references:

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024 • edited Loading

SabrinaTardio left a comment

Choose a reason for hiding this comment

SabrinaTardio Feb 23, 2024

Choose a reason for hiding this comment

alessandroboron Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

SabrinaTardio Feb 23, 2024

Choose a reason for hiding this comment

SabrinaTardio Feb 23, 2024

Choose a reason for hiding this comment

SabrinaTardio Feb 23, 2024

Choose a reason for hiding this comment

SabrinaTardio Feb 23, 2024

Choose a reason for hiding this comment

alessandroboron Feb 26, 2024

Choose a reason for hiding this comment

SabrinaTardio Feb 23, 2024

Choose a reason for hiding this comment

alessandroboron Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024 •

edited

Loading

alessandroboron Feb 26, 2024 •

edited

Loading

alessandroboron Feb 26, 2024 •

edited

Loading