1024145 - When adding and removing bookmarks, display the toast only after the bookmark has been removed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Like bug 918494.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We queue the async action to remove bookmarks at [1], then trigger the toast. From a UX perspective, the user should probably only receive the toast once the bookmark has actually been removed.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=b72560289748#2431

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Actually, there is an alternate code path to worry about [1] from bug 918494 so this applies to adding bookmarks as well.

Two approaches that I see are to add the toasts to the tab methods, i.e. Tab.*BookmarkAndToast, or to pull the code out from the Tab class. I'm leaning towards the former.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=b72560289748#2435

Comment 3

[Hathibelagal A]

Attachment: 1024145.patch - Moved the Toast from BrowserApp.java to Tab.java

Moved the Toast notification to removeBookmark() method of Tab

Comment 4

[Hathibelagal A]

Attachment: 1024145.patch - Fixes for addBookmark and removeBookmark

Added fixes for both addBookmark() and removeBookmark().

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8443652 [details] [diff] [review]
1024145.patch - Fixes for addBookmark and removeBookmark

Review of attachment 8443652 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +2415,5 @@
>  
>          return true;
>      }
>  
> +    void showBookmarkAddedToast(){

nit: It is only my opinion but `protected` is generally preferable.

@@ +2452,3 @@
>                      item.setIcon(R.drawable.ic_menu_bookmark_add);
>                  } else {
> +                    tab.addBookmark(BrowserApp.this);                    

nit: We prefer not to add excess whitespace to the end of lines - can you remove it here? Let me know if you're curious for an explanation.

::: mobile/android/base/Tab.java
@@ +446,5 @@
>              }
>          });
>      }
>  
> +    public void addBookmark(final BrowserApp browserApp) {

The Tab class should not have to know about BrowserApp - is there another way you can do this? Can you use a listener or pass in the ButtonToast (I think I prefer the former, because a tab should be able to add/remove a bookmark with explicitly notifying the user, and use the listener gives more flexibility to that end)?

@@ +455,5 @@
>                  if (url == null)
>                      return;
>  
>                  BrowserDB.addBookmark(getContentResolver(), mTitle, url);
> +                

nit: Excess end-of-line whitespace.

Comment 6

[Hathibelagal A]

Attachment: p2.patch - Added fixes so that Tab.java doesn't have to know about BrowserApp

Added fixes as suggested by :mcomella

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8443905 [details] [diff] [review]
p2.patch - Added fixes so that Tab.java doesn't have to know about BrowserApp

Review of attachment 8443905 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review until the questions are answered.

::: mobile/android/base/BrowserApp.java
@@ +251,5 @@
>              case MENU_UPDATED:
>                  if (Tabs.getInstance().isSelectedTab(tab)) {
>                      invalidateOptionsMenu();
>                  }
> +                if(data!=null) {

nit: Coding style. We put spaces between operators (e.g. `data != null`) and between the if keyword and parens (e.g. `if (...`). Here and below.

See the rest of the file for examples.

::: mobile/android/base/Tab.java
@@ +454,5 @@
>                  if (url == null)
>                      return;
>  
>                  BrowserDB.addBookmark(getContentResolver(), mTitle, url);
> +                Tabs.getInstance().notifyListeners(Tab.this, Tabs.TabEvents.MENU_UPDATED, "Bookmark:Added");

Why are using the MENU_UPDATED event here? I think BrowserDB.addBookmark should notify the listeners of this event anyway.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey, Hathibelagal. Are you still working on this?

Comment 9

[Ankur Garg]

I am new to mozilla, can I take it up ?
If yes then please provide repo, docs details to start on.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hathibelagal, I'm unassigning you and assigning Ankur.

Welcome to Bugzilla, Ankur! :)

To set up a build environment, you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

Comment 11

[Ankur Garg]

@Michael, thanks for sharing the instructions, it helped and now I am able to build and deploy fennec in my device.

Comment 12

[Ankur Garg]

Can you also share the approach for reproducing/debugging this bug ?

Comment 13

[Ankur Garg]

At first place, I am not absolutely clear about bug description, could you explain it from end-user persepective.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Currently, when bookmarks are removed, we are saving the data asynchronously. After the asynchronous action is queued, we display a toast saying the bookmark has been removed. However, we should really wait until *after* the bookmark is removed from the database, i.e. the asynchronous action completed, before displaying the toast.

If the remove action is fast, it will be no different from the end-user perspective. If the remove action is slow, the users will experience a delay before the toast is delayed after removing the bookmark. However, this is more accurate to what is occuring in the system and gives us the opportunity to catch errors with removing the bookmark, and display an appropriate toast.

Similarly, there are also some similar issues with adding bookmarks mentioned in comment 2.

These can be reproduced by selecting, and then deselecting the bookmarks star from the options menu.

Comment 15

[Ankur Garg]

Can you give a walk through of the relevant code section.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Ankur Garg from comment #15)
> Can you give a walk through of the relevant code section.

I'm not sure how to do that - do you have specific questions? comment 1 and comment 2 should point you in the right direction.

Comment 17

[Tristan Pollitt]

I was thinking of taking this one up, if no one else is working on it.

(In reply to Michael Comella (:mcomella) from comment #7)
> Why are using the MENU_UPDATED event here? I think BrowserDB.addBookmark
> should notify the listeners of this event anyway.

Following on from this, I couldn't find anywhere in the BrowserDB code that notifies the Tabs listeners when a bookmark is added/removed. Perhaps we could add BOOKMARK_ADDED/REMOVED to the TabEvent enum, rather than using MENU_UPDATED?

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Tristan Pollitt from comment #17)
> I was thinking of taking this one up, if no one else is working on it.

Ankur last commented only 2 days ago, so we should give him an opportunity to take a stab at this.

Comment 19

[Tristan Pollitt]

Attachment: bookmark.patch

It's been a week since Ankur's last comment, so it seems like he's no longer working on it. Here's a patch I've created, which I've tested manually on Android 4.1.2, are there any automated tests that need to be updated or created for this?

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8471710 [details] [diff] [review]
bookmark.patch

Review of attachment 8471710 [details] [diff] [review]:
-----------------------------------------------------------------

I like it.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Try is closed (bug 1040308), so I'll push a test run once it opens back up.

Comment 22

[Tristan Pollitt]

Any updates on this?

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

My apologies, I didn't mean to clear my needinfo flag before, and I completely overlooked this! try push coming...

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try: https://tbpl.mozilla.org/?tree=Try&rev=bfc3a8283020

Comment 25

[Tristan Pollitt]

Looking green, should be able to check-in :)

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/4243b005e874

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4243b005e874