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)

Product:
Firefox for Android Graveyard
Component:
Theme and Visual Design
Version:
24 Branch
Platform:
x86
macOS
Type:
defect

Tracking

(firefox49 fixed, fennec+)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox49 --- fixed
fennec + ---

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)

893836.patch
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.
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]
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! :)
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
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 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+
Flod - Do we need to make the entity name change for changes to capitalization?
Flags: needinfo?(francesco.lodolo)
(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.
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 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+
Keywords: checkin-needed
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
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?
(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.
tracking-fennec: --- → ?
tracking-fennec: ? → +
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)
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)
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=properties][lang=dtd] → [lang=properties][lang=dtd]
filter on [mass-p5]
Priority: -- → P5
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)
Blocks: 1073294
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)
Attached patch SentenceCase.patch (obsolete) — — Splinter Review
./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 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+
vikneshwar, are you still working on this? It would be great to finish this up and land it!
Flags: needinfo?(michaelcevans10) → needinfo?(lviknesh)
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]
Flags: needinfo?(lviknesh)
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?
D'oh, looks like I forgot to needinfo? this.
Flags: needinfo?(margaret.leibovic)
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)
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)
(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)
(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.
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)
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 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)
Attachment #8677535 - Flags: review?(margaret.leibovic)
Oh, sorry about that. Will upload the patch in the proper way soon.
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)
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)
(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]
User Story: (updated)
Attachment #8510655 - Attachment is obsolete: true
Attachment #8677534 - Attachment is obsolete: true
Attachment #8677535 - Attachment is obsolete: true
Oh! sorry left out guest session. Exist Guest Session > Exist guest session. Find in Page > Find in page Request Desktop Site > Request desktop site
User Story: (updated)
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
User Story: (updated)
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.
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
(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.
Attached patch patch_893836_android_strings.dtd (obsolete) — — Splinter Review
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
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.
Attached patch 893836.patch — — Splinter Review
Hi Mike, Hope this patch file conforms to the standard.
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 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+
Attachment #8754010 - Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
Assignee: nobody → bhavindave88
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/340aab956ac7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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)
Hi Ryan, Can you share some preventive steps to ensure this or should I simply delete lines with "autom4te.cache" in future patches?
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)
Flags: needinfo?(bhavindave88)
Margaret, can you weigh in on comment 54 please?
Flags: needinfo?(margaret.leibovic)
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)
Depends on: 1282511
Flags: needinfo?(bhavindave88)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: