369813 - persist container open state (in both history sidebar, bookmarks sidebar, and other places based trees)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

persist container open state (in both history sidebar, bookmarks sidebar, and other places based trees)

as an end user, you'll really see this with the bookmarks sidebar.  each time you open it, the top level items are collapsed.

I believe the fix for this will be to nsNavHistoryResultTreeViewer::IsContainerOpen() and to ToggleOpenState(), similar to what we are doing in nsXULTreeBuilder.cpp, but instead of using the localdatastore, we'd use something else.

myk and mano both suggested that we could either use annotations or the place db

keep in mind, we'd need to this for all containers, history and bookmarks, and history containers (by host, and by date) don't have place_ids, but we could use the place query.

see http://wiki.mozilla.org/Places/StatusMeetings/2007-02-08 for more discussion on this.

Comment 1

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

actually, I think we should use local store (rdf) for this, as this is view data, not model data.

I think if I implement nsNavHistoryResultTreeViewer::IsContainerOpen() and to ToggleOpenState() and follow the code in nsXULTreeBuilder.cpp's IsContainerOpen() and to ToggleOpenState().

as for what to use as the unique uri for the resource, the place query for the object should be unique, as I think we can use the "view", and the place uri (encoded onto the query string?) 

something like, and I'm hand waving here,

"chrome://browser/content/bookmarks/bookmarksPanel.xul?folder=1&group=3;excludeQueries=1"

I like the idea of this view data living in localstore.rdf, with all the other view data in Firefox 3.

Comment 2

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

I chatted with mano over irc, and he is ok with using localstore for this, as long as it doesn't introduce browser dependencies in toolkit.

he also pointed out that in fx 2 the state changes to the bm sidebar affect the organize bookmark dialog and the tree in the add bookmark dialog.  this seems like a bug to me that we could address in fx 3, but I could not fix a fx 2 bug on it.  (mconnor / gavin, does that ring any bells?)

Comment 3

[Phil Ringnalda (:philor)]

I'd be very surprised if there is one, what with it seeming a lot more like a feature. What would the bug report say, "When I use the bookmark manager to rearrange a folder of bookmarks I use a lot, then that's open the next time I use the sidebar, which is way too convenient."? I could probably make up a use-case for not keeping them in sync, if I had to, but trying to explain why keeping them in sync is the right thing seems so obvious I just degenerate into waving my arms and pointing at the screen. I've never known anyone to say that they have separate sidebar-folders, organized-folders, and added-to-folders.

Comment 4

[O. Atsushi (Torisugari)]

(In reply to comment #2)
What will happen to a RDF entry in localstore.rdf, when a user
wants to delete the bookmark folder with its status described
in that RDF node? I assume it would remain forever, until his/her
bookmark service re-use the same folder ID. Then, the user
will find the new folder is "Open" automatically, not knowing
why.

(Note that we don't see this bug in Fx2, because the the bookmark
service never uses the same folder ID again.)

So, some delete-observer has to remove such RDF resources. As
far as I know, no other implementation tries to remove something
from localstore.rdf successfully, because, I guess, it's painful
to keep up-to-date. For example, when Fx crashes, there should be
some mismatch between localstore and places sqlite. It costs
relatively high, if possible, to maintain this feature to be
product-level quality via localstore.rdf, I think.

Comment 5

[Neil Deakin]

Attachment: persist open state for tree rows

Also cleans up the row count adjusting, which needs to remove and insert separately.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 261802 [details] [diff] [review]
persist open state for tree rows 

I guess there are missing utils.js changes here?

Anyway, this probably was not tested with query (as in, non-folder) containers (e.g. "Today").

Comment 7

[Neil Deakin]

Is there a unique id that can be used for the 'day' type items? And how would I create a query container in the UI?

Comment 8

[Asaf Romano (gone)]

(In reply to comment #7)
> Is there a unique id that can be used for the 'day' type items?

node.uri should work for both folders and queries.

> And how would I create a query container in the UI?

Group By Site/Data & Site in the History Sidebar

Comment 9

[Neil Deakin]

(In reply to comment #8)
> 
> > And how would I create a query container in the UI?
> 
> Group By Site/Data & Site in the History Sidebar
> 

No, Group By Date gives 'day' type items and Group By Site gives 'host' type items, neither of which have a uri.

Comment 10

[Asaf Romano (gone)]

Er, yeah; s/queries/containers but folders/ then. Day/Host items not having place: uris is a bug.

Comment 11

[Neil Deakin]

mano, is there a bug filed for that?

Comment 12

[Asaf Romano (gone)]

No, and after looking at these containers implementation further, I think we need a different solution, how about taking the root node URI + rowId if the container does not have its own URI?

Comment 13

[Neil Deakin]

Do you mean the database row id, or the tree view row index? There isn't a means to get the former and the latter will change when rows are added or removed or a some other container is opened or closed.

Comment 14

[Asaf Romano (gone)]

Hrm, yeah, there's no rowid for those in the DB.

So, please file file a follow up for day/host containers, let's keep this bug for bookmark folders only :-/

Comment 15

[Neil Deakin]

Attachment: update patch

Comment 16

[Neil Deakin]

Filed bug 380735 about day/host containers

Comment 17

[Asaf Romano (gone)]

Comment on attachment 264872 [details] [diff] [review]
update patch

* the folder id getter is gone
* you could use node.uri for both folders and queries
* what's the doChildren variable about?

Comment 18

[Neil Deakin]

(In reply to comment #17)
> (From update of attachment 264872 [details] [diff] [review])
> * the folder id getter is gone

Will change to itemId

> * you could use node.uri for both folders and queries

No, because that uri includes extra query arguments that are present in some cases and not others.
  
> * what's the doChildren variable about?

Something leftover that should be removed

Comment 19

[Asaf Romano (gone)]

I don't think that's the case, e.g. if we happen to excludeItems persisting the container state separately is desired IMO.

Comment 20

[Neil Deakin]

Attachment: just get the uri

Comment 21

[Asaf Romano (gone)]

Comment on attachment 265019 [details] [diff] [review]
just get the uri

s/getResourceForNode :/_getResourceForNode:/

and please also get rid of braces around single-line blocks.

r=mano otherwise.

Comment 22

[Dietrich Ayala (:dietrich)]

neil, can you land this asap? if you are not able to land tonight, please let me know.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200708020808 Minefield/3.0a7pre. 

The state persists in History and Bookmarks (Sidebar and Organize) as expected. 

Comment 24

[Aakash Desai [:aakashd]]

Test cases on 3.x test runs were updated for regression testing this bug.

For 3.0,
https://litmus.mozilla.org/show_test.cgi?id=7494

For 3.1,
https://litmus.mozilla.org/show_test.cgi?id=7495

Comment 25

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv