Closed Bug 1024145 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: mcomella, Assigned: treemantris, Mentored)

References

Details

(Keywords: reproducible, Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 3 obsolete files)

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
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
Summary: When removing bookmarks, display the toast only after the bookmark has been removed → When adding and removing bookmarks, display the toast only after the bookmark has been removed
Whiteboard: [mentor=mcomella][lang=java]
Whiteboard: [mentor=mcomella][lang=java] → [mentor=mcomella][lang=java][good first bug]
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java][good first bug] → [lang=java][good first bug]
Moved the Toast notification to removeBookmark() method of Tab
Attachment #8443463 - Flags: review?(michael.l.comella)
Attachment #8443463 - Attachment is obsolete: true
Attachment #8443463 - Flags: review?(michael.l.comella)
Added fixes for both addBookmark() and removeBookmark().
Attachment #8443652 - Flags: review?(michael.l.comella)
Attachment #8443652 - Attachment description: 1024145.patch → 1024145.patch - Fixes for addBookmark and removeBookmark
Assignee: nobody → hathymo
Status: NEW → ASSIGNED
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.
Attachment #8443652 - Flags: review?(michael.l.comella) → review-
Attachment #8443652 - Attachment is obsolete: true
Attachment #8443652 - Attachment is obsolete: false
Added fixes as suggested by :mcomella
Attachment #8443905 - Flags: review?(michael.l.comella)
Attachment #8443652 - Attachment is obsolete: true
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.
Attachment #8443905 - Flags: review?(michael.l.comella) → feedback+
Hey, Hathibelagal. Are you still working on this?
Flags: needinfo?(hathymo)
I am new to mozilla, can I take it up ?
If yes then please provide repo, docs details to start on.
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! ^_^
Assignee: hathymo → jskankurgarg
Flags: needinfo?(hathymo)
@Michael, thanks for sharing the instructions, it helped and now I am able to build and deploy fennec in my device.
Can you also share the approach for reproducing/debugging this bug ?
At first place, I am not absolutely clear about bug description, could you explain it from end-user persepective.
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.
Keywords: reproducible
Can you give a walk through of the relevant code section.
(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.
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?
(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.
Attached patch bookmark.patchSplinter Review
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?
Attachment #8471710 - Flags: review?(michael.l.comella)
Assignee: jskankurgarg → treemantris
Comment on attachment 8471710 [details] [diff] [review]
bookmark.patch

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

I like it.
Attachment #8471710 - Flags: review?(michael.l.comella) → review+
Try is closed (bug 1040308), so I'll push a test run once it opens back up.
Flags: needinfo?(michael.l.comella)
Attachment #8443905 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Any updates on this?
Flags: needinfo?(michael.l.comella)
My apologies, I didn't mean to clear my needinfo flag before, and I completely overlooked this! try push coming...
Flags: needinfo?(michael.l.comella)
Looking green, should be able to check-in :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/4243b005e874
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4243b005e874
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.