Closed
Bug 1255077
Opened 8 years ago
Closed 8 years ago
Bookmarks menu item does nothing if bookmarks panel is disabled
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox46 fixed, firefox47 verified, firefox48 verified, fennec46+)
VERIFIED
FIXED
Firefox 48
People
(Reporter: mcomella, Assigned: liuche)
References
Details
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
Margaret
:
review+
|
Details |
10.27 KB,
patch
|
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We should either not show the menu item, or re-enable the bookmarks panel when the item is clicked. I assume this also works the same way for history.
Comment 1•8 years ago
|
||
Discussion in triage is that we're not sure whether we should somehow show the bookmarks panel when you tap on this menu item, or if we should hide the menu item. Anthony will come up with the UX for this. Chenxia, can you own coming up with a fix for this, whichever way we decide to go?
tracking-fennec: ? → 46+
Flags: needinfo?(liuche)
Flags: needinfo?(alam)
Assignee | ||
Comment 2•8 years ago
|
||
Sure - do we think this could be a mentored bug? If not, I can own taking care of it too.
Flags: needinfo?(liuche)
Comment 3•8 years ago
|
||
(In reply to Chenxia Liu [:liuche] from comment #2) > Sure - do we think this could be a mentored bug? If not, I can own taking > care of it too. We need a fix we can uplift to beta soon, so no, I don't think it should be a mentored bug (unless you have someone in mind who would be prepared for a quick turnaround).
Assignee: nobody → liuche
Comment 4•8 years ago
|
||
I've thought about this more and, all things considered, I think we should hide the menu item if they hide the panel. --- Strictly speaking, it's true that "Hide" from within General > Home > Panels, might be seen to only apply to Home panels. But I don't think we should reinforce this type of behaviour going forward. The point of these items in the menu is to address a UX issue of discovery. We've seen users have trouble finding these panels, and we've seen users go into the menu looking for these items (maybe because other browsers/apps have these types of shortcuts in their menu?). So, as we look at these items as shortcuts, we should disable them if the destination has been disabled. We can explore more fine grain customization in the future if we see that it's valuable. But in the mean time, I think it's important to maintain this relationship between the Bookmarks panel, and the Bookmarks menu shortcut.
Flags: needinfo?(alam) → needinfo?(liuche)
Comment 5•8 years ago
|
||
Thanks for the thoughtful response, I agree that makes sense. Right now, if a user disabled their bookmarks panel, there's no way for them to see a list of bookmarks, and clearly they're fine with that. So I don't think we need to give them a shortcut to the bookmarks panel in this case.
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40589/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40589/
Attachment #8731429 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8731429 [details] MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40589/diff/1-2/
Assignee | ||
Comment 8•8 years ago
|
||
Margaret, is there's a better way to fetch which home panels are enabled? I'm fetching from HomePager right now (which is higher in the UI stack than we need), but looking at the HomeConfigLoader code, it's not very straightforward to add my own OnLoadListener to check which panels are enabled/disabled. This uses SharedPreferences to keep track of enabled/hidden state of History or Bookmarks, and also updates that whenever a panel is enabled/disabled from preferences. If that pref doesn't exist, we set it from whatever the current configuration is in HomePager. This doesn't handle the case before the HomePager is shown and the user doesn't change the history/bookmark state from settings.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(liuche)
Comment 9•8 years ago
|
||
(In reply to Chenxia Liu [:liuche] from comment #8) > Margaret, is there's a better way to fetch which home panels are enabled? > I'm fetching from HomePager right now (which is higher in the UI stack than > we need), but looking at the HomeConfigLoader code, it's not very > straightforward to add my own OnLoadListener to check which panels are > enabled/disabled. Unfortunately, I don't think there's a better way, but reading directly from HomePager like this doesn't seem great :/ > This uses SharedPreferences to keep track of enabled/hidden state of History > or Bookmarks, and also updates that whenever a panel is enabled/disabled > from preferences. If that pref doesn't exist, we set it from whatever the > current configuration is in HomePager. > > This doesn't handle the case before the HomePager is shown and the user > doesn't change the history/bookmark state from settings. Given that a user can only enable/disable these things in settings, I think we should just set the SharedPreferences in settings, and we can introduce some migration logic to handle setting this when the user updates. I think we should avoid setting these prefs in multiple places, since it can become confusing to maintain.
Comment 10•8 years ago
|
||
Comment on attachment 8731429 [details] MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret https://reviewboard.mozilla.org/r/40589/#review37425 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3389 (Diff revision 2) > + } > + > + if (!prefs.contains(PanelsPreferenceCategory.PREF_KEY_HISTORY_PANEL_ENABLED)) { > + prefs.edit().putBoolean(PanelsPreferenceCategory.PREF_KEY_HISTORY_PANEL_ENABLED, mHomePager.containsPanel(PanelType.HISTORY)).apply(); > + } > + } My initial reaction is that this is a bad place to do this. Can we instead include this as migration logic somewhere in the home package (perhaps in HomeConfig?), so that we can avoid needing to expose more APIs on HomePager? We actually already have migration logic in HomeConfigPrefsBackend. Although we're not migrating JSON here, it seems like setting the preferences could work well there. ::: mobile/android/base/java/org/mozilla/gecko/preferences/PanelsPreferenceCategory.java:25 (Diff revision 2) > > public class PanelsPreferenceCategory extends CustomListCategory { > public static final String LOGTAG = "PanelsPrefCategory"; > > + public static final String PREF_KEY_BOOKMARKS_PANEL_ENABLED = "bookmarksPanelEnabled"; > + public static final String PREF_KEY_HISTORY_PANEL_ENABLED = "historyPanelEnabled"; Is this the right class to own these keys? Seems like it would be better to centralize this logic in one of the home classes.
Attachment #8731429 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•8 years ago
|
Attachment #8731429 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8731429 [details] MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40589/diff/2-3/
Comment 12•8 years ago
|
||
Comment on attachment 8731429 [details] MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret https://reviewboard.mozilla.org/r/40589/#review38231 ::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:358 (Diff revision 3) > // We should only make the banner active if the toolbar is not focused and we are on the default page > final boolean active = !hasFocus && getCurrentItem() == mDefaultPageIndex; > mHomeBanner.setActive(active); > } > > + private void updatePrefsFromConfigState(HomeConfig.State configState) { Add some comments to explain why we need to do this. ::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:361 (Diff revision 3) > } > > + private void updatePrefsFromConfigState(HomeConfig.State configState) { > + final SharedPreferences prefs = GeckoSharedPrefs.forProfile(getContext()); > + if (!prefs.contains(HomeConfig.PREF_KEY_BOOKMARKS_PANEL_ENABLED) > + || !prefs.contains(HomeConfig.PREF_KEY_HISTORY_PANEL_ENABLED)) { With this change, we're going to do this check every time the panels are loaded... I think we should do this in HomeConfigPrefsBacked, so we can at least avoid this for people who haven't customized their home panels at all. I still think it would make more sense to put this in `maybePerformMigration`: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java#186 We don't need to make this a proper migration, but we can at least do the check at the top of this method, which is only called if the user has customized their home panels.
Attachment #8731429 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•8 years ago
|
Attachment #8731429 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8731429 [details] MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40589/diff/3-4/
Comment 14•8 years ago
|
||
Comment on attachment 8731429 [details] MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret https://reviewboard.mozilla.org/r/40589/#review38265 Thanks for your patience with my reviews!
Attachment #8731429 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 15•8 years ago
|
||
No problem! I think these were really relevant comments and made the patch better :)
Assignee | ||
Comment 17•8 years ago
|
||
Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c5060a5dc0 Beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f501a41120c Approval Request Comment [Feature/regressing bug #]: Bug 1231792, added panels to menu but did not add code to remove them [User impact if declined]: Users who have customized their panels may have panel menu items that don't do anything [Describe test coverage new/current, TreeHerder]: local testing, try pushes [Risks and why]: low, adds a code path that sets some prefs for users with customized home panels - default is to do the same thing as the existing code [String/UUID change made/needed]: none
Attachment #8733619 -
Flags: approval-mozilla-beta?
Attachment #8733619 -
Flags: approval-mozilla-aurora?
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9de25eb6d07d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Hi Mike, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(michael.l.comella)
Reporter | ||
Comment 20•8 years ago
|
||
Looks good to me!
Status: RESOLVED → VERIFIED
Flags: needinfo?(michael.l.comella)
Comment on attachment 8733619 [details] [diff] [review] Aurora/Beta patch: Disable Bookmarks/History menu item if panels are not enabled Fix was verified, Aurora47+
Attachment #8733619 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox47:
--- → affected
status-firefox46:
--- → affected
Comment 22•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1806435f8228
Comment 23•8 years ago
|
||
Comment on attachment 8733619 [details] [diff] [review] Aurora/Beta patch: Disable Bookmarks/History menu item if panels are not enabled Fix for recent regression in bookmarks, please uplift to beta
Attachment #8733619 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 24•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/da8775314086d5f36fdc54262a114ffa59eb5745
Comment 25•8 years ago
|
||
Bookmarks menu item is hidden if bookmarks panel is disabled. Verified as fixed using: Device: Nexus 9 (Android 6.0) Build: Firefox for Android 47 Beta 1
Comment 26•8 years ago
|
||
Verified as fixed in build 48.0a2 2016-04-27; Device: Asus Transformer Pad (Android 4.2.1).
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•