Closed Bug 766940 Opened 9 years ago Closed 9 years ago

Unable to remove bookmarks from the menu, bookmark checkmark/filled star icon remain in view

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox15 + verified
firefox16 + verified
firefox17 --- verified
fennec 15+ ---

People

(Reporter: aaronmt, Assigned: bnicholson)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files, 4 obsolete files)

See screenshot.

Add a bookmark, remove a bookmark. 

Checkmark remains in view.

--
Nightly (06.21)
Galaxy Nexus (Android 4.0.4)
Keywords: reproducible
Lucas and Sriram were talking about this on IRC yesterday morning. Maybe they already figured out what caused this.
Adding Wes because I see lots of bookmark fun stuff on Inbound in bug 759041 (re comment #0, correction, filed against Inbound).
Good build: 06/03

Bad build: 06/04

Possible regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d0ebcaa7efb5&tochange=dd6ec482a85d

--
Device: Galaxy Nexus
OS: Android 4.0.2
I still can't remove a bookmark (the checkmark and now the star icon) remain.
Summary: Bookmark checkmark remains in view in the menu on bookmark removal → Unable to remove bookmarks from the menu, bookmark checkmark/filled star icon remain in view
Upping to major loss as it's the primary UI for bookmark removal
Severity: normal → major
Attached patch Patch (obsolete) — Splinter Review
This depends on "tab's" bookmark state and not on the menu item.
Also, the checked state is made to be proper.
Attachment #639524 - Flags: review?(mark.finkle)
Comment on attachment 639524 [details] [diff] [review]
Patch

We do the same kind of code in onPrepareOptionsMenu:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#592

Since the UI is now visible in the actionbar, we can't wait for onPrepareOptionsMenu to be called on it's own, so this patch works.

Could we just invalidate the menu here? Or is that overkill.

I don't like needing to update the UI all over the place. It doesn't scale well. It might be better to add an on change listener, so we can update the UI no matter how the tab bookmark state changes.

I assume we invalidate the menu when loading new pages? If so, let's do it here too, in case the UI changes we only need to update the code in the onPrepareOptionsMenu.

Thoughts?
I thought of doing that, but in one of the patches, Brad said that it's an overkill to call "invalidateOptionsMenu()" and go through everything, for a single menu item change. Hence, I changed the part that does it for bookmarks alone.

I agree that it's twice the hassle. But it's more of performance to do the part we want.
Comment on attachment 639524 [details] [diff] [review]
Patch

I disagree. In fact, as I look in the existing code, I see invalidateOptionsMenu used in many places to update a single menu... and I like it. It's also a pattern we seem to be using.

We should be centralizing the menu update code.

Please change this patch to remove the setChecked and setIcon calls, and add a single invalidateOptionsMenu call.
Attachment #639524 - Flags: review?(mark.finkle) → review-
Assigning to Sriram as he's working on it.
Assignee: nobody → sriram
I can reproduce this on Fx15 beta 1, ASUS transformer.  Please fix for beta 2!
tracking-fennec: ? → 15+
Sriram - Can we get an updated patch? We need this for Fx15
I tried using invalidateOptionsMenu(). That won't work.
We check for "tab.isBoomark()" and do a add/removeBookmark(). This basically sends a message to Gecko -- which responds -- which updates our Java side Tab. But invalidateOptionsMenu() is called before all these messaging happen. So, the state never gets updated.

The patch I posted doesn't wait for Gecko (just like now). It updates the state without waiting for Gecko.
If we send "a message to Gecko -- which responds -- which updates our Java side Tab" then we should be able to update the menu from that, right?
Basically we are updating a DB -- which informs a ContentResolver -- which calls an updateBoomark(). Do I want to do UI handling there?

If so, the optimization to make it look faster -- a tap and I bookmarked -- instead of waiting for all DB stuff to happen and then late update -- will be lost? Do we want to lose this "fast UI response" faking thing?
Let me be clear: I do not want UI menu update code strung out all over the source code.
I'll post a patch that might regress the time taken to update the star icon soon -- by calling invalidateOptionsMenu() from updateBookmark()
Why can't we add something like this to the BrowserToolbar or BrowserApp?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#107
(In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> I'll post a patch that might regress the time taken to update the star icon
> soon -- by calling invalidateOptionsMenu() from updateBookmark()

If you mean Tab.updateBookmark() please don't post a patch. That is too tightly coupled and I will r- it too.
(In reply to Mark Finkle (:mfinkle) from comment #18)
> Why can't we add something like this to the BrowserToolbar or BrowserApp?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.
> java#107

Doing something like this won't help either. If you look into updateBookmark(), it still calls the DB to know if the page is a bookmark or not and then updates its copy of mBookmark. Do we want to have an observer in BrowserApp/Toolbar that calls DB? The only place (other than my option 1) is to add it in Tab.updateBookmark() - after updating the mBookmark. I'm not up for adding a ContentObserver to BrowserApp/Toolbar unnecessarily, when we have that overhead with every Tab already.
Attached patch Patch (obsolete) — Splinter Review
Another r- ready patch (;)) :
This does the invalidateOptionsMenu() in Tab.updateBookmark(). The speed of icon change didnt regress. (which raises a question: why did we do it in 2 places earlier then?)
Attachment #644416 - Flags: review?(mark.finkle)
Comment on attachment 644416 [details] [diff] [review]
Patch

This is better, but we can make it even better:
* Add BOOKMARK as a TabEvent here http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#354
* Fire the tab listener, passing BOOKMARK, instead of directly calling invalidateOptionsMenu, like we do here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#207
* Add a case to handle BOOKMARK here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#65
(you can collapse the START and STOP cases along with it
Attachment #644416 - Flags: review?(mark.finkle) → review-
Assignee: sriram → bnicholson
[triage comment]
tracking for both 15/16 just to be on the safe side.
Just quick note: this bug also has to account for the reading list menu item (which behaves pretty much the same way than the bookmark one).
This is technically a separate bug, but I'll just put it here. I noticed that the "Reading List" menu item is always greyed out on the Nexus; we need to invalidate it here like we do in other places.
Attachment #639524 - Attachment is obsolete: true
Attachment #644416 - Attachment is obsolete: true
Attachment #645093 - Flags: review?(sriram)
Comment on attachment 645093 [details] [diff] [review]
Part 1: Invalidate options menu for soft menu button

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

This looks good to me.
Attachment #645093 - Flags: review?(sriram) → review+
Implemented comment 22. I made a generic MENU_UPDATED event so it could be used by both bookmarks and reader mode.
Attachment #645098 - Flags: review?(mbrubeck)
I merged updateBookmark() and updateReadingListItem() into a single method. They do virtually the same thing, so we might as well handle them both simultaneously for performance reasons (and avoid invalidating the menu twice every time). And a reading list item is technically a bookmark anyway.
Attachment #645098 - Attachment is obsolete: true
Attachment #645098 - Flags: review?(mbrubeck)
Attachment #645102 - Flags: review?(mbrubeck)
Comment on attachment 645102 [details] [diff] [review]
Part 2: Notify MENU_UPDATED TabEvent when menu items change, v2

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

::: mobile/android/base/GeckoApp.java
@@ +448,5 @@
>      }
>  
>      @Override
>      public void invalidateOptionsMenu() {
> +        Log.d("BRN", "invalidate");

Remember to remove this.
Attachment #645102 - Flags: review?(mbrubeck) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #28)
> Created attachment 645102 [details] [diff] [review]
> Part 2: Notify MENU_UPDATED TabEvent when menu items change, v2
> 
> I merged updateBookmark() and updateReadingListItem() into a single method.
> They do virtually the same thing, so we might as well handle them both
> simultaneously for performance reasons (and avoid invalidating the menu
> twice every time). And a reading list item is technically a bookmark anyway.

Brian, I think your patch is missing the case when the tab gets a Content:ReaderEnabled event and enables the the reading list menu item, no?
(In reply to Lucas Rocha (:lucasr) from comment #30)
> Brian, I think your patch is missing the case when the tab gets a
> Content:ReaderEnabled event and enables the the reading list menu item, no?

Thanks. I had filed bug 776740 for this, but since this is just because of the Content:ReaderEnabled message like you said, I'll just go ahead and fix it here.
Attachment #645102 - Attachment is obsolete: true
Attachment #645455 - Flags: review?(mbrubeck)
Attachment #645455 - Flags: review?(mbrubeck) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #31)
> Created attachment 645455 [details] [diff] [review]
> Part 2: Notify MENU_UPDATED TabEvent when menu items change, v3
> 
> (In reply to Lucas Rocha (:lucasr) from comment #30)
> > Brian, I think your patch is missing the case when the tab gets a
> > Content:ReaderEnabled event and enables the the reading list menu item, no?
> 
> Thanks. I had filed bug 776740 for this, but since this is just because of
> the Content:ReaderEnabled message like you said, I'll just go ahead and fix
> it here.

I confirm that your patch fixes bug 767959 too. Add a reference to it on the commit message.
Blocks: 767959
Comment on attachment 645093 [details] [diff] [review]
Part 1: Invalidate options menu for soft menu button

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ?
User impact if declined: menu will not be updated on phones with soft menu button
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #645093 - Flags: approval-mozilla-beta?
Attachment #645093 - Flags: approval-mozilla-aurora?
Comment on attachment 645455 [details] [diff] [review]
Part 2: Notify MENU_UPDATED TabEvent when menu items change, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ?
User impact if declined: bookmarks cannot be removed from menu
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #645455 - Flags: approval-mozilla-beta?
Attachment #645455 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 760452
https://hg.mozilla.org/mozilla-central/rev/8d64c2ce1cc0
https://hg.mozilla.org/mozilla-central/rev/9ce69db8aac3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Attachment #645093 - Flags: approval-mozilla-beta?
Attachment #645093 - Flags: approval-mozilla-beta+
Attachment #645093 - Flags: approval-mozilla-aurora?
Attachment #645093 - Flags: approval-mozilla-aurora+
Attachment #645455 - Flags: approval-mozilla-beta?
Attachment #645455 - Flags: approval-mozilla-beta+
Attachment #645455 - Flags: approval-mozilla-aurora?
Attachment #645455 - Flags: approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.