[Feature] Bookmarks Improvement #1441

4shutosh · 2021-09-22T07:17:45Z

Task/Issue URL:
#1438
#1439
#1440

PR related to the feature proposal mentioned in discussion #1442

Description

Users can create a bookmark folder from the edit bookmark screen.
Edit Bookmark shows the correct Bookmark Location, From BrowserFragment

Steps to test this PR

Add a bookmark
Edit the bookmark from the menu or from the snackbar
Select the Location
New bookmark folder can be created.
After creating a new bookmark folder, select the bookmark folder
Save : bookmark saved inside the new bookmark folder.

4shutosh · 2021-09-22T07:22:40Z

cmonfortep · 2021-09-22T07:47:48Z

@4shutosh thanks for your time here. As I posted in the different issues you created, let's first have a discussion before we introduce any improvement.

We accept contributions for bug fixes after we triage the issue and we add the label "help wanted".

For feature requests/improvements, you need to open a discussion first. Inside the discussion, we will have a nice chat to know more about the proposal, understand what's the user value, etc. That's because we will need to share the proposed change with our Product team and/or Design team. If it's aligned with our roadmap and we are happy with the idea then we will be able to give you support with the improvements.

Hope that's ok with you, let's close this PR for now. We can open it again once I get the internal approval. Thanks again, and I think the proposal makes sense, but we need to check internally first 👍

4shutosh · 2021-09-22T08:15:35Z

Thank you for the explanation @cmonfortep 🙌🏻

That's unfortunate but will surely do the required.

I've created a discussion here #1442

4shutosh · 2021-10-28T11:25:19Z

@cmonfortep can you please reopen this PR, I'll make it as a draft and ask for approval on completion.

# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

4shutosh · 2021-11-03T07:55:37Z

Behavioral changes introduced in this PR:

The edit bookmark shows the folder name now
We can now create a new bookmark folder while editing the bookmark from the edit option either from snackbar or from the menu options.

@cmonfortep please review.
Do let me know with any changes are required along with new tests to be written or not.

cmonfortep · 2021-11-03T08:38:14Z

Thanks @4shutosh.

"The edit bookmark shows the folder name now". Sorry if it was not clear, but we discarded de idea of showing folder names in the snackbar (see my comment: https://github.com/duckduckgo/Android/discussions/1442#discussioncomment-1551699)

4shutosh · 2021-11-03T08:41:20Z

Sorry for the confusion. Yes it is not on the snackbar but on the edit saved site screen.

Here the "Edited" is the bookmark folder name.

cmonfortep · 2021-11-03T08:44:59Z

Thanks. I will try to review the PR this week. Could you update the description? (just to reflect what this PR is finally addressing since we discarded some initial proposals)

cmonfortep · 2021-11-12T16:32:51Z

@4shutosh just to give you some heads up. Testing this right now, looks great. Our designer is taking a look, should give me feedback on Monday.

4shutosh · 2021-11-12T16:44:49Z

Thank you for the update 🙌🏻

cmonfortep · 2021-11-15T17:01:32Z

@4shutosh thank you for your patience.

Feedback from the design team was really positive, that's really cool! They only asked if we can take the user back to the edit screen when the user created the new folder (same as in chrome).

Feedback is optional and I haven't checked how complex it is, so let me know if you are interested (and if you have time, it's ok if you can't) or not. In case you want to contribute on that, you can do it here or in another PR.

4shutosh · 2021-11-16T02:00:49Z

That's great!

I'm interested in working on the feedback.

But I'll be able to do that (or any test related progress of this PR: as some tests are failing) after a week.

Please proceed with the tests fix if this feature is of priority for upcoming release within a week. In that case, I'll be continuing the feedback work on a new PR.

cmonfortep · 2021-11-16T13:50:56Z

Sounds good, I will review this PR as it is now, and further improvements can be done on a separated PR.

cmonfortep

Thanks @4shutosh, haven't reviewed all the changes. I've only focused on the dialogs and the browsertabviewmodel. Left some comments bellow.

While testing this, I've noticed an unexpected behavior: After editing a bookmark (changing folder), a snackbar saying "bookmark added" appears. Testing our latest app, the behavior is different. We only show the snackbar when the user adds a new bookmark, not after editing the bookmark.

app/src/main/java/com/duckduckgo/app/bookmarks/ui/SavedSiteDialogFragment.kt

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

…ewState

4shutosh · 2021-12-22T16:15:00Z

Sorry for the delay. @cmonfortep
I've done the improvements in the code as mentioned by you. Also removed the unnecessary snackbar behaviour after edit of bookmark.

This is getting stretched more than expected. I'll try my best to complete the changes related to this PR as early as possible now. Please help me with the review.

Thank you again. 🙌🏻

malmstein · 2022-01-05T12:49:56Z

@cmonfortep can you check on this?

cmonfortep · 2022-01-18T09:51:02Z

@4shutosh sorry for the delay here, Christmas break + workload got me busy.

I'm going to review this ASAP.

cmonfortep

Thanks and sorry for the delay in the review. As you said, this is taking longer than we expected and it was difficult to remember what was missing here.

I'm leaving some comments. So far it's looking good. Just something important to mention:

be sure new code is properly tested if the class has a test (otherwise is not necessary, e.g: activities/fragments)
when aligning with develop, you may get lot of conflicts. We have introduced changes in our code style, so expect some conflicts there.

Thanks.

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

cmonfortep · 2022-01-18T18:03:44Z

app/src/main/java/com/duckduckgo/app/bookmarks/model/BookmarksRepository.kt

@@ -82,6 +87,10 @@ class BookmarksDataRepository(
        return bookmarksDao.getBookmarksByParentIdSync(parentId)
    }

+    override suspend fun getBookmarkFolderByParentId(parentId: Long): BookmarkFolder? {
+        return bookmarkFoldersDao.getBookmarkFolders().firstOrNull()?.find { it.id == parentId }


Haven't checked, but I think filtering at SQL level is more efficient than getting the whole list of folders here, and then filtering. Can you create a new method in BookmarkFoldersDao, and just use it here?

Created a new method please check.

app/src/main/java/com/duckduckgo/app/bookmarks/ui/EditSavedSiteDialogFragment.kt

cmonfortep · 2022-01-18T18:25:03Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

+    private fun editSavedSite(savedSiteChangedViewState: SavedSiteChangedViewState) {
+        val addBookmarkDialog = EditSavedSiteDialogFragment.instance(
+            savedSiteChangedViewState.savedSite,
+            savedSiteChangedViewState.bookmarkFolder?.id ?: 0,


I want to check with someone from the team, what's the difference between savedSiteChangedViewState.bookmarkFolder?.id vs savedSiteChangedViewState.savedSite.parentId when we have a bookmark. So maybe I will ask to double check this part.

cmonfortep · 2022-01-18T18:26:15Z

app/src/main/java/com/duckduckgo/app/bookmarks/ui/bookmarkfolders/BookmarkFoldersViewModel.kt

+        viewState.value = currentViewState().copy(currentFolder = currentFolder, selectedFolderId = selectedFolderId)
+    }
+
+    fun newFolderAdded(rootFolderName: String) {


Test missing for this.

Added please check

app/src/main/java/com/duckduckgo/app/bookmarks/ui/bookmarkfolders/BookmarkFoldersViewModel.kt

app/src/main/java/com/duckduckgo/app/bookmarks/ui/bookmarkfolders/BookmarkFoldersActivity.kt

4shutosh · 2022-01-20T11:48:39Z

Feedback from the design team was really positive, that's really cool! They only asked if we can take the user back to the edit screen when the user created the new folder (same as in chrome).

I tried to work on this. This can be done in a couple of ways as per my understanding.

1

    override fun onBookmarkFolderAdded(bookmarkFolder: BookmarkFolder, exitAndSetSelected: Boolean) {
        viewModelScope.launch(dispatcherProvider.io()) {
            bookmarksRepository.insert(bookmarkFolder)
        }
        if (exitAndSetSelected) onItemSelected(bookmarkFolder)
        else command.value = NewFolderCreatedUpdateTheStructure
    }

But this will not give us the ideal behavior and the list will be visible for a split second.

Removing the list screen (Select Location) from back stack and maybe implementing the interface of folder addition directly on the Edit Bookmark screen. But this is not possible in our case as the Select Location list screen i.e BookmarkFoldersActivity is an activity. All the screens inside the chrome bookmark flow would be Fragments.

Please take a look and let me know any other changes required.

cmonfortep · 2022-01-20T12:13:32Z

Feedback was optional. If you want to work on that, it's awesome! but my proposal is to do it on another PR once this one is merged. Just to avoid delaying more this feature.

4shutosh · 2022-01-21T05:27:06Z

Sure I'll do that.
I've done all the changes required, resolved merge conflicts and the feature is working as expected. Please let me know any other changes required.

cmonfortep

Thanks @4shutosh. We are almost done here 🎉

I've just left one last comment to cover with a unit test the method in the repository. I've tested the functionality and it works as expected.

cmonfortep · 2022-01-28T09:25:28Z

app/src/main/java/com/duckduckgo/app/bookmarks/model/BookmarksRepository.kt

@@ -96,6 +100,10 @@ class BookmarksDataRepository(
        return bookmarksDao.getBookmarksByParentIdSync(parentId)
    }

+    override suspend fun getBookmarkFolderByParentId(parentId: Long): BookmarkFolder {


Can you add a test to cover this method?

What will happen when parentId is not found or doesn't return any folder? Please also test that scenario.

I've added the test.

In that case, the bookmark is treated as if it is stored in the default root folder under the name "Bookmarks".

cmonfortep

Thanks for the changes @4shutosh LGTM!

Please, solve the conflicts with develop (probably the test has been moved into unit tests folder), and as soon as the check is green we will merge it. :)

cmonfortep · 2022-02-01T16:49:23Z

Sharing here the stacktrace of the failing test.

com.duckduckgo.app.bookmarks.model.BookmarksDataRepositoryTest > whenGetBookmarkFolderByParentIdThenReturnBookmarkFolderForParentId[test(AVD) - 5.1.1] FAILED 
	java.lang.AssertionError: expected:<BookmarkFolder(id=1, name=name, parentId=11, numBookmarks=0, numFolders=0)> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)

4shutosh · 2022-02-01T16:53:13Z

Yes, got the problem in the test. I've pushed a commit with a change in the test. The tests should pass now.

4shutosh · 2022-02-01T17:45:17Z

Yayyy 🎉 thank you for your guidance @cmonfortep 🙌🏻

4shutosh added 3 commits September 22, 2021 12:39

Bookmarks Improvement

e8f177e

redundant timber

4413c26

used : instead of in

8948e71

cmonfortep closed this Sep 22, 2021

cmonfortep reopened this Oct 28, 2021

4shutosh marked this pull request as draft October 28, 2021 11:38

4shutosh added 3 commits October 29, 2021 23:10

Merge branch 'develop' into feature_add_bookmark_folder_from_edit

bcb020a

# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

folder name and saving after merge: no tests

d0d1b70

spotless

e0a83f6

cmonfortep added the hacktoberfest-accepted label Oct 31, 2021

4shutosh added 5 commits November 1, 2021 16:11

selected folder stays selected

a649340

spotless

0609e45

better name

cb94914

tests fixed

a634897

code cleanup

4de3724

4shutosh marked this pull request as ready for review November 3, 2021 07:55

cmonfortep self-assigned this Nov 3, 2021

4shutosh mentioned this pull request Dec 9, 2021

[Bug] Edit Bookmark does not display the correct bookmark location. #1438

Closed

cmonfortep requested changes Dec 13, 2021

View reviewed changes

app/src/main/java/com/duckduckgo/app/bookmarks/ui/SavedSiteDialogFragment.kt Outdated Show resolved Hide resolved

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt Outdated Show resolved Hide resolved

4shutosh added 4 commits December 22, 2021 21:31

imports cleanup

37ebc99

unnecessary snackbar removed on edit & bookmarkFolder removed from vi…

0bbd7b0

…ewState

configureClickListener moved back to parent class

4dd6e88

removed bookmarkFolder from viewState

84ca835

cmonfortep reviewed Jan 18, 2022

View reviewed changes

"getBookmarkFolderByParentId" added & code improvement

ef7fdc9

cmonfortep reviewed Jan 19, 2022

View reviewed changes

app/src/main/java/com/duckduckgo/app/bookmarks/ui/bookmarkfolders/BookmarkFoldersActivity.kt Show resolved Hide resolved

4shutosh added 4 commits January 19, 2022 23:28

develop merge and resolution of conflicts: build success

820099b

spotless

aefaef3

code improvement

01c8148

test added & spotless

f8b2308

cmonfortep reviewed Jan 28, 2022

View reviewed changes

4shutosh added 2 commits January 28, 2022 15:38

test added for getBookmarkFolderByParentId

5985dee

null case for bookmarkFolder from dao using parentId

6420445

cmonfortep approved these changes Feb 1, 2022

View reviewed changes

4shutosh added 2 commits February 1, 2022 21:30

Merge branch 'develop' into feature_add_bookmark_folder_from_edit

5b5f387

test fix

14b61bb

merge and resolve conflict

f16e59b

cmonfortep merged commit 4fa7a13 into duckduckgo:develop Feb 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Bookmarks Improvement #1441

[Feature] Bookmarks Improvement #1441

4shutosh commented Sep 22, 2021 •

edited

4shutosh commented Sep 22, 2021 •

edited

cmonfortep commented Sep 22, 2021

4shutosh commented Sep 22, 2021

4shutosh commented Oct 28, 2021

4shutosh commented Nov 3, 2021

cmonfortep commented Nov 3, 2021

4shutosh commented Nov 3, 2021 •

edited

cmonfortep commented Nov 3, 2021

cmonfortep commented Nov 12, 2021

4shutosh commented Nov 12, 2021

cmonfortep commented Nov 15, 2021

4shutosh commented Nov 16, 2021

cmonfortep commented Nov 16, 2021

cmonfortep left a comment

4shutosh commented Dec 22, 2021

malmstein commented Jan 5, 2022

cmonfortep commented Jan 18, 2022

cmonfortep left a comment

cmonfortep Jan 18, 2022

4shutosh Jan 21, 2022

cmonfortep Jan 18, 2022

cmonfortep Jan 18, 2022

4shutosh Jan 21, 2022

4shutosh commented Jan 20, 2022

cmonfortep commented Jan 20, 2022

4shutosh commented Jan 21, 2022

cmonfortep left a comment

cmonfortep Jan 28, 2022

4shutosh Jan 28, 2022

cmonfortep left a comment

cmonfortep commented Feb 1, 2022

4shutosh commented Feb 1, 2022

4shutosh commented Feb 1, 2022

[Feature] Bookmarks Improvement #1441

[Feature] Bookmarks Improvement #1441

Conversation

4shutosh commented Sep 22, 2021 • edited

Description

Steps to test this PR

4shutosh commented Sep 22, 2021 • edited

cmonfortep commented Sep 22, 2021

4shutosh commented Sep 22, 2021

4shutosh commented Oct 28, 2021

4shutosh commented Nov 3, 2021

cmonfortep commented Nov 3, 2021

4shutosh commented Nov 3, 2021 • edited

cmonfortep commented Nov 3, 2021

cmonfortep commented Nov 12, 2021

4shutosh commented Nov 12, 2021

cmonfortep commented Nov 15, 2021

4shutosh commented Nov 16, 2021

cmonfortep commented Nov 16, 2021

cmonfortep left a comment

Choose a reason for hiding this comment

4shutosh commented Dec 22, 2021

malmstein commented Jan 5, 2022

cmonfortep commented Jan 18, 2022

cmonfortep left a comment

Choose a reason for hiding this comment

cmonfortep Jan 18, 2022

Choose a reason for hiding this comment

4shutosh Jan 21, 2022

Choose a reason for hiding this comment

cmonfortep Jan 18, 2022

Choose a reason for hiding this comment

cmonfortep Jan 18, 2022

Choose a reason for hiding this comment

4shutosh Jan 21, 2022

Choose a reason for hiding this comment

4shutosh commented Jan 20, 2022

cmonfortep commented Jan 20, 2022

4shutosh commented Jan 21, 2022

cmonfortep left a comment

Choose a reason for hiding this comment

cmonfortep Jan 28, 2022

Choose a reason for hiding this comment

4shutosh Jan 28, 2022

Choose a reason for hiding this comment

cmonfortep left a comment

Choose a reason for hiding this comment

cmonfortep commented Feb 1, 2022

4shutosh commented Feb 1, 2022

4shutosh commented Feb 1, 2022

4shutosh commented Sep 22, 2021 •

edited

4shutosh commented Sep 22, 2021 •

edited

4shutosh commented Nov 3, 2021 •

edited