Closed
Bug 893836
Opened 11 years ago
Closed 9 years ago
Change Android menu contents to sentence case
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect, P5)
Tracking
(firefox49 fixed, fennec+)
RESOLVED
FIXED
Firefox 49
People
(Reporter: ibarlow, Assigned: bdave, Mentored)
References
Details
(Whiteboard: [lang=properties][lang=dtd][good first bug])
User Story
We should change the strings of 4 menu items: Report Site Issue > Report site issue Exist Guest Session > Exist guest session Find in Page > Find in page Request Desktop Site > Request desktop site
Attachments
(1 file, 5 obsolete files)
2.57 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
Our general rule of thumb is to use sentence case text for all of our menus and lists. We don't seem to be doing that in our menu right now, so we should fix that.
Comment 1•11 years ago
|
||
Ian, do you have a specific list of strings that need changing?
I'll make this into a mentor bug, and if we don't have a specific list of strings already, the first task here will be to audit all our strings :)
Here are some files to look at:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties
Whiteboard: [mentor=margaret][lang=properties][lang=dtd]
Comment 2•11 years ago
|
||
Margaret: How do I sign up to be mentored for this bug? I'm an Android developer and this one seems like a pretty simple way to get started! :)
Comment 3•11 years ago
|
||
Welcome Michael. It's all yours. Feel free to hop into #mobile on irc.mozilla.org if you have any general questions.
Assignee: nobody → michaelcevans10
Comment 4•11 years ago
|
||
Welcome! You'll first want to follow directions here to get a build environment set up:
https://wiki.mozilla.org/Mobile/Fennec/Android
Once you're happy with the changes you've made, you should upload a patch to this bug, and set the review? flag to me. Here are some docs on generating 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
And definitely feel free to ask questions on IRC as Aaron mentioned.
Comment 5•11 years ago
|
||
Attachment #8351606 -
Flags: review?(margaret.leibovic)
Comment 6•11 years ago
|
||
Comment on attachment 8351606 [details] [diff] [review]
This patch makes menu and list text more consistent by switching to sentence case.
Review of attachment 8351606 [details] [diff] [review]:
-----------------------------------------------------------------
Unfortunately, you'll need to update the entity names so that our localization automation will alert localizers that these strings have changed, since I imagine that western-style languages that have similar capitalization rules to English will want to follow our lead here.
Since you're not changing the meanings of these strings, you can just append a "2" or something to the entity names (e.g. downloadCancelPromptTitle -> downloadCancelPromptTitle2), and you'll need to update where these entities are used. For the string in android_strings.dtd, this is pretty easy because we can just update where the entity is converted into an android resource in strings.xml.in, without touching the code where the resources are used. However, you'll need to search in the code for where the browser.properties strings are used.
We should also make a post to the l10n mailing list when we land this, just to make it clear what this change was all about.
Attachment #8351606 -
Flags: review?(margaret.leibovic) → feedback+
Comment 7•11 years ago
|
||
Flod - Do we need to make the entity name change for changes to capitalization?
Flags: needinfo?(francesco.lodolo)
Comment 8•11 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Flod - Do we need to make the entity name change for changes to
> capitalization?
If we don't change the entity names, I think we would at least want to let localizers know about this change in convention, so that they can update the strings if it suits their locales.
Comment 9•11 years ago
|
||
No need for new entity names, each locale should have its own set of rules for capitalization (basically this is like fixing a typo relevant only for en-US).
But I agree that a message in dev-l10n wouldn't hurt :-)
Flags: needinfo?(francesco.lodolo)
Comment 10•11 years ago
|
||
Comment on attachment 8351606 [details] [diff] [review]
This patch makes menu and list text more consistent by switching to sentence case.
Review of attachment 8351606 [details] [diff] [review]:
-----------------------------------------------------------------
Okay, then ignore my last comment! But let's make a post to dev-l10n.
Attachment #8351606 -
Flags: feedback+ → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Keywords: checkin-needed
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
I'm sorry, I should have caught that we'd probably need to update some robocop tests to account for these string changes.
Michael, you can follow these directions to run robocop tests locally:
https://wiki.mozilla.org/Auto-tools/Projects/Robocop
After that, you can look in the logs linked above to see which tests failed, and try fixing them. Once you have a patch that's working for you locally, I can push it to try server to make sure it works on our test infrastructure.
It should be relatively straightforward (albeit somewhat annoying) to update the tests. You'll just need to look for where those strings are used in the tests and update them appropriately. For example, it looks like this is what caused the first failure in the log:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAddSearchEngine.java#22
A lot of the strings are probably in this file:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/StringHelper.java
Comment 14•11 years ago
|
||
Margaret, no problem! I'll take a look at fixing up those tests and upload a patch. Is it preferred to put them in the same patch? Or to do two separate patches, one for the strings and one for the updated tests?
Comment 15•11 years ago
|
||
(In reply to michaelcevans10 from comment #14)
> Margaret, no problem! I'll take a look at fixing up those tests and upload a
> patch. Is it preferred to put them in the same patch? Or to do two separate
> patches, one for the strings and one for the updated tests?
Thanks! We usually prefer a separate patch for the test changes, just so the patches are smaller to review.
Reporter | ||
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
tracking-fennec: ? → +
Comment 16•11 years ago
|
||
Hi Michael, did you ever mange to look into fixing this? I'm sorry I didn't check in on this bug sooner! Let me know if you still want to work on this, and if you don't we can reset the assignee.
Flags: needinfo?(michaelcevans10)
Comment 17•11 years ago
|
||
I was actually working on it this weekend! I was having a little trouble with getting Robocop working, but I think I'll be able to get the patch updated with the fixed tests. I do still want to fix it though!
Does Robocop spit out some sort of test report at the end with the number of passed/failed tests? I let it run for a good while and it never gave me any sort of report about tests that failed.
Flags: needinfo?(michaelcevans10)
Updated•10 years ago
|
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=properties][lang=dtd] → [lang=properties][lang=dtd]
Comment 19•10 years ago
|
||
Hey Michael, are you still interested in working on this bug? We'll probably need to update the patch at this point. I would love to see this get finished!
Flags: needinfo?(michaelcevans10)
Comment 20•10 years ago
|
||
vikneshwar, this could be a good next bug for you. Are you interested?
The patch here would need to be updated, and we'll need to update the robocop tests, but luckily you already have experience editing strings in robocop tests from bug 1085595 :)
Assignee: michaelcevans10 → nobody
Flags: needinfo?(lviknesh)
Comment 21•10 years ago
|
||
./mach package gave some unrelated error http://pastebin.com/gZDMKRwz
Assignee: nobody → lviknesh
Attachment #8351606 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(lviknesh)
Attachment #8510655 -
Flags: feedback?(margaret.leibovic)
Comment 22•10 years ago
|
||
Comment on attachment 8510655 [details] [diff] [review]
SentenceCase.patch
Review of attachment 8510655 [details] [diff] [review]:
-----------------------------------------------------------------
This is looking good! I didn't go through all of the exact string changes very closely, but we can always catch other strings that need updating in a follow-up.
Are you still having build problems? You should make a try push with this patch to make sure you caught all the tests that need updating.
Attachment #8510655 -
Flags: feedback?(margaret.leibovic) → feedback+
Comment 23•10 years ago
|
||
vikneshwar, are you still working on this? It would be great to finish this up and land it!
Flags: needinfo?(michaelcevans10) → needinfo?(lviknesh)
Comment 24•10 years ago
|
||
Resetting assignee due to a lack of activity.
The patch here probably needs to be updated at this point, but it would be good to finish this up!
Assignee: lviknesh → nobody
Whiteboard: [lang=properties][lang=dtd] → [lang=properties][lang=dtd][good next bug]
Updated•10 years ago
|
Flags: needinfo?(lviknesh)
Comment 25•10 years ago
|
||
I can work on this. Looks like it's on the home stretch, if I'm not mistaken -- should I just run the existing patch through the try server? Or should I first get Robocop set up and see if there are any other tests failing due to inconsistent string case?
Comment 26•10 years ago
|
||
D'oh, looks like I forgot to needinfo? this.
Flags: needinfo?(margaret.leibovic)
Comment 27•10 years ago
|
||
Sorry for the spam, but things are going to be pretty busy around here for the next couple of weeks, so I'll defer to anyone else who wants to work on this bug.
Flags: needinfo?(margaret.leibovic)
Comment 28•9 years ago
|
||
Hi! Is this bug still alive and open to be taken up? I'm an Android Developer and would like to start with something simple.
If I can work on it, can you please share its current status and other links to get started?
Flags: needinfo?(margaret.leibovic)
Comment 29•9 years ago
|
||
(In reply to Pramod G from comment #28)
> Hi! Is this bug still alive and open to be taken up? I'm an Android
> Developer and would like to start with something simple.
> If I can work on it, can you please share its current status and other links
> to get started?
Sure! The state of this bug is that the patch here needs to be updated. At this point, it's probably easier to just start over, since it's been a year since the patch here was written.
Do you have a Firefox for Android build set up? If not, you should follow the instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android
Flags: needinfo?(margaret.leibovic)
Comment 30•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #29)
> Sure! The state of this bug is that the patch here needs to be updated. At
> this point, it's probably easier to just start over, since it's been a year
> since the patch here was written.
>
> Do you have a Firefox for Android build set up? If not, you should follow
> the instructions here:
> https://wiki.mozilla.org/Mobile/Fennec/Android
Awesome! Setting up the build.
Comment 31•9 years ago
|
||
Patch 1/2 that has been modified as per requirements. I was unable to run any test on these, I guess I don't have the necessary authorization yet.
Attachment #8677534 -
Flags: review?(margaret.leibovic)
Comment 32•9 years ago
|
||
Patch 2/2 that has been modified as per requirements. I was unable to run any test on these, I guess I don't have the necessary authorization yet.
Attachment #8677535 -
Flags: review?(margaret.leibovic)
Comment 33•9 years ago
|
||
Comment on attachment 8677534 [details] [diff] [review]
Modified strings to follow sentence case formatting
Thanks for getting involved!
It looks here like you uploaded a new version of the file, as opposed to a patch.
I'm not sure what directions you were following, but we have a new tool called MozReview that makes uploading patches for review much easier. I would encourage you to read this page to generate a patch for review:
http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html
You'll have to follow some steps to configure the tool, but all you need is bugzilla credentials, and you already have those :)
Let me know if you have any questions! You can also join #mobile on irc.mozilla.org for help getting set up.
Attachment #8677534 -
Flags: review?(margaret.leibovic)
Updated•9 years ago
|
Attachment #8677535 -
Flags: review?(margaret.leibovic)
Comment 34•9 years ago
|
||
Oh, sorry about that. Will upload the patch in the proper way soon.
Comment 35•9 years ago
|
||
Anthony, is this still an issue? Is there a subset of menu/settings items we can update to make a big impact?
I'm worried that this bug has never been fixed because its scope is too large.
Flags: needinfo?(alam)
Comment 36•9 years ago
|
||
Yes, this is still an issue. Perhaps it best to just scope this back to just our overflow menu for now. That is, anything inside that menu as soon as the user presses "...".
As for a list, I think we only need these two changed in the menu.
Find in Page > Find in page
Request Desktop Site > Request desktop site
Flags: needinfo?(alam) → needinfo?(margaret.leibovic)
Comment 37•9 years ago
|
||
(In reply to Anthony Lam (:antlam) from comment #36)
> Yes, this is still an issue. Perhaps it best to just scope this back to just
> our overflow menu for now. That is, anything inside that menu as soon as the
> user presses "...".
>
> As for a list, I think we only need these two changed in the menu.
>
> Find in Page > Find in page
> Request Desktop Site > Request desktop site
Thanks! Let's do this.
Flags: needinfo?(margaret.leibovic)
Whiteboard: [lang=properties][lang=dtd][good next bug] → [lang=properties][lang=dtd][good first bug]
Updated•9 years ago
|
User Story: (updated)
Updated•9 years ago
|
Attachment #8510655 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8677534 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8677535 -
Attachment is obsolete: true
Comment 38•9 years ago
|
||
Oh! sorry left out guest session.
Exist Guest Session > Exist guest session.
Find in Page > Find in page
Request Desktop Site > Request desktop site
Updated•9 years ago
|
User Story: (updated)
Comment 39•9 years ago
|
||
I also noticed another:
Report Site Issue > Report site issue
Exist Guest Session > Exist guest session
Find in Page > Find in page
Request Desktop Site > Request desktop site
Updated•9 years ago
|
User Story: (updated)
Updated•9 years ago
|
Blocks: fennec-polish
Comment 40•9 years ago
|
||
I think the New Tab and New Private Tab options should be in here too. So let's add that to the list:
New Tab > New tab
New Private Tab > New private tab
Report Site Issue > Report site issue
Exist Guest Session > Exist guest session
Find in Page > Find in page
Request Desktop Site > Request desktop site
That should be it.
Comment 41•9 years ago
|
||
Hi Margaret,
Is this bug still open? If yes, I would like to work on it as my first bug.
Let me know.
Thanks,
Anu
Comment 42•9 years ago
|
||
(In reply to Anindita Das from comment #41)
> Is this bug still open? If yes, I would like to work on it as my first bug.
Sure, this bug is still available! Here's a wiki page with some details to help you get started:
https://wiki.mozilla.org/Mobile/Fennec/Android/Suggested_workflow
Let me know if you have any questions, or feel free to jump in #mobile on irc.mozilla.org to ask questions.
Assignee | ||
Comment 43•9 years ago
|
||
Hi Margaret,
I've made changes to android_strings.dtd as per the list shared by Anthony.
However, the following two options seemed to be missing from this file.
--Report Site Issue > Report site issue
--Exist Guest Session > Exist guest session
Tested the changes by releasing the apk on my Nexus 5 and it was reflecting.
Search Tag = Bug 893836 - Change Android menu contents to sentence case
Comment 44•9 years ago
|
||
Report Site Issue is found here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/webcompatReporter.properties
bhavindave88, your patch needs to be in Mercurial patch format before it can be reviewed and merged in. See https://wiki.mozilla.org/Mobile/Fennec/Android/Suggested_workflow (the Mercurial section) for tips on how to get started.
Assignee | ||
Comment 45•9 years ago
|
||
Hi Mike,
Hope this patch file conforms to the standard.
Comment 46•9 years ago
|
||
Cool! Maragaret, since you're the mentor on this bug who would be a good person for bdave to ask for review?
Flags: needinfo?(margaret.leibovic)
Comment 47•9 years ago
|
||
Comment on attachment 8755486 [details] [diff] [review]
893836.patch
Review of attachment 8755486 [details] [diff] [review]:
-----------------------------------------------------------------
Since you're just changing the capitalization, you don't need to rev the entity names, so this looks great. Thanks for the patch!
Attachment #8755486 -
Flags: review+
Updated•9 years ago
|
Attachment #8754010 -
Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
Updated•9 years ago
|
Assignee: nobody → bhavindave88
Updated•9 years ago
|
Keywords: checkin-needed
Comment 48•9 years ago
|
||
Keywords: checkin-needed
Comment 49•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 50•9 years ago
|
||
This patch appears to have added a new "autom4te.cache" top level directory to the source tree, which I'm assuming was unintentional. Please be more careful when submitting future patches.
Flags: needinfo?(bhavindave88)
Comment 51•9 years ago
|
||
Assignee | ||
Comment 52•9 years ago
|
||
Hi Ryan,
Can you share some preventive steps to ensure this or should I simply delete lines with "autom4te.cache" in future patches?
Comment 53•9 years ago
|
||
bugherder |
Comment 54•9 years ago
|
||
New Tab -> New tab
New Private Tab -> New private tab
Report Site Issue -> Report site issue
Find in Page -> Find in page
Request Desktop Site -> Request desktop site
but 'New Guest Session' and 'Exist Guest Session' remained the same. Is it expected?
Tested using:
Device: One A2001 (Android 5.1.1)
Build: Firefox for Android 49.0a1 (2016-05-29)
Updated•9 years ago
|
Flags: needinfo?(bhavindave88)
Comment 55•8 years ago
|
||
Margaret, can you weigh in on comment 54 please?
Flags: needinfo?(margaret.leibovic)
Comment 56•8 years ago
|
||
Based on comment 40, it sounds like we do want to lower case "guest session". I'll file a follow-up bug.
Flags: needinfo?(margaret.leibovic)
Updated•8 years ago
|
Flags: needinfo?(bhavindave88)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•