215489 - User-created toolbars don't remember on/off settings

Status: RESOLVED DUPLICATE of bug 220701

(Firefox :: Toolbars and Customization, defect, P3)

Description

[pantsgolem]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030804 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030804 Mozilla Firebird/0.6.1

User-created toolbars in FB are always visible on startup, even if you
previously turn them off in View -> Toolbars.  Standard toolbars remember their
on/off settings properly.

Reproducible: Always

Steps to Reproduce:
1. Create a custom toolbar with buttons and name of your choice.
2. In View -> Toolbars, deselect the toolbar you just created so that it doesn't
display.
3. Close and restart Firebird.

Actual Results:  
The custom toolbar is visible.

Expected Results:  
The custom toolbar should not be visible until you activate it again via View ->
Toolbars.

I've seen this bug on Windows XP (don't remember the build), my current
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030804 Mozilla
Firebird/0.6.1, as well as the 0.6.1 release for Linux (the latter being a clean
install with a clean profile), so OS would be All.  Chances are hardware is All
as well, but since I haven't technically tested it on a Mac, I'll leave that alone.

A workaround is to simply get rid of the toolbar by removing all items from it,
but this is impractical for someone who has need of a specific group of toolbar
buttons some of the time, but doesn't want to make a custom toolbar each time. 
At any rate, it's still pretty minor.

Comment 1

[Simon Paquet [:sipaq]]

Confirming with 20030805 build on W2K.
-> NEW

Comment 2

[djk]

I recommend making this bug a dup of bug 220701 since my patch over there fixes
the problem in this bug.  Or maybe a dependancy on bug 220701 instead...

David

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch to fix.

Adds a new "persistent" attribute that syncs from localstore.rdf. Brendan
requested this rather than just changing the behaviour of "persist" so as not
to break XUL1.0 apps that rely on setting the persist attribute to save changes
to localstore.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 136699 [details] [diff] [review]
patch to fix. 

Alright, what do you folks think of this approach?

Comment 5

[Brendan Eich [:brendan]]

I'm obviously for this approach, modulo better name that connotes the direction
(persistent vs. persist is ok, but adjective vs. verb doesn't convey the "load"
vs. "store").  But I defer to hyatt on adding to XUL.  Ben, you mailing hyatt or
collaring him on IRC?

/be

Comment 6

[djk]

This patch doesn't deal with "mode" or "iconsize", correct?

Persisting those settings is not _really_ needed at the moment, as we can't
(yet?) customize those modes "per toolbar" in the UI.

However, it would be nice to add persistance support for those attributes in the
backend so that it would be possible to add a UI in the future, or even allow
people to modify the localstore now to achieve mixed-mode user-created toolbars.

David

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

djk, that's really orthogonal to this bug. 

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Patch aside, this is getting kicked to 0.9. Hyatt isn't around until the new
year to give the needed moa. 

Comment 9

[Pete Boyd]

I haven't seen this mentioned so far in this bug so I just want to add that I
don't see how one of these toolbars can be removed without using 'restore
default set' - which wipes out existing customisations to other toolbars
(including losing Personal Toolbar Folder from the Personal Toolbar, yikes!);
and that whilst the browser is running, if you create a new window the toolbar
you've asked to be hidden returns in that new window (not the existing one)

Comment 10

[Brendan Eich [:brendan]]

How about a new patch, and requesting review from bryner instead of hyatt.

/be

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 136699 [details] [diff] [review]
patch to fix. 

Got hyatt on IRC, and ben is now trying to recollect the problem here.	We need
to regroup, figure out what's really wrong, and fix that.

/be

Comment 12

[Blake Ross]

-> me

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 136699 [details] [diff] [review]
patch to fix. 

+    rv = aElement->GetAttr(kNameSpaceID_None, nsXULAtoms::persistent,
persist);
+    if (NS_FAILED(rv)) return;
+
+    // Sync attributes with localstore datasource. This is a new, post Mozilla
1.0
+    // behavior. 
+    if (rv == NS_CONTENT_ATTR_HAS_VALUE)
+      ApplyPersistentAttributesToElement(aElement);
+

Maybe use HasAttr() here in stead of GetAttr() since we don't use the value
here at all.

+nsresult
+nsXULDocument::ApplyPersistentAttributesToElement(nsIContent* aElement)
+{
+    nsresult rv;
+
+    nsCOMPtr<nsIRDFResource> element;
+    rv = nsXULContentUtils::GetElementResource(aElement,
getter_AddRefs(element));
+    if (NS_FAILED(rv)) return rv;
+
+    nsCOMPtr<nsISupportsArray> elements;
+    rv = NS_NewISupportsArray(getter_AddRefs(elements));
+    if (NS_FAILED(rv)) return rv;
+
+    // Only this one element to restore persisted attributes for.
+    elements->AppendElement(aElement);
+    
+    return ApplyPersistentAttributesToElements(element, elements);

Seems like this is backwards, and sub-optimal perf wise. We create an array
(heap malloc, refcount etc) with a single item, only to pass to a method that
iterates over the items and does stuff. Maybe flip this around and move the
meat of this into the method that takes one element, and make the method that
takes an array iterate over the array and call the method that takes one
element.

Or if you're convinced it doesn't matter, then I can deal with this as written
here too :-)

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

grumble. Not a "blocker"

Comment 15

[:Gavin Sharp [email: [e-mail]]]

*** Bug 258493 has been marked as a duplicate of this bug. ***

Comment 16

[Tracy Walker [:tracy]]

*** Bug 294163 has been marked as a duplicate of this bug. ***

Comment 17

[Phil Ringnalda (:philor)]

*** Bug 308936 has been marked as a duplicate of this bug. ***

Comment 18

[Dave Townsend [:mossop]]

*** Bug 309087 has been marked as a duplicate of this bug. ***

Comment 19

[David Bruhn]

(In reply to comment #9)
> I haven't seen this mentioned so far in this bug so I just want to add that I
> don't see how one of these toolbars can be removed without using 'restore
> default set' - which wipes out existing customisations to other toolbars
> (including losing Personal Toolbar Folder from the Personal Toolbar, yikes!);
> and that whilst the browser is running, if you create a new window the toolbar
> you've asked to be hidden returns in that new window (not the existing one)

Actually, if you clear all of the buttons from the toolbar then it is deleted upon  leaving the "Customize Toolbar" screen.

Comment 20

[Martin Lemburg]

I found this behaviour the first time in firefox 1.5RC2 and it remains in firefox 1.5

Comment 21

[Phil Ringnalda (:philor)]

*** Bug 329186 has been marked as a duplicate of this bug. ***

Comment 22

[Marcia Knous [:marcia]]

Still present in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2.

Does this need a new patch?

Comment 23

[Phil Ringnalda (:philor)]

*** Bug 283087 has been marked as a duplicate of this bug. ***

Comment 24

[Corey]

I,m using firefox 2.0.0.4 and thunderbird 2.0.0 this is still an issue. at least for me I'm running win98se 

Comment 26

[Tony Chung [:tchung]]

Adding [FFT3.5] in whiteboard.  Came across this issue during RC1 testing

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, kinda embarassing that it's still around. Marking polish and I've asked mconnor to see if that patch applies ;)