95111 - Persisted attributes should only be applied from localstore if specified in the xul

Status: RESOLVED WONTFIX

(Core :: XUL, defect, P2)

Description

[jag (Peter Annema)]

Once an attribute is persisted to localstore.rdf, it will be applied regardless
of whether persistance is still specified for that attribute. It should check if
persist is still specified for the attribute before applying (and remove it from
localstore if it's not).

Comment 1

[jag (Peter Annema)]

Note to self: this will affect documents that programmatically persist without
having an accompanying persist attribute.

Comment 2

[jag (Peter Annema)]

One thing to note for this is that if we change the xul to specify persist where
currently the attributes are only persisted from js, there could be a weird case
where someone depends on that behaviour (e.g. "apply" vs. "cancel"). All our
current uses don't have that problem, though one could argue that the persist on
close is redundant when you're also doing persist "when needed". Would it be
worth adding a |restore| function to nsIDOMXULDocument?

hewitt: could you see what would need to change in DOM Inspector once this bug
is fixed?

/extensions/inspector/resources/content/jsutil/xul/inFormManager.js, line 75 --
aDoc.persist(aEl.getAttribute("id"), "value");

Not sure what you're doing here. The line above it does something like:
aEl.setAttribute("value", aEl.value);. What are you trying to do there?

/extensions/inspector/resources/content/utils.js, line 71 --
document.persist(aId, attrs[i]);

This is in a persistAll function that persists all attributes for a given
element. Apart from the question why you're calling that instead of letting xul
persist do its job (I'm sure there's a good explanation), you'd probably need to
modify this to skip the id and persist attributes, and add a persist attribute
enumerating all the other attributes.

----- ----- ----- -----

jag:

/extensions/wallet/resources/content/walletNavigatorOverlay.xul, line 61 --
document.persist(id, 'hidden');

Add persist="hidden" to:
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/resources/content/walletNavigatorOverlay.xul#136

/extensions/wallet/resources/content/walletNavigatorOverlay.xul, line 62 --
document.persist(elementID, 'checked');

Add persist="checked" to:
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/resources/content/walletNavigatorOverlay.xul#97

/xpfe/communicator/resources/content/utilityOverlay.js, line 174 --
document.persist(id, 'hidden');
/xpfe/communicator/resources/content/utilityOverlay.js, line 175 --
document.persist(elementID, 'checked');

Ugh. Similar to goToggleFormToolbar:
http://lxr.mozilla.org/seamonkey/search?string=goToggleToolbar



/xpfe/components/console/resources/content/console.js, line 52 --
document.persist("ConsoleBox", "mode");
/xpfe/components/console/resources/content/console.js, line 65 --
document.persist("ConsoleBox", "sortOrder");

Add persist="mode sortOrder" to:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/console/resources/content/console.xul#151

/xpfe/components/console/resources/content/console.js, line 101 --
document.persist(toolbar.id, "hidden");
/xpfe/components/console/resources/content/console.js, line 102 --
document.persist(bc.id, "checked");

Same toolbar code.



/xpfe/components/sidebar/resources/sidebarOverlay.js, line 1000 --
setTimeout("document.persist('sidebar-panels-splitter-box','height');",100);

Add persist="height" to:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/sidebar/resources/sidebarOverlay.xul#61



/xpfe/global/resources/content/bindings/toolbar.xml, line 37 --
document.persist(toolbar.id, "moz-collapsed");
/xpfe/global/resources/content/bindings/toolbar.xml, line 57 --
document.persist(toolbar.id, "moz-collapsed");

This one is slightly harder. We'd either end up adding persist="moz-collapsed"
to every toolbar, or add some js way of reading the value from localstore. We
can't add it to the xbl binding in a declerative way, because that is (or should
be) called after we already applied localstore.

Adding an "ignore-persisted" attribute might look good for a while (backward
compatible, less intrusive), but then you'll end up with a large-ish list of
attributes you want to ignore after a few version changes.

Input?

Comment 3

[saari (gone)]

->waterson as per jag

Comment 4

[Håkan Waara]

This blocks us from hiding the total/unread columns by default for all users
with existing accounts. (bug 95468)

We added two new columns to the folderpane: Unread and Total. We want both to be
hidden by default, so I hacked up a patch, basically adding: |hidden="true"
persist="hidden"| to the outliner.

I realized that it was now never persisted. hyatt told me about this bug.

The result for all users with existing accounts is an unusable folderpane which
looks something like this:

| Name  | Unr... | Total |
 St...   123...   2890
 Ha...   433      924
 Ac...   0        0

(for screenshots: attachment 45947 [details] , attachment 45948 [details] [diff] [review])

So we now get a bunch of bugreports and feedback in the newsgroups from
desperated users that thinks their folders are gone, their accounts "broken"...
let alone their messages.

According to the feedback, many users don't even understand how to hide or
resize these columns so they end up importing (thus recovering, from their point
of view) their emails in Outlook and switching email app or throwing away their
existing accounts with unread messages.

Sorry for the long comment, but this has extremely negative impact on mailnews
and I wanted you to know. I think you should consider this for 0.9.4.  Thanks.

Comment 5

[Chris Waterson]

This sounds like it oughtta be nsenterprise...

Comment 6

[Chris Waterson]

Attachment: only apply persistent values to those attributes listed in the ``persist'' attribute

Comment 7

[Chris Waterson]

Invert logic so that we read the persist attribute's value, and use that as the
driver for applying persistent values. Do we really need this for mozilla-0.9.4?
At one point I thought jag said all the problems had been hacked around, but...

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 47868 [details] [diff] [review]
only apply persistent values to those attributes listed in the ``persist'' attribute

sr=jst

Comment 9

[jag (Peter Annema)]

As far as I know, the issues I've mentioned in my 2001-08-14 00:27 comment are
still open issues, checking this in would break those cases. I believe
security/* has hacked/worked around their issues.

If this is needed for 0.9.4, I can try and get these places fixed tonight. Can
you get a "yes, we want this" or "no, can wait"?

Comment 10

[jag (Peter Annema)]

Comment on attachment 47868 [details] [diff] [review]
only apply persistent values to those attributes listed in the ``persist'' attribute

(Removed the - lines for clarity)

>+        nsAutoString persist;
>+        element->GetAttr(kNameSpaceID_None, nsXULAtoms::persist, persist);
> 
>+        nsAString::const_iterator iter, end;
>+        persist.BeginReading(iter);
>+        persist.EndReading(end);

In this case, if you use |nsAutoString::const_iterator| (or |nsAFlatString::const_iterator|, 
it's the same thing), your iterators will for now be the normal |nsStringIterator|s, but 
will become |const PRUnichar*| (since your string is known to be flat and doesn't need 
multi fragment handling logic).

Generally you can safely use this pattern:

nsSomeString foopy; // put something in foopy
nsSomeString::const_iterator iter, end;
foopy.BeginReading(iter);
foopy.EndReading(end);

This will end up using the most appropriate iterator for that type of string.

>+        while (iter != end) {
>+            while (iter != end && (nsCRT::IsAsciiSpace(*iter) || *iter == PRUnichar(',')))
>+                ++iter;
> 
>+            nsAString::const_iterator mark = iter;
> 
>+            while (iter != end && !nsCRT::IsAsciiSpace(*iter) && *iter != PRUnichar(','))
>+                ++iter;
>+
>+            ApplyPersistentAttributeTo(aResource, element, Substring(mark, iter));
>         }

This doesn't look right. Aren't you going to skip the first token in the persist attribute 
this way?

You should just be able to remove the first while scan loop and get what you want.

Comment 11

[Chris Waterson]

Ok, I'm moving this to mozilla-0.9.5. I assume that if anyone really needs it,
they'll scream.

jag: I'll change the |nsAString::const_iterator to
|nsAutoString::const_iterator|; thanks for pointing that out. Also, I think the
loop is correct (at least, it ought not fail in the way you suggested).
Specifically, the first nested |while| will skip over any whitespace, but if
there is none, will leave |iter| positioned at the start of the string.

Comment 12

[Håkan Waara]

Waterson: Let me remind you of my scream from 2001-08-25 18:16. Does that count? ;)

Mailnews is still horked for new users post-folderpane rewrite and I don't think
we should ship 0.9.4 like that.

Comment 13

[Chris Waterson]

Attachment: Same as above with some comments, and using nsAutoString::const_iterator

Comment 14

[Chris Waterson]

Okay, back to mozilla-0.9.4. jag, can you work up a patch for the instances you
describe on 2001-08-14 00:27?

Comment 15

[jag (Peter Annema)]

Waterson: d'oh, you're right. That parse loop is fine.

Okay, I'll go convert. Can you give me that |void readPersisted(in DOMString id,
in DOMString attribute);| I mentioned in e-mail? Should I implement it?

Comment 16

[jag (Peter Annema)]

Attachment: [patch] Fix most places that depend on this bug

Comment 17

[Chris Waterson]

XUL changes look fine by me. hyatt and/or ben, could you sr= this whole mess?

Comment 18

[David Hyatt]

First of all, hwaara has glommed onto the wrong bug.  This bug has nothing to do
with the folder pane issue (which is actually about attribute removals not
causing the localstore to unassert).

I disagree with this change and believe this warrants more consideration before
landing.  This change basically says that any time you use
document.persist(), you are now screwed.  Unless there's a corresponding read
function on XUL document, you won't be able to bring those attributes back.

IMO the way to fix this bug is to keep the behavior of the local store the same
and to rev IDs if/when you decide to alter their persistence properties.
I am planning on converting persistence code to overlays (hopefully before 1.0),
and I am not going to get into a partial overlay application hell.  You either
persisted or you didn't, and if you want to change this, rev the ID of the
element in question or change your entire file name (version your file names).

My $0.02.

Comment 19

[Chris Waterson]

Sounds good to me!

Comment 20

[Mike Potter]

A bug that I filed, but 115296 (Persist should work for items created by JS) is
related to this bug.  Mabye people who were / are working on this bug would like
to know about it.

Comment 21

[V@no]

Why this bug was marked as resolved wontfix?
The suggested work around of this bug is ridiculous!
Not only it requires multiple changes in the code when persistence no longer needed (versus simply remove persist attribute), but it also pollutes the xulstore.json storage with no way of cleaning it!