381519 - "Visit Date" should be "Most Recent Visit" in the Library

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Jesse Ruderman]

The new bookmark organizer has a "Visit Date" column.  I assume it's showing the last visit date, and the column label should make that clear.

Comment 1

[Joey Minta]

Attachment: change wording

I'm still not clear on the policy on changing entities, but 
http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#2140 confirms that Jesse's assertion about it showing the last visit seems to be correct.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 265594 [details] [diff] [review]
change wording

Hrm, i guess this was renamed to Visit Date for visits views (RESULT_TYPE_VISIT).

So it's fine to make this Last Visit Date again, you do need to rev the entity name though.

Comment 3

[Marcia Knous [:marcia]]

While testing Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8) Gecko/2007091211 GranParadiso/3.0a8, I noticed this column only shows a time and not a date at all. In the 2.0 branch last visited shows both. Nominating to get on the radar for polish.

Comment 4

[Marcia Knous [:marcia]]

I will move the stamping date and time stamping behavior to a spin off bug. I think for polish the column header should be renamed last visited.

(In reply to comment #3)
> While testing Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8)
> Gecko/2007091211 GranParadiso/3.0a8, I noticed this column only shows a time
> and not a date at all. In the 2.0 branch last visited shows both. Nominating to
> get on the radar for polish.
> 

Comment 5

[Marcia Knous [:marcia]]

I found this bug by Brett a while back which might explain things - Bug 333141

Comment 8

[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

Comment 10

[Nikhil Johny]

i am new, how should i proceed to fix this bug?

Comment 11

[Marco Bonardo [:mak]]

(In reply to Nikhil Johny from comment #10)
> i am new, how should i proceed to fix this bug?

Hi, supposing you have already looked at this: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build, what should be done here is taking the patch in comment 1, and addressing comment 2.
Basically, when changing a string you also have to change the entity name, so that localizers can notice that a string changed.
So col.lastvisit.label could be changed to col.lastvisitdate.label for example.
And all of the code points using col.lastvisit.label should be updated to the new entity name (http://mxr.mozilla.org/mozilla-central/search?string=col.lastvisit.label)

I also suggest the following documentation for the first patch:
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 12

[Nikhil Johny]

Attachment: changes made to browser/components/places/content/places.xul

Comment 13

[Nikhil Johny]

Attachment: changes made to browser/locales/en-US/chrome/browser/places/places.dtd

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 713386 [details] [diff] [review]
changes made to  browser/locales/en-US/chrome/browser/places/places.dtd

Review of attachment 713386 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your contribution, please check how to add your name and a commit message to the patch at

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
(short: you need -U option to add yourself as author for all of the patches, I suggest using Mercurial queues to manage patches and add commit messages easily)

r=me with the following comments addressed:

::: browser/locales/en-US/chrome/browser/places/places.dtd
@@ +80,5 @@
>  
>  <!ENTITY col.name.label          "Name">
>  <!ENTITY col.tags.label          "Tags">
>  <!ENTITY col.url.label           "Location">
> +<!ENTITY col.lastvisitdate.label     "Last Visit Date">

could your please still keep the previous values indentation here, by leaving only a single space in the middle?

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 713384 [details] [diff] [review]
changes made to  browser/components/places/content/places.xul

Review of attachment 713384 [details] [diff] [review]:
-----------------------------------------------------------------

As I said above, a patch with author and commit message would be welcome.

commit messages are like "Bug 12346 - description of the change. r=mak"

The 2 patches here can be combined into one, too.

Comment 16

[Nikhil Johny]

Attachment: "visit date" to "last visit date" in bookmark organiser

Comment 17

[Marco Bonardo [:mak]]

I'm thinking we may make this even better changing Last Visit Date to Most Recent Visit, since Last Visit Date could still be confused with "last visit date in this bucket" while it's instead "last visit date globally".
I'm sorry for the trouble, I just figured that would be better, would you mind changing the patch to use Most Recent Visit?

PS: your latest patch has now a proper author line, but you still miss the commit comment (hg qref -m "comment" or hg qref -e, using mercurial queues).

Comment 18

[Nikhil Johny]

Attachment: "visit date" to "most recent visit" in the library

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 715137 [details] [diff] [review]
"visit date" to "most recent visit" in the library

Review of attachment 715137 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patience!

Now the patch has a commit message!
Just a couple not important thing, we usually use a dash after the bug number and don't space around flags, so 'Bug 381519 - "Visit Date" should be "Most Recent Visit" in the Library. r=mak'

And the following change:

::: browser/locales/en-US/chrome/browser/places/places.dtd
@@ +85,5 @@
> +<!ENTITY col.visitcount.label      "Visit Count">
> +<!ENTITY col.keyword.label         "Keyword">
> +<!ENTITY col.description.label     "Description">
> +<!ENTITY col.dateadded.label       "Added">
> +<!ENTITY col.lastmodified.label    "Last Modified">

please don't reindent the other rows, otherwise you overwrite their blame information (in the previous case was fine to look for alignment cause it was not required to reindent all of the lines, just the new one). The general rule of thumb for patches is "change the minor number of rows possible".

Comment 20

[Marco Bonardo [:mak]]

I just found a problem here, we should also update the Sort By Visit Date entry here
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.properties#32

unfortunately this means that we have to change the anonid from here...
 <treecol label="&col.mostrecentvisit.label;" id="placesContentDate" anonid="date" flex="1" hidden="true"

And this open a can of worms, since that anonid is used in a bunch of places for the most various purposes, like figuring out the column type...
We should probably break this double use of the anonid, changing what fillWithColumns does
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#1020

We could have a "Sort by %S" generic string and replace %S with the actual column label... On the other side this means accesskeys will be far from their strings...

Mano, any idea what's worth doing here? If we don't break this now in future will always be hard to replace column labels.

Comment 23

[Filipus Klutiero]

Ticket #497202 was marked as a duplicate of this one. While it is definitely related, I do not read #497202 as requesting the same thing as this one. This one requests an actual bug fix, but #497202 requests to change the data displayed, which is a bit of a RFE, but one bordering a bug report as the current behavior makes little sense at best (for one thing, the visit date is not the last visit date in the requested period, but the real last visit date period).

Comment 24

[Marco Bonardo [:mak]]

(In reply to Filipus Klutiero from comment #23)
> This one requests an actual bug fix, but #497202 requests to change the data
> displayed, which is a bit of a RFE

We are not going to change the data displayed, thus the reason I duped it here, this is what we will do.

Comment 25

[Marco Bonardo [:mak]]

Ok, sorry for the brief stop, we discussed on IRC and reached consensus on how to proceed.

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.properties#28
Here you should change all of view.sortBy. to view.sortBy.1.
I'm now going to try to explain you the reason: when we do a localization change, we must always change the string id so localizers. In this case we should change view.sortBy.date.label to something else, but this string is build dinamically using "date" that is the anonid attribute defined here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.xul#387
Unfortunately we cannot change this anonid, otherwise other code should be changed and it's likely we may introduce regressions and break add-ons compatibility.
So we basically add versioning to all of the sortBy strings, by adding the .1 as I said above. When in future we should need to change again one of these strings we will bump it to .2 and so on.

The drawback of this approach is that changing only one of these strings will enforce us to change all of them, but we can't do differently in this case. This also means localizers will be notified of all of the changes instead of just a single string so, to help them, you should add just above the view.sortBy. strings, a localization note like this one:
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.properties#60
It should be something like

# LOCALIZATION NOTE (view.sortBy.1.name.label): sortBy properties are versioned.
# When any of these changes, all of the properties must be bumped, and the
# change must be annotated here.  Both label and accesskey must be updated.
# - version 1: changed view.sortBy.1.date.

Once that is done, you should also fix the code that builds the string id dinamically that is here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#1061
Just changing "view.sortBy." to "view.sortBy.1." should be enough.

At that point you can verify the change in the Library window (View menu)
If you have any doubts feel free to ask.

Comment 26

[Marco Bonardo [:mak]]

looks like I didn't re-read all of the above, errata corrige "when we do a localization change, we must always change the string id, so localizers can be notified about the change and fix the string accordingly."

Comment 27

[Nikhil Johny]

Attachment: "visit date" to "most recent visit" in the library

Comment 28

[Nikhil Johny]

@Marco Bonardo : Thank you for explaining the code that i needed to add/change, it helped me to understand the code better.

Comment 29

[Filipus Klutiero]

(In reply to Marco Bonardo [:mak] (Away Feb 22) from comment #24)
> (In reply to Filipus Klutiero from comment #23)
> > This one requests an actual bug fix, but #497202 requests to change the data
> > displayed, which is a bit of a RFE
> 
> We are not going to change the data displayed, thus the reason I duped it
> here, this is what we will do.

Right, you and Nikhil are implementing a simple fix to the way the data is presented. What Igor and I requested is to change what data is shown.

I suppose there are 2 non-identical questions someone visiting the history may be trying to answer:
1. Which pages did I visit (during some period)?
2. Which navigation path did I follow (during some period). What is the sequence of pages I visited?

The history currently neither directly answers 1 nor 2. It answers something between both, which effectively answers 1, but not 2.

Question 2 is more important in an organizational context, for time tracking, but question 1 is also valid. The ideal solution may be to add a preference letting the user choose what answer he is looking for.


The change you are making is not incompatible with what we're asking for, but it's not sufficient to address our concern, which is why marking #497202 is not technically correct.

Comment 30

[Marco Bonardo [:mak]]

(In reply to Filipus Klutiero from comment #29)
> Right, you and Nikhil are implementing a simple fix to the way the data is
> presented. What Igor and I requested is to change what data is shown.

And my answer is that we are not going to do that for performance reasons. Having a view with complete information that would take minutes to load is not particularly more useful then a view with partial information that takes milliseconds.

> I suppose there are 2 non-identical questions someone visiting the history
> may be trying to answer:
> 1. Which pages did I visit (during some period)?

This is already supported, when you open yesterday you see pages visited yesterday... this bug is exactly to adress the miscomprehension that it's not the case.

> 2. Which navigation path did I follow (during some period). What is the
> sequence of pages I visited?

This is not something you can extract from out history views cause they are grouped by uri, so regardless you'd have a "in which order i visited those uris" and not a navigation path. And even that, what's the use-case to figure your navigation path of yesterday? I think add-ons could use that for statistical scopes but I don't see a good reason for average users.

> The history currently neither directly answers 1 nor 2. It answers something
> between both, which effectively answers 1, but not 2.

Right, we won't address 2, we can't.

Comment 31

[Nikhil Johny]

@Marco Bonardo : Hi, is there anything else i need to add to the patch, please let me know.

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 716639 [details] [diff] [review]
"visit date" to "most recent visit" in the library

sorry for late, please always set a flag (either review, feedback or needinfo based on the request) when you need to target a request, we get a lot of mails from bugzilla and that helps prioritizing it.

Comment 33

[Marco Bonardo [:mak]]

Comment on attachment 716639 [details] [diff] [review]
"visit date" to "most recent visit" in the library

Review of attachment 716639 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/places/places.dtd
@@ +80,5 @@
>  
>  <!ENTITY col.name.label          "Name">
>  <!ENTITY col.tags.label          "Tags">
>  <!ENTITY col.url.label           "Location">
> +<!ENTITY col.mostrecentvisit.label "Most Recent Visit">

hm, there's something strange in the patch, looks like you may have splitted the removal of col.lastvisit.label and the places.xul change to another patch in the queue?

Could you please remerge the changes to a single patch?

Comment 34

[Filipus Klutiero]

(In reply to Marco Bonardo [:mak] from comment #30)
> (In reply to Filipus Klutiero from comment #29)
> > Right, you and Nikhil are implementing a simple fix to the way the data is
> > presented. What Igor and I requested is to change what data is shown.
> 
> And my answer is that we are not going to do that for performance reasons.

I'm perfectly fine with the change you're doing, whatever motivations you have. My point is that the change you're planning to do to solve this ticket does not address our request, therefore it was wrong to merge #497202 with this ticket.

[...]
> 
> > 2. Which navigation path did I follow (during some period). What is the
> > sequence of pages I visited?
> 
> This is not something you can extract from out history views cause they are
> grouped by uri, so regardless you'd have a "in which order i visited those
> uris" and not a navigation path.

We can have a navigation path if the user browsed in a single tab. Otherwise, you can have each visit, ordered, with its time.

> And even that, what's the use-case to
> figure your navigation path of yesterday? I think add-ons could use that for
> statistical scopes but I don't see a good reason for average users.

As I wrote, time tracking is important for organizations. I'd say that's the main use case I see. Which doesn't mean the average user needs. I for one never claimed that addressing #497202 would make a difference for most users. I'm not even sure most users use the history.

[...]

Comment 35

[Marco Bonardo [:mak]]

(In reply to Filipus Klutiero from comment #34)
> My point is that the change you're planning to do to solve this ticket
> does not address our request, therefore it was wrong to merge #497202 with
> this ticket.

I duped it here cause this is what we will do to address the issue, probably not in the requested way, but that's common in bug fixes: the solution may differ from the required change.
The final result doesn't change, if that bug wouldn't be duped to this solution, it would be a wontfix.

Comment 36

[Filipus Klutiero]

(In reply to Marco Bonardo [:mak] from comment #35)
> (In reply to Filipus Klutiero from comment #34)
> > My point is that the change you're planning to do to solve this ticket
> > does not address our request, therefore it was wrong to merge #497202 with
> > this ticket.
> 
> I duped it here cause this is what we will do to address the issue, probably
> not in the requested way, but that's common in bug fixes: the solution may
> differ from the required change.

Uh, if a solution to a problem differs from the change required, then... it's not a solution to that problem. Again, your change is appreciated and surely addresses this issue, I am simply saying it does not address ours.

> The final result doesn't change, if that bug wouldn't be duped to this
> solution, it would be a wontfix.

The WONTFIX status means a bug will *never* be fixed. The mere fact that a certain change "won't fix" a certain bug doesn't justify the WONTFIX status for that bug (else, all tickets would have the status). Typically, WONTFIX is applied when satisfying a request would cause security issues or other important regressions.

Comment 37

[Marco Bonardo [:mak]]

(In reply to Filipus Klutiero from comment #36)
> The WONTFIX status means a bug will *never* be fixed. The mere fact that a
> certain change "won't fix" a certain bug doesn't just

We indeed use wontfix to mark things that are not planned to be fixed.  Though some bugs are filed again in future and if the design direction changed the bug may be considered valid. It's not forever, but the current design direction is that it's not planned at all, so it's not worth to track it for now.

Comment 38

[Filipus Klutiero]

(In reply to Marco Bonardo [:mak] from comment #37)
> (In reply to Filipus Klutiero from comment #36)
> > The WONTFIX status means a bug will *never* be fixed. The mere fact that a
> > certain change "won't fix" a certain bug doesn't just
> 
> We indeed use wontfix to mark things that are not planned to be fixed. 
> Though some bugs are filed again in future and if the design direction
> changed the bug may be considered valid. It's not forever, but the current
> design direction is that it's not planned at all, so it's not worth to track
> it for now.

Not planning to fix a bug is not a reason to tag WONTFIX. Basically, unless you know what you're doing, tagging a bug WONTFIX is almost always wrong. When a bug really should be tagged WONTFIX, there will be a specific reason not to address the bug given. If you're not sure, you shouldn't.

Comment 39

[Josh Matthews [:jdm]]

Nikhil, can you confirm that you're still working on this?

Comment 40

[Nikhil Johny]

Attachment: new patch

sorry for the delay, this is the new patch which solves the problem mentioned in comment 33, i hope this solves the bug, please let me know if there is something else to change.

Comment 41

[Marco Bonardo [:mak]]

Comment on attachment 742700 [details] [diff] [review]
new patch

Review of attachment 742700 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/places/places.dtd
@@ +80,5 @@
>  
>  <!ENTITY col.name.label          "Name">
>  <!ENTITY col.tags.label          "Tags">
>  <!ENTITY col.url.label           "Location">
> +<!ENTITY col.mostrecentvisit.label "Most Recent Visit">

I'm sorry, but the patch is still showing the addition of col.mostrecentvisit.label but not the removal of col.lastvisit.label in places.dtd

You might have a modified tree or a patch in mq, please ensure you are working on a clean tree

if you need help we may adjust the patch for you.

Comment 42

[Nikhil Johny]

Attachment: patch

Comment 43

[Marco Bonardo [:mak]]

Comment on attachment 751047 [details] [diff] [review]
patch

Review of attachment 751047 [details] [diff] [review]:
-----------------------------------------------------------------

This looks correct!

Sorry if it took a while and thank you for your contribution.

Comment 44

[Tiziana Sellitto [:tiziana]]

The review is positive. Nikhil you should add a "checkin-needed" keyword or ask your mentor to get the code into the tree. 
Look at this https://developer.mozilla.org/en-US/docs/Introduction

Comment 45

[Nikhil Johny]

@Marco Bonardo: could you please add the code into the tree, i don't know how to add the "checkin-needed" keyword. Thank you.

Comment 46

[Marco Bonardo [:mak]]

sure, I set the needinfo request to myself to check the code applies cleanly and  land it. Thanks.

Comment 47

[Marco Bonardo [:mak]]

FWIW, the answer to your question is:
to have someone push the code to the tree for you, add checkin-needed to the keywords field, above in the bug details.

Comment 48

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/6170a3f8f722

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6170a3f8f722