Closed Bug 574511 Opened 14 years ago Closed 14 years ago

Make bookmarks button customize-able (movable and removable by users)

Categories

(Firefox :: Bookmarks & History, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b2

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [Input])

Attachments

(1 file, 7 obsolete files)

Original patch had this functionality, but it did not make it, try to reintroduce it without ugly hacks.
No longer blocks: 544817
Depends on: 544817
Looks like we have a partial workaround for now.  Cmad in nightly thread writes:

"I just found out that you can indeed move the search field and any other [navigation] toolbar elements to the right of the [new] Bookmarks [navigation] widget...I just had to move other stuff to the right of it instead of moving the widget itself to the left."

STR:
Turn off Menu Bar
Turn Off Bookmarks Toolbar
Go to Customize->Toolbar Layout
Drag and Drop widgets to the right of the Bookmarks Widget
Close Toolbar Layout

Everything still works the same.
Comment #1 is a workaround, not a full solution for a customizable bookmarks menu button.
Attached patch PoC v1.0 (obsolete) — Splinter Review
this is a proof of concept of a new approach, should be far less hackish than the previous (that involved playing with currentSet, defaultSet and whatever), indeed, apart some renaming, are just few lines of code.
On customizeStart the button is moved out of its wrapper, on customizeDone the wrapper is moved to the button's position and button is moved back inside it.
This allows to handle already customized toolbars and new toolbars, the magic movement with bookmarks-toolbar-items is preserved, user can choice button position when bookmarks toolbar is not hidden or remove the button putting it back in the palette.
This is not ready for review since I've still to do a plethora of testing, but before going on I'd like to get feedback if this is a better approach than the previous and something we could take.
Attachment #454723 - Flags: feedback?(dao)
Comment on attachment 454723 [details] [diff] [review]
PoC v1.0

looks like this might work reliably at first glance
Attachment #454723 - Flags: feedback?(dao) → feedback+
From first tests I did it worked great... clearly I need to cleanup and test on all platforms. plus update my b-c test to check it.
Thanks for looking at it.
Attached patch patch v1.0 (obsolete) — Splinter Review
From the PoC I had to fix some issues:
- changes were not persisted across restarts, this was due to the fact customizeDone is called after sets are persisted.  So now I move the (wrapped) container on customizeChange.
- style was wrong when dropping on #TabsToolbar
- if the user moves the button to the same toolbar as bookmarksItems he probably wants any bookmark related thing in that toolbar, and wants the button in that specific position, so don't switch position.
- this includes the suggested "autohide" check from bug 575437, it's just one line, if it's fine I can just use it, otherwise will remove it.
Assignee: nobody → mak77
Attachment #454723 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #455002 - Flags: review?(dao)
Attached patch patch v1.1 (obsolete) — Splinter Review
found a small style bug
Attachment #455002 - Attachment is obsolete: true
Attachment #455004 - Flags: review?(dao)
Attachment #455002 - Flags: review?(dao)
Does this cover icon vs (icon and text) mode while on the bookmarks toolbar?
(In reply to comment #8)
> Does this cover icon vs (icon and text) mode while on the bookmarks toolbar?

clarify what do you mean by "on the bookmarks toolbar", if you mean "when the button in on Personal Toolbar" yes.
(In reply to comment #10)
> (In reply to comment #8)
> > Does this cover icon vs (icon and text) mode while on the bookmarks toolbar?
> 
> clarify what do you mean by "on the bookmarks toolbar", if you mean "when the
> button in on Personal Toolbar" yes.

Under the default scenario, if a user wants to use icons only for all toolbars, will that work for the bookmarks toolbar when the bookmarks toolbar is open when this bug is fixed?  Or will there still be an icon + text there? 

Not sure where/what personal toolbar is, just going by the default menu naming convention here ;)
hm, bookmarks items don't respect icons only mode, regardless how the toolbar is setup and this button follows same rule when it's added to bookmarks items.
Maybe it should respect mode if bookmarks are not on Personal Toolbar (the toolbar where bookmarks live by default). But looks more like a polish followup, I'm unsure how we want to handle the case, it would complicate a bit styling, not crazy with current patch, but will leave the call to Dao if I should do that or not.
Ok, then it sounds like another bug for that makes sense then to figure out how it should be addressed since default design on the mockups aren't setup that way, not trying to push for it here though.
There's a problem with the way bookmarks-menu-button-container is added to existing profiles with a customized nav-bar: It will be appended as the last child, after the "fullscreenflex" and "window-controls". We should probably migrate the currentset like we did for Firefox 3: http://hg.mozilla.org/mozilla-central/annotate/c5c14a904af4/browser/components/nsBrowserGlue.js#l955
This would allow us to make bookmarks-menu-button-container directly customizable, I think.
Do you mean that happens in current beta?
Allowing to customize directly the container would save a bunch of work here.
Would not touching the currentSet cause headaches with the urlbar|search splitter, as you said me before?
(In reply to comment #15)
> Do you mean that happens in current beta?

Yes.

> Allowing to customize directly the container would save a bunch of work here.
> Would not touching the currentSet cause headaches with the urlbar|search
> splitter, as you said me before?

Not when doing it in nsBrowserGlue.js, as you're not dealing with live toolbars there.
Oh, we add that splitter later. Cool, will look at this migration.
Attachment #455004 - Flags: review?(dao)
Attached patch patch v2.0 (obsolete) — Splinter Review
Attachment #455004 - Attachment is obsolete: true
Attachment #456615 - Flags: review?(dao)
FYI, there's a lot of input coming in for help on being able to move and/or remove the bookmarks toolbar http://input.mozilla.com/search/?q=bookmark+button&product=firefox
Whiteboard: [Input]
Comment on attachment 456615 [details] [diff] [review]
patch v2.0

>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>
>+  get buttonContainer() {
>+    delete this.buttonContainer;
>+    return this.buttonContainer =
>              document.getElementById("bookmarks-menu-button-container");
>   },

You probably don't really want to delete this getter.

>     bookmarksToolbarElt.collapsed =
>-      this.button.parentNode == this.bookmarksToolbarItem;
>+      this.button && this.button.parentNode == this.bookmarksToolbarItem;

You should assign "button" to a local variable in such cases, since it's not a smart getter anymore.

>-    if (bookmarksToolbarItem && !bookmarksToolbarItem.parentNode.collapsed) {
>+    if (bookmarksToolbarItem && !bookmarksToolbarItem.parentNode.collapsed &&
>+        this.buttonContainer.parentNode != bookmarksToolbarItem.parentNode &&
>+        bookmarksToolbarItem.parentNode.getAttribute("autohide") != "true") {

Can you make this more readable? The collapsed and autohide stuff should be grouped somehow, for instance.

>   customizeStart: function BMB_customizeStart() {
>-    var bmToolbarItem = this.bookmarksToolbarItem;
>-    if (this.button.parentNode == bmToolbarItem)
>-      bmToolbarItem.removeChild(this.button);
>+    this._resetView();
>+    if (this.button && this.button.parentNode != this.buttonContainer) {
>+      // Move button back to the container, so user can move or remove it.
>+      this.buttonContainer.appendChild(this.button);
>+      this._updateStyle();
>+    }

this.buttonContainer can be null here.

>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
> 
>-// Factory object
>+XPCOMUtils.defineLazyServiceGetter(this, "idleService",
>+                                   "@mozilla.org/widget/idleservice;1",
>+                                   "nsIIdleService");
>+
>+XPCOMUtils.defineLazyGetter(this, "distributionCustomizer", function() {
>+                              Cu.import("resource:///modules/distribution.js");
>+                              return new DistributionCustomizer();
>+                            });
>+
>+XPCOMUtils.defineLazyServiceGetter(this, "idleService",
>+                                   "@mozilla.org/widget/idleservice;1",
>+                                   "nsIIdleService");
>+
>+XPCOMUtils.defineLazyServiceGetter(this, "RDF",
>+                                   "@mozilla.org/rdf/rdf-service;1",
>+                                   "nsIRDFService");
>+
>+XPCOMUtils.defineLazyGetter(this, "localStore", function() {
>+  return RDF.GetDataSource("rdf:local-store");
>+});
>+
>+XPCOMUtils.defineLazyGetter(this, "sanitizer",
>+  function() {
>+    let sanitizerScope = {};
>+    Services.scriptloader.loadSubScript("chrome://browser/content/sanitize.js", sanitizerScope);
>+    return sanitizerScope.Sanitizer;
>+  });
>+
> const BrowserGlueServiceFactory = {
>   _instance: null,
>   createInstance: function BGSF_createInstance(outer, iid) {
>     if (outer != null)
>       throw Components.results.NS_ERROR_NO_AGGREGATION;
>     return this._instance == null ?
>       this._instance = new BrowserGlue() : this._instance;
>   }
> };
> 
>-// Constructor
>-
> function BrowserGlue() {
>-  XPCOMUtils.defineLazyServiceGetter(this, "_idleService",
>-                                     "@mozilla.org/widget/idleservice;1",
>-                                     "nsIIdleService");
>-
>-  XPCOMUtils.defineLazyGetter(this, "_distributionCustomizer", function() {
>-                                Cu.import("resource:///modules/distribution.js");
>-                                return new DistributionCustomizer();
>-                              });
>-
>-  XPCOMUtils.defineLazyGetter(this, "_sanitizer",
>-    function() {
>-      let sanitizerScope = {};
>-      Services.scriptloader.loadSubScript("chrome://browser/content/sanitize.js", sanitizerScope);
>-      return sanitizerScope.Sanitizer;
>-    });
>-
>   this._init();
> }

Please avoid these changes.

>         if (!currentset) {
>-          // toolbar isn't customized
>+          // Toolbar isn't customized.
>           if (i == 0)
>-            // new button is in the defaultset, nothing to migrate
>+            // The new button is in defaultset of this toolbar, so nothing to migrate.
>             break;
>           continue;
>         }
>         if (/(?:^|,)unified-back-forward-button(?:$|,)/.test(currentset))
>-          // new button is already there, nothing to migrate
>+          // New button has already been added, nothing to migrate.

ditto

>+    if (currentUIVersion < 2) {
>+      // This code adds the customizable bookmarks button.
>+      let currentsetResource = RDF.GetResource("currentset");
>+      let toolbarResource = RDF.GetResource("chrome://browser/content/browser.xul#nav-bar");
>+      let currentset = this._getPersist(toolbarResource, currentsetResource);
>+      // Need to migrate only if toolbar is customized and the element is not found.
>+      if (currentset && !/(?:^|,)bookmarks-menu-button-container(?:$|,)/.test(currentset)) {
>+        currentset += ",bookmarks-menu-button-container";

This is wrong, you need to add the container before the fullscreenflex.
Attachment #456615 - Flags: review?(dao) → review-
(In reply to comment #22)
> This is wrong, you need to add the container before the fullscreenflex.

I tried that first, but fullscreenflex is not in currentset, maybe just because it's not removable, and same for window-controls, thus I just appended.
I guess it would be possible to move a button after fullscreenflex while in fullscreen and get it in currentset (will try). In such a case I should probably first check if fullscreenflex exists, if it does add before it, otherwise append.
hm, now I see it, could be I tried to migrate too old profiles or bogus profiles, I'm sure i printed out currentset and it was stopping at searchbar :\
Oh ok, I was not drunk, this is what I see when I upgrade a Firefox 3.6.x profile after moving home button before refresh button:
"unified-back-forward-button,back-button,forward-button,home-button,reload-button,stop-button,urlbar-container,search-container"
Attached patch patch v2.1 (obsolete) — Splinter Review
addressed comments
Attachment #456615 - Attachment is obsolete: true
Attachment #457824 - Flags: review?(dao)
Comment on attachment 457824 [details] [diff] [review]
patch v2.1

>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js

>+    if (button.parentNode == this.bookmarksToolbarItem ||
>+        containerOnPersonalToolbar) {
>+      button.classList.add("bookmark-item");
>+      button.classList.remove("toolbarbutton-1");
>+    }
>+    else {
>+      button.classList.remove("bookmark-item");
>+      button.classList.add("toolbarbutton-1");
>     }

Not sure who started with this and why, but } else { looks nicer to me...

>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js

>-      let currentsetResource = this._rdf.GetResource("currentset");
>+      let currentsetResource = this._RDF.GetResource("currentset");

>-        let toolbar = this._rdf.GetResource("chrome://browser/content/browser.xul#" + toolbars[i]);
>-        let currentset = this._getPersist(toolbar, currentsetResource);
>+        let toolbarResource = this._RDF.GetResource("chrome://browser/content/browser.xul#" + toolbars[i]);
>+        let currentset = this._getPersist(toolbarResource, currentsetResource);

>-      var oldTarget = this._dataSource.GetTarget(aSource, aProperty, true);
>+      let oldTarget = this._localStore.GetTarget(aSource, aProperty, true);
>       if (oldTarget) {
>         if (aTarget)
>-          this._dataSource.Change(aSource, aProperty, oldTarget, this._rdf.GetLiteral(aTarget));
>+          this._localStore.Change(aSource, aProperty, oldTarget, this._RDF.GetLiteral(aTarget));
>         else
>-          this._dataSource.Unassert(aSource, aProperty, oldTarget);
>+          this._localStore.Unassert(aSource, aProperty, oldTarget);
>       }
>       else {
>-        this._dataSource.Assert(aSource, aProperty, this._rdf.GetLiteral(aTarget), true);
>+        this._localStore.Assert(aSource, aProperty, this._RDF.GetLiteral(aTarget), true);

I'd like to avoid unnecessary code churn in nsBrowserGlue, and it doesn't look like there was anything wrong with this code prior to your changes.
Attachment #457824 - Flags: review?(dao)
(In reply to comment #27)
> Comment on attachment 457824 [details] [diff] [review]
> >+    else {
> >+      button.classList.remove("bookmark-item");
> >+      button.classList.add("toolbarbutton-1");
> >     }
> 
> Not sure who started with this and why, but } else { looks nicer to me...

Places code style does not use cuddled else, so it's consistent with it. a Bunch of browser code also does not use cuddled else. devs don't agree on some points of the code style and the discussion has taken far more time then needed. I just tend to be consistent with the module that the code is part of.


> I'd like to avoid unnecessary code churn in nsBrowserGlue, and it doesn't look
> like there was anything wrong with this code prior to your changes.

Well, toolbar is not a toolbar, dataSource is a generic name that does not mean a thing. RDF is usually uppercase, so I tried to be consistent. but I can revert that.
(In reply to comment #28)
> > I'd like to avoid unnecessary code churn in nsBrowserGlue, and it doesn't look
> > like there was anything wrong with this code prior to your changes.
> 
> Well, toolbar is not a toolbar, dataSource is a generic name that does not mean
> a thing. RDF is usually uppercase, so I tried to be consistent. but I can
> revert that.

Please do. "toolbar" couldn't be a DOM node, as there's no document; "dataSource" is generic but private, so it doesn't matter.
Attached patch patch v2.2 (obsolete) — Splinter Review
reduced changes to nsBrowserGlue
Attachment #457824 - Attachment is obsolete: true
Attachment #458342 - Flags: review?(dao)
Why are you removing the _dirty flag? _setPersist used to handle this automatically, now you're tracking it manually for each _setPersist call. Doesn't seem beneficial.
it was used only internally to the calling method, so it can track it by itself. Tracking it in the component looks like polluting it with stuff from a single method.
Maybe we should rip all the stuff related to this out and put it in a separate object. I wouldn't suggest doing that here, but keeping _dirty seems like a sensible preparation (we wouldn't be polluting anything in a separate object).
Another option would be to inline setPersist, I guess.
(In reply to comment #34)
> Another option would be to inline setPersist, I guess.

and getPersist for consistency? As you prefer, I also thought about splitting all of this to a new object, but it was going to change far more rows with whitespace changes. Same if i inline. So maybe I can just restore _dirty for now, let me know your preferred solution. I probably vote for the smallest change.
Since tracking the dirtiness in _setPersist seems basically right I'd vote for keeping it as is and filing a new bug for consolidating the code, be it by adding a separate object or by inlining stuff.
Comment on attachment 458342 [details] [diff] [review]
patch v2.2

> #TabsToolbar > toolbarbutton,
>+#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button,
> #TabsToolbar > toolbarpaletteitem > toolbarbutton {
>   margin-bottom: 1px;
> }

Please keep the "toolbarbutton" and "toolbarpaletteitem > toolbarbutton" grouped together (appears multiple times in this patch).
Attached patch patch v2.3 (obsolete) — Splinter Review
moved styles, restored _dirty
Attachment #458342 - Attachment is obsolete: true
Attachment #458400 - Flags: review?(dao)
Attachment #458342 - Flags: review?(dao)
Comment on attachment 458400 [details] [diff] [review]
patch v2.3

>+    let currentUIVersion = 0;
>     try {
>-      migration = Services.prefs.getIntPref("browser.migration.version");
>+      currentUIVersion = Services.prefs.getIntPref("browser.migration.version");
>+      if (currentUIVersion >= UI_VERSION)
>+        return;
>     } catch(ex) {}

nit: in order to keep the try/catch block tight I would move the version comparison outside of it
Attachment #458400 - Flags: review?(dao) → review+
Attached patch patch v2.4Splinter Review
unbitrot and address comment
Attachment #458400 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/404a045ad4d5
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b2
Blocks: 580235
Depends on: 580957
It took me a while to figure out how to remove the button, so I filed bug 581238.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: