Closed
Bug 1257599
Opened 8 years ago
Closed 8 years ago
Rename »Unsorted Bookmarks« into »Other Bookmarks«
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | verified |
People
(Reporter: phlsa, Assigned: ktbee, Mentored)
References
Details
(Whiteboard: [qx:spec][Onboarding][outreachy-12])
Attachments
(1 file, 5 obsolete files)
19.56 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
»Unsorted Bookmarks« is a problematic name because - It implies that the user needs to do work (to sort them) - It uses negative tone Chrome uses »Other Bookmarks« as a name for a similar folder, which doesn't have these problems. Since that's clearly better and consistency with other products doesn't hurt in that area, we should be using that name again. I think this is the file that needs changing in order to accomplish that (but there might be others): https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd
Comment 1•8 years ago
|
||
Katie, would you like to work on this bug next?
Mentor: jaws
Flags: needinfo?(kbroida)
Whiteboard: [qx:spec][Onboarding] → [qx:spec][Onboarding][outreachy-12]
Assignee | ||
Comment 2•8 years ago
|
||
Sure thing!
Assignee | ||
Comment 3•8 years ago
|
||
I've updated the label to be "Other Bookmarks" in the browser. Should the variables referring to this label be changed to "other" versus "unsorted" as well? For example in https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd on line 172 we have: `<!ENTITY unsortedBookmarksCmd.label "Unsorted Bookmarks">` Which I've changed to: `<!ENTITY unsortedBookmarksCmd.label "Unsorted Bookmarks">` Should I also change references like "unsortedBookmarksCmd.label" to "otherBookmarksCmd.label" as well?
Flags: needinfo?(kbroida) → needinfo?(jaws)
Attachment #8735275 -
Flags: review?(jaws)
Comment 4•8 years ago
|
||
Wow that was fast :) Yeah, when we change the meaning of a string we need to change the name of the string. This is done so the people who maintain other languages will know about the string change and have the opportunity to update their version. When you change the name of the string in the DTD file, you will need to update the places in our code that reference the name. You should be able to search MXR for the name to find those places.
Assignee: nobody → kbroida
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Comment 5•8 years ago
|
||
Comment on attachment 8735275 [details] [diff] [review] bugfix.patch See previous comment for next steps.
Attachment #8735275 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 6•8 years ago
|
||
Thanks for the clarification Jared, I've gone ahead and updated the variable names as well with this new patch.
Attachment #8735275 -
Attachment is obsolete: true
Attachment #8735587 -
Flags: review?(jaws)
Comment 7•8 years ago
|
||
Comment on attachment 8735587 [details] [diff] [review] Updates "Unsorted" bookmark labels and variable names to "Other" Review of attachment 8735587 [details] [diff] [review]: ----------------------------------------------------------------- We shouldn't change all of the places in our codebase to use "OTHER" instead of "UNSORTED" as logicially they aren't really any different. I think we should only need to change the string, and the entity names of those strings. Tests that check the value of the strings and look for "Unsorted Bookmarks" will need to be updated to "Other Bookmarks" (like you've done in test_ext_bookmarks.html), but we shouldn't make changes that aren't strictly related to the renaming of the strings. Can you undo some of these changes and produce a simpler patch?
Attachment #8735587 -
Flags: review?(jaws) → review-
Comment 8•8 years ago
|
||
yes please, just change the strings, or you're going to break bookmarks forever :) The code doesn't care about how things are named in the visible ui.
Assignee | ||
Comment 9•8 years ago
|
||
Thank you for clarifying, I definitely don't want to permanently break bookmarks ;p I think the third time is the charm with this patch. I've only updated strings and entity names that refer to them.
Attachment #8735587 -
Attachment is obsolete: true
Attachment #8735923 -
Flags: review?(jaws)
Comment 10•8 years ago
|
||
Comment on attachment 8735923 [details] [diff] [review] Updates "Unsorted Bookmarks" label to "Other Bookmarks" on the front end Review of attachment 8735923 [details] [diff] [review]: ----------------------------------------------------------------- Close! Just a minor change that can be reverted and we should be ready to go. When you make the changes below I'll push the new patch to our tryserver which will run your changes against our full testsuite. ::: browser/components/places/content/places.xul @@ +268,5 @@ > accesskey="&view.sort.accesskey;"> > <menupopup onpopupshowing="ViewMenu.populateSortMenu(event);" > oncommand="ViewMenu.setSortColumn(event.target.column, null);"> > <menuitem id="viewUnsorted" type="radio" name="columns" > + label="&view.other.label;" accesskey="&view.other.accesskey;" This should be reverted along with the changes to places.dtd ::: browser/locales/en-US/chrome/browser/places/places.dtd @@ +20,5 @@ > <!ENTITY view.columns.accesskey "C"> > <!ENTITY view.sort.label "Sort"> > <!ENTITY view.sort.accesskey "S"> > +<!ENTITY view.other.label "Other"> > +<!ENTITY view.other.accesskey "U"> These two string should not be changed, as they actually don't relate to "Unsorted Bookmarks". To see these, open the Library by pressing Cmd+Shift+B (I'm assuming you're using OSX). In the top of that window, click on Views, then show the "Sort" submenu. At the top you'll see the "Unsorted" menuitem. As a tip for the future, if you change the value of a label, you should also change the value of the accesskey if the character doesn't exist in the new label. Accesskeys are used to help users jump to the item via their keyboard, and it helps to keep them consistent with each other.
Attachment #8735923 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 11•8 years ago
|
||
Changes reverted! I didn't realize "unsorted" referred to something other than bookmarks in those cases. Thank you for letting me know.
Attachment #8735923 -
Attachment is obsolete: true
Attachment #8736091 -
Flags: review?(jaws)
Comment 12•8 years ago
|
||
Comment on attachment 8736091 [details] [diff] [review] Updates "Unsorted Bookmarks" label to "Other Bookmarks" on the front end Review of attachment 8736091 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for sticking with this. The patch is very close, but two of the strings that got their values changed still need their entity names changed too, and references to those entities need to be updated as well. ::: mobile/android/base/locales/en-US/sync_strings.dtd @@ +32,5 @@ > <!ENTITY bookmarks.folder.menu.label 'Bookmarks Menu'> > <!ENTITY bookmarks.folder.places.label ''> > <!ENTITY bookmarks.folder.tags.label 'Tags'> > <!ENTITY bookmarks.folder.toolbar.label 'Bookmarks Toolbar'> > +<!ENTITY bookmarks.folder.unfiled.label 'Other Bookmarks'> The entity name here needs to be updated since we're changing the semantics of this string. This will let other localizers know to update their version. ::: toolkit/locales/en-US/chrome/places/places.properties @@ +3,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > BookmarksMenuFolderTitle=Bookmarks Menu > BookmarksToolbarFolderTitle=Bookmarks Toolbar > +UnsortedBookmarksFolderTitle=Other Bookmarks This entity name should be renamed too.
Attachment #8736091 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 13•8 years ago
|
||
Those changes make sense, thank you for catching them Jared. Let me know if there's anything else I can tweak. I've been reading more about XML entities and .properties files, so this is all making a lot more sense.
Attachment #8736091 -
Attachment is obsolete: true
Attachment #8736758 -
Flags: review?(jaws)
Comment 14•8 years ago
|
||
Comment on attachment 8736758 [details] [diff] [review] Updates "Unsorted Bookmarks" label to "Other Bookmarks" on the front end Review of attachment 8736758 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8736758 -
Flags: review?(jaws) → review+
Comment 15•8 years ago
|
||
I pushed your patch to tryserver for you, https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e69c944b927 I'll check back in a few hours to make sure that all tests passed, then we can set the "checkin-needed" keyword on the bug.
Comment 16•8 years ago
|
||
Attachment #8736758 -
Attachment is obsolete: true
Attachment #8736766 -
Flags: review+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 17•8 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/fx-team/rev/140c43bb6f49
Keywords: checkin-needed
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/140c43bb6f49
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•8 years ago
|
Points: --- → 2
Updated•8 years ago
|
QA Whiteboard: [good first verify]
Comment 19•8 years ago
|
||
I've see this issue on Nightly 48.0a1 (2016-03-17) ; (Build ID: 20160317030235) on Linux, 64 Bit This Bug is now verified as fixed on Latest Firefox Beta 48.0b6 Build ID: 20160706215822 User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [good first verify] → [good first verify][testday-20160708]
Comment 20•8 years ago
|
||
I have reproduced this bug in Nightly 48.0a1 (2016-03-17) in windows 8.1 32 bit Bug is fixed now on Latest Beta 48.0b6(Build ID:20160706215822) User Agent:Mozilla/5.0 (Windows NT 6.3; rv:48.0) Gecko/20100101 Firefox/48.0 [testday-20160708]
Comment 21•8 years ago
|
||
I was able to reproduce this bug in Nightly 48.0a1 on Mac Yosemite 10.10.2. Bug is showing fixed in Firefox 48.0b7 on Mac Yosemite 10.10.2. [bugday-20160713]
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•