Closed
Bug 369813
Opened 18 years ago
Closed 18 years ago
persist container open state (in both history sidebar, bookmarks sidebar, and other places based trees)
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
People
(Reporter: moco, Assigned: enndeakin)
References
()
Details
(Whiteboard: [Fx2-parity])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•18 years ago
|
Whiteboard: [Fx2-parity]
Updated•18 years ago
|
Whiteboard: [Fx2-parity] → [Fx2-parity] affects schema?
Reporter | ||
Comment 1•18 years ago
|
||
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.
Reporter | ||
Comment 2•18 years ago
|
||
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?)
Assignee: nobody → sspitzer
Whiteboard: [Fx2-parity] affects schema? → [Fx2-parity]
Comment 3•18 years ago
|
||
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•18 years ago
|
||
(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.
Assignee | ||
Comment 5•18 years ago
|
||
Also cleans up the row count adjusting, which needs to remove and insert separately.
Assignee: sspitzer → enndeakin
Attachment #261802 -
Flags: review?(mano)
Comment 6•18 years ago
|
||
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").
Attachment #261802 -
Flags: review?(mano) → review-
Assignee | ||
Comment 7•18 years ago
|
||
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•18 years ago
|
||
(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
Assignee | ||
Comment 9•18 years ago
|
||
(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•18 years ago
|
||
Er, yeah; s/queries/containers but folders/ then. Day/Host items not having place: uris is a bug.
Assignee | ||
Comment 11•18 years ago
|
||
mano, is there a bug filed for that?
Comment 12•18 years ago
|
||
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?
Assignee | ||
Comment 13•18 years ago
|
||
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•18 years ago
|
||
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 :-/
Assignee | ||
Comment 15•18 years ago
|
||
Attachment #261802 -
Attachment is obsolete: true
Attachment #264872 -
Flags: review?(mano)
Comment 17•18 years ago
|
||
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?
Attachment #264872 -
Flags: review?(mano)
Assignee | ||
Comment 18•18 years ago
|
||
(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•18 years ago
|
||
I don't think that's the case, e.g. if we happen to excludeItems persisting the container state separately is desired IMO.
Assignee | ||
Comment 20•18 years ago
|
||
Attachment #264872 -
Attachment is obsolete: true
Attachment #265019 -
Flags: review?(mano)
Comment 21•18 years ago
|
||
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.
Attachment #265019 -
Flags: review?(mano) → review+
Comment 22•18 years ago
|
||
neil, can you land this asap? if you are not able to land tonight, please let me know.
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 23•17 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Comment 24•16 years ago
|
||
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
Flags: in-litmus? → in-litmus+
Comment 25•15 years ago
|
||
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•