95457 - Folders in new outliner folder view not persisting state

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Jason Kersey]

Reproduce:
.Open all subfolders of an account
.Close Mail
.Open Mail

Expected:  Folders that were open should be open now
Actual:    Folders are all closed again

This happens reguardless of what view you are using (Normal 3 pane or the Better
3 pane).  The open accounts do persist, only subfolders of them do not.  This is
bad because if someone has filters that direct mail to subfolders, because we
don't bold the parent folder if any of it's subfolder have new messages, the
closed folders won't reveal the fact they have mail in them without the user
hunting for it.

Comment 1

[(not reading, please use [e-mail] instead)]

I could have sworn this was working, but kerz showed me it's not.

I'll investigate

Comment 2

[Håkan Waara]

This is working, but the problem is that there's a slight delay before they are
automatically expanded at startup. If you click any of them before they expand,
they loose the persistance.

Comment 3

[David :Bienvenu]

it doesn't work at all for me. Also, I believe we should be showing the
servers+folders expanded at startup, not expanding them after startup, if possible.

Comment 4

[Håkan Waara]

In other words, we should do whatever our persistance says. :P

Comment 5

[David :Bienvenu]

I'm not making myself clear. 

Here's what happens today:

On startup, all the servers are shown collapsed initially, and then get
expanded. None of the sub-folders get expanded.

Here's what should happen, IMO

On startup, servers and folders should be shown initially expanded if they were
expanded when we shut down.
To try to be even more clear, I should not see the servers shown as collapsed at
all, if they were expanded when I shut down.

Comment 6

[(not reading, please use [e-mail] instead)]

ok, after some debugging from bienvenu (thanks) I know more.

first, it's never really worked properly.  there's two issues:

1)  there's some bad code doing the wrong thing with the elided (elided == 
closed) flag.  bienvenu's got some of those fixes started.

2)  the outliner builder is (nsXULOutlinerBuilder.cpp, SetOutliner(), line 
1788) sets the datasource we check for open / closed state of a resource to be 
the in memory datasource for localstore.rdf

in this case, we (mailnews) want it to use the folder datasource.

bienvenu's done the work to store the open / closed state in the per folder 
summary files, and caching it in the folder cache (panacea.dat), and the 
there's front js to set the elided state in the folder flags.  we just need the 
outliner builder to ask us for it.

a good reason for doing it this way is by not using localstore.rdf, we can 
prevent loading the mailnews dlls at start up.  (the other way is to implement 
rdf delegation, but that's a bigger, harder problem.)

the trick will be to figure out how we can fix SetOutliner() to conditionally 
use an alternate datasource for the open / closed state.

maybe an another attribute on the outliner:

datasources="rdf:foo" openStateDatasource="rdf:bar"

waterson, suggestions?

Comment 7

[(not reading, please use [e-mail] instead)]

by the way, the reason why I thought it worked is that I had an old 
localstore.rdf, which had some old state (folder uri X was open) which is why 
it appeared to work for me.

another thing that bienvenu found that we need to address is that we need to 
make sure we're committing the change when the elided flag changes.  he 
mentioned that we weren't always committing it.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Fixing database corruption caused by bug 95743.  Pay no attention to the man
behind the curtain.

Comment 9

[[e-mail] (gone - use [e-mail] instead)]

Sheela, Bradley, am I crazy, or do I specifically remember this working on the
branch, before we landed?

Comment 10

[Bradley Hart]

I'm fairly certain that this was working in branch I tested before the landing.

Comment 11

[(not reading, please use [e-mail] instead)]

looking at the code, I can tell you persistance across sessions (quit and
restart) isn't there.

things might have appeared to work if you had the right thing in your
localstore.rdf (which is what happened to me.)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Reversing database corruption caused by bug 95857 and bug 95798.  Pay no
attention to the man behind the curtain.

Comment 13

[(not reading, please use [e-mail] instead)]

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

Comment 14

[(not reading, please use [e-mail] instead)]

Attachment: start of the mozilla/content part of the patch, bunch of mailnews changes still required.

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: more of the fix, just giving waterson / bienvenu / varga a heads up to what I'm doing

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 17

[Chris Waterson]

The stuff in content seems pretty good; couple of comments.

1. Getting an arbitrary datasource is a possible security hole. See code in
   nsXULDocument that does some checks on the document's principal.

2. Don't NS_ENSURE that you got a datasource. A user could put anything in
   that attribute, and there's no reason to assert if they type in garbage
   that won't load.

3. If they _do_ type in garbage that won't load, or they're not allowed to
   access the datasource because of security, then we ought to fallback on
   creating an in-memory datasource so the tree will work.

4. Could you just use an NS_ASSERTION instead of hiding the flow-of-control
   change with NS_ENSURE? Those macros drive me crazy.

thanks!

Comment 18

[David :Bienvenu]

r=bienvenu for mailnews stuff, except, should
nsMsgFolderDataSource::createFolderOpenNode

 check if target is null?

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 20

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, talking to waterson on #mozilla

Comment 21

[Chris Waterson]

Attachment: suggested mods for nsXULOutlinerBuilder.cpp

Comment 22

[Chris Waterson]

Only a few nits on that last patch on nsXULOutlinerBuilder. We always want to
fall back on an in-memory datasource for the persist datasource, even when not
trusted. Also, noted that at some point we may want to consider comparing the
codebase for untrusted documents -- not an issue now (because we can't ``write
an arbitrary datasource back''), but maybe some day. Finally, since we can
potentially bail early out of this routine, and because the outliner code
doesn't bother to check return code, I fixed a place where we do an unchecked
dereference of mPersistStateStore. Look ok?

Comment 23

[(not reading, please use [e-mail] instead)]

Attachment: latest patch, (based on waterson's patch) that will use localstore if we are trusted

Comment 24

[Chris Waterson]

groovy. r= or sr=waterson on attachment 46570 [details] [diff] [review].

Comment 25

[(not reading, please use [e-mail] instead)]

that last patch has an extra bonus.

the open state for bookmarks (which uses outliner) now persists across sessions.

cc ben, blake and bryner, who are outliner users.

now that I've added statedatasource, is there some doc about outliner that need 
updating?

Comment 26

[(not reading, please use [e-mail] instead)]

fix checked in.

Comment 27

[[e-mail] (gone - use [e-mail] instead)]

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

Comment 28

[laurel]

Newer instance of this problem reported in bug 100325, will track that one open
since it has a + rating for branch.  Don't know that we want to reopen this to
point to that one, so will leave as is until 100325 is evaluated.

Comment 29

[laurel]

Update:  Using sept19 commercial 0.9.4 branch builds this is working fine for me
with win98, linux rh6.2 (still haven't gotten to mac). I'll won't mark this
verified, though, until more info/comments are gathered in the new bug 100325.

Comment 30

[laurel]

I'm going to mark this one verified and let the other new bug stand -- it has
been spotted only on winME and may go away after repeated collapse folders.