Closed Bug 215489 Opened 17 years ago Closed 7 years ago

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

Categories

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

1.0 Branch
defect

Tracking

()

RESOLVED DUPLICATE of bug 220701

People

(Reporter: pantsgolem, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: polish, Whiteboard: [FFT3.5][polish-easy])

Attachments

(1 file)

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.
Confirming with 20030805 build on W2K.
-> NEW
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Target Milestone: --- → Firebird0.8
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
Depends on: 220701
Attached patch patch to fix. Splinter Review
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 on attachment 136699 [details] [diff] [review]
patch to fix. 

Alright, what do you folks think of this approach?
Attachment #136699 - Flags: superreview?(brendan)
Attachment #136699 - Flags: review?(hyatt)
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
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
djk, that's really orthogonal to this bug. 
Patch aside, this is getting kicked to 0.9. Hyatt isn't around until the new
year to give the needed moa. 
Flags: blocking0.8-
Priority: -- → P3
Target Milestone: Firebird0.8 → Firebird0.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)
Target Milestone: Firefox0.9 → Firefox1.0beta
How about a new patch, and requesting review from bryner instead of hyatt.

/be
Flags: blocking1.0+
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
Attachment #136699 - Flags: superreview?(brendan)
Attachment #136699 - Flags: review?(hyatt)
Severity: minor → normal
Assignee: hyatt → sspitzer
Status: ASSIGNED → NEW
-> me
Assignee: sspitzer → firefox
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 :-)
Whiteboard: http://testrunner.mozilla.org/test.cgi?run_id=243#863
grumble. Not a "blocker"
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
Flags: blocking-aviary1.1?
Keywords: helpwanted
*** Bug 258493 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1-
*** Bug 294163 has been marked as a duplicate of this bug. ***
Assignee: firefox → nobody
QA Contact: bugzilla → toolbars
Target Milestone: Firefox1.0beta → ---
*** Bug 308936 has been marked as a duplicate of this bug. ***
*** Bug 309087 has been marked as a duplicate of this bug. ***
(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.
I found this behaviour the first time in firefox 1.5RC2 and it remains in firefox 1.5
*** Bug 329186 has been marked as a duplicate of this bug. ***
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?
*** Bug 283087 has been marked as a duplicate of this bug. ***
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 
Duplicate of this bug: 396507
Adding [FFT3.5] in whiteboard.  Came across this issue during RC1 testing
Whiteboard: http://testrunner.mozilla.org/test.cgi?run_id=243#863 → http://testrunner.mozilla.org/test.cgi?run_id=243#863, [FFT3.5]
Flags: in-litmus?
Flags: blocking-firefox3.6?
Hardware: x86 → All
Whiteboard: http://testrunner.mozilla.org/test.cgi?run_id=243#863, [FFT3.5] → [FFT3.5]
Version: unspecified → 1.0 Branch
Not blocking, kinda embarassing that it's still around. Marking polish and I've asked mconnor to see if that patch applies ;)
Flags: blocking-firefox3.6? → blocking-firefox3.6-
Keywords: helpwantedpolish
Whiteboard: [FFT3.5] → [FFT3.5][polish-easy]
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 220701
No longer depends on: 220701
You need to log in before you can comment on or make changes to this bug.