380735 - Persist open state of the "age in days" and "site" containers in the history sidebar

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Neil Deakin]

A unique id is needed for these.

Comment 2

[Dietrich Ayala (:dietrich)]

finding owners for all the A6 blockers. can you please put a swag in the
whiteboard? then we can review and load-balance at the next places meeting.

note: see bug 369813 for an example of this. also:

http://lxr.mozilla.org/mozilla/source/browser/components/places/content/treeView.js#837

Comment 3

[Dietrich Ayala (:dietrich)]

er, see previous comment :)

Comment 4

[Dietrich Ayala (:dietrich)]

%#!%^?!!

Comment 5

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

summarizing an idea I had with Steve:

keep treeView.js as is, but in nsNavHistory.cpp, fix it so we specify a uri for the containers

For example:

curTopGroup = new nsNavHistoryContainerResultNode(EmptyCString(), title,
          EmptyCString(), nsNavHistoryResultNode::RESULT_TYPE_HOST, PR_TRUE,
          EmptyCString());

Instead of passing in EmptyCString() for the uri, generate a uri.  

For example:

urn:places-hack-schema:escaped(current date name):escaped(title)

That should be unique:

urn:places-hack-schema::bugzilla.mozilla.org
urn:places-hack-schema:Today:bugzilla.mozilla.org
urn:places-hack-schema:Today:

I wish we could use a place: uri, but I don't think we can get one for the generate nsNavHistoryContainerResultNodes we create.

Comment 6

[Asaf Romano (gone)]

Given that titles are not unique, it is not be ideal. It is sure better than serializing group-containers to place: uris though.

Seems OK as long as you make sure to escape the titles... I would also make it so the urn starts with the query uri.

Comment 7

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

> Given that titles are not unique, it is not be ideal

Are you pointing out the the "Today" folder for when in "Date" mode would map to the same "Today" folder when in "Date and Site" mode?

I'm assuming that firefox 2 treated those containers as unique when persisting state.

If so, we could also include the type (nsNavHistoryResultNode::RESULT_TYPE_DAY or nsNavHistoryResultNode::RESULT_TYPE_HOST) in the urn to make those unique.

For actually host titles, TitleForDomain() will create unique titles.

> I would also make it so the urn starts with the query uri.

good point.

How about something like:

urn:places-persist:query-uri,type,age-in-days-value,escaped(title)

where type and age-in-days-value are integers.

Comment 8

[Asaf Romano (gone)]

Well, I read your previous comment as a generic solution for all "group containers" (e.g. group a bookmark folder by site. Please ignore the fact that persisting container state would be the very last bug there). If we're going to just fix day/host containers here, then sure, any syntax will do.

Comment 9

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

patch in hand.

Comment 10

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

Attachment: patch, v1

for View By Date, this generates URNs like:

urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&sort=1&type=1,0,
urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&sort=1&type=1,1,
urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&sort=1&type=1,2,

for View By Date and Site, this generates urns like:

urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&group=1&sort=1&type=1,-1,%28local%20files%29
urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&group=1&sort=1&type=1,-1,www.mozilla.com
urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&group=1&sort=1&type=1,-1,www.google.com

and also:

urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&group=1&sort=1&type=1,0
urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&group=1&sort=1&type=1,1

Notice how these two "Today" containers (one from View By Date and one from View by Date and Site) are not the same:

urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&sort=1&type=1,0,
urn:places-persist:place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&group=0&group=1&sort=1&type=1,0

Comment 11

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

I am noticing one problem.  when I switch the view with the view picker, the urns are missing part of the uri:

urn:places-persist:place:group=0&group=1&sort=1&type=1,-1,mail.mozilla.com

I think there might be a bug in applyFilter() in tree.xml, investigating.

Comment 12

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

> I think there might be a bug in applyFilter() in tree.xml, investigating.

it's partly an applyFilter() issue [we shouldn't be calling it], and partly that our time query options may not be needed!  

both are covered by bug #384671.

Comment 13

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

with the fix for bug #384677 and bug #384671, we now have:

for View By Date, this generates URNs like:

urn:places-persist:place:group=0&sort=1&type=1,0,
urn:places-persist:place:group=0&sort=1&type=1,1,
urn:places-persist:place:group=0&sort=1&type=1,2,

for View By Date and Site, this generates urns like:

urn:places-persist:place:group=0&group=1&sort=1&type=1,-1,%28local%20files%29
urn:places-persist:place:group=0&group=1&sort=1&type=1,-1,www.mozilla.com
urn:places-persist:place:group=0&group=1&sort=1&type=1,-1,www.google.com

and also:

urn:places-persist:place:group=0&group=1&sort=1&type=1,0
urn:places-persist:place:group=0&group=1&sort=1&type=1,1

Notice how these two "Today" containers (one from View By Date and one from
View by Date and Site) are not the same:

urn:places-persist:place:group=0&sort=1&type=1,0,
urn:places-persist:place:group=0&group=1&sort=1&type=1,0

Comment 14

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

Attachment: same as the last patch, but include the change to nsNavHistoryResult.cpp that I failed to include

Comment 15

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

Attachment: again, missed a change to nsNavHistoryResult.cpp on my patch

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 268634 [details] [diff] [review]
again, missed a change to nsNavHistoryResult.cpp on my patch

> 
>+nsresult
>+CreatePlacesPersistURN(nsNavHistoryQueryResultNode *aResultNode, PRInt64 aAgeInDays, const nsCString& aTitle, nsCString& aURN)
>+{
>+  nsCAutoString uri;
>+  nsresult rv = aResultNode->GetUri(uri);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  aURN.Assign(NS_LITERAL_CSTRING("urn:places-persist:"));
>+  aURN.Append(uri);
>+
>+  aURN.Append(NS_LITERAL_CSTRING(","));
>+  aURN.AppendInt(aAgeInDays);
>+
>+  nsCAutoString escapedTitle;
>+  PRBool success = NS_Escape(aTitle, escapedTitle, url_XAlphas);
>+  NS_ENSURE_TRUE(success, NS_ERROR_OUT_OF_MEMORY);
>+
>+  aURN.Append(NS_LITERAL_CSTRING(","));
>+  aURN.Append(escapedTitle);
>+
>+  return NS_OK;
>+}

nits:

- we know that title will sometimes be empty, so should you check length of title, not bother escaping if empty? maybe not bother adding at all?

- if we decide to not add empty titles, maybe should do the same treatment for aAgeInDays w/ value -1.

looks fine otherwise, r=me.

Comment 17

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

Attachment: updated patch, remove comment in treeView.js and address dietrich's review comments

Comment 18

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

morphing summary.

fixed.

cvs ci browser/components/places/content/treeView.js toolkit/components/places/s
rc/nsNavHistory.cpp toolkit/components/places/src/nsNavHistory.h toolkit/compone
nts/places/src/nsNavHistoryResult.cpp
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.
js
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.134; previous revision: 1.133
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHisto
ry.h
new revision: 1.82; previous revision: 1.81
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  ns
NavHistoryResult.cpp
new revision: 1.104; previous revision: 1.103
done

Comment 19

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

Comment on attachment 269028 [details] [diff] [review]
updated patch, remove comment in treeView.js and address dietrich's review comments

as checked in (carried over dietrich's r=)

Comment 20

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

thinkings a bit, we could do a history like query in our test js, grouped appropriately (day, and day and site) and then verify that the uri of the nodes was what we expected it to be, so a test could be written for this.

adding in-testsuite?

Comment 21

[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