Closed
Bug 1058140
Opened 10 years ago
Closed 10 years ago
"troubleshooting" button in WebIDE should be capitalized
Categories
(DevTools Graveyard :: WebIDE, defect)
DevTools Graveyard
WebIDE
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: dholbert, Assigned: anirudhgp, Mentored)
References
Details
(Whiteboard: [good first bug][lang=html])
Attachments
(2 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
STR:
1. Tools|Web Developer|WebIDE
2. Click the runtime dropdown at upper-right, and pick "Remote Runtime"
3. Hit "Cancel" button
ACTUAL RESULTS: A dropdown appears saying:
> Operation failed: connecting to runtime [troubleshooting]x
EXPECTED RESULTS: The "troubleshooting" button should be capitalized, since button-labels in Firefox UI are generally capitalized.
I believe this button's text lives here, in webide.properties:
> notification_showTroubleShooting_label=troubleshooting
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/webide.properties?rev=a7c77bdfd4ac&mark=22-22#22
This text was added in bug 1011464.
Reporter | ||
Comment 1•10 years ago
|
||
Mentor: jryans
Whiteboard: [good first bug][lang=html]
Assignee | ||
Comment 2•10 years ago
|
||
Hi I'd like to work on this bug. How do i begin?
Great! Please take a look at our hacking guide[1] to get started.
Comment 0 mentions the file to change.
Normally when changing strings, we also have to change the string key, but since this one does not change the meaning, we can leave it alone[2].
[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #3)
> Great! Please take a look at our hacking guide[1] to get started.
>
> Comment 0 mentions the file to change.
>
> Normally when changing strings, we also have to change the string key, but
> since this one does not change the meaning, we can leave it alone[2].
>
> [1]: https://wiki.mozilla.org/DevTools/Hacking
> [2]:
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_best_practices#Changing_existing_strings
Hey I just built the fx-team on my PC. What exactly do i change in the file? Do i change troubleshooting to TROUBLESHOOTING?
Reporter | ||
Comment 5•10 years ago
|
||
Just the "T" at the beginning should be capitalized -- so, "Troubleshooting"
Reporter | ||
Comment 6•10 years ago
|
||
(And then you'll want to rebuild & test by performing the STR (steps to reproduce) in comment 0, to be sure your change actually has the right effect.)
Assignee | ||
Comment 7•10 years ago
|
||
Ok i'll do that right away!
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8500022 -
Flags: review?(jryans)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8500022 [details] [diff] [review]
bug-1058140-fix.patch
Two drive-by nits, on the patch headers:
># User anirudhgp
This should include both a name and email address. This comes from your .hgrc file -- see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_do_I_check_stuff_in.3F for how to configure that with your username.
>Bug 1058140 - Changed troubleshooting to Troubleshooting in c:/mozilla-source/mozilla-central/browser/locales/en-US/chrome/browser/devtools/webide.properties
There's no need to mention "c:/mozilla-source/mozilla-central/" in the commit message -- I'm assuming that's the path on your system, but that's probably not the path for many other folks. :) You could really just say "webide.properties" with no path information; that's unique enough. (and people who are curious can always look at the commit for the actual file path)
Assignee | ||
Comment 10•10 years ago
|
||
Thanks! I'll keep that in mind.
Reporter | ||
Comment 11•10 years ago
|
||
(or if you prefer, you could just say "...on a button in the WebIDE UI", instead of mentioning the properties filename. That makes it a little clearer what the real reason / goal for the change is.)
Assignee | ||
Comment 12•10 years ago
|
||
Ok ill do that from now!
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8500022 -
Attachment is obsolete: true
Attachment #8500022 -
Flags: review?(jryans)
Attachment #8500104 -
Flags: review?(jryans)
Comment 14•10 years ago
|
||
Can i be assigned to this please
Comment 15•10 years ago
|
||
Can I be assigned to this bug please
Comment 16•10 years ago
|
||
I would like to be assigned this bug if this is possible, where do I start?
Assignee: nobody → anirudh.gp
Status: NEW → ASSIGNED
For comments 14 - 16, this bug already has a working patch, so please check out other good first bugs[1] in DevTools.
[1]: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs
Comment on attachment 8500104 [details] [diff] [review]
bug-1058140-fix.patch
Review of attachment 8500104 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, this looks good. In the future, you should also add something like "r=jryans" to the end of your commit message to note the reviewer's name[1]. But, don't worry about updating it this time.
For more complex patches, I would now push this to our Try test infrastructure, but this is a simple change, so land it directly in a few minutes.
Please take a look at our other bugs if you'd like something more complex now that you've got your local setup working.
[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #8500104 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #18)
> Comment on attachment 8500104 [details] [diff] [review]
> bug-1058140-fix.patch
>
> Review of attachment 8500104 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thanks, this looks good. In the future, you should also add something like
> "r=jryans" to the end of your commit message to note the reviewer's name[1].
> But, don't worry about updating it this time.
>
> For more complex patches, I would now push this to our Try test
> infrastructure, but this is a simple change, so land it directly in a few
> minutes.
>
> Please take a look at our other bugs if you'd like something more complex
> now that you've got your local setup working.
>
> [1]:
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
Thanks a lot. I'll surely look into more complex bugs. Do I have to do anything else with regard to this word? I think I have to add some keyword?
(In reply to anirudh.gp from comment #19)
> Thanks a lot. I'll surely look into more complex bugs. Do I have to do
> anything else with regard to this word? I think I have to add some keyword?
The typical workflow is push to Try and then to set "checkin-needed" on the bug, so that the sheriffs will land the patch for us. But since this is so simple, I'm going to land it manually. There's no need to change any keywords on the bug in this case.
Also, don't worry about resolving the bug. After I land the patch on fx-team, it will eventually get merged into mozilla-central, and a sheriff will resolve the bug then.
Whiteboard: [good first bug][lang=html] → [good first bug][lang=html][fixed-in-fx-team]
Assignee | ||
Comment 22•10 years ago
|
||
Oh ok thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=html][fixed-in-fx-team] → [good first bug][lang=html]
Target Milestone: --- → Firefox 35
Comment 24•10 years ago
|
||
Note that by changing the first character of the string only and not the lowercase access key, the latter is probably no longer the first character and hence should have been capitalized as well. Could anyone take action to fix this in the future if this is not what's intended?
Access keys are case sensitive, even though they work for the other case character as well, so it's only cosmetic. It's a good habit though to always use the exact case for a key (even when it occurs only once) in order to avoid wrong keys on string changes like this.
Reporter | ||
Comment 25•10 years ago
|
||
Ah, you're referring to the fact that "Troubleshooting" now has an underline next to the final "t", e.g.
[Troubleshoo_t_ing]
which indeed looks a bit odd (though has no functional effect).
So I think we need to just change the line right after the patch, as well:
> notification_showTroubleShooting_accesskey=t
That should now be "T", presumably, to continue matching the first character. (Though as Ton says, the user is free to type "t" or "T" to trigger the button.)
anirudh, would you be up for posting a followup patch to make that change?
Flags: needinfo?(anirudh.gp)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8500104 -
Attachment is obsolete: true
Flags: needinfo?(anirudh.gp)
Attachment #8506388 -
Flags: review?(jryans)
Reporter | ||
Comment 27•10 years ago
|
||
Comment on attachment 8506388 [details] [diff] [review]
bug-1058140-fix2.patch
Thanks! Three drive-by nits:
># HG changeset patch
># Parent df5248069d98160fdcd4fb0b4bcd1f0a41a2670f
># User anirudhgp <anirudh.gp@gmail.com>
>Changed "Troubleshoo_t_ing" to "Troubleshooting" in a UI in web IDE
^-------.
(1) I think you meant to underline this "T" in the commit message
(2) Commit message should include bug number and generally speculatively include "r=whoever"
So I think this wants to say something like:
Bug 1058140 followup: Change Changed "Troubleshoo_t_ing" to "_T_roubleshooting" in a UI in web IDE. r=jryans
>+++ b/browser/locales/en-US/chrome/browser/devtools/webide.properties
>-notification_showTroubleShooting_label=troubleshooting
>-notification_showTroubleShooting_accesskey=t
>+notification_showTroubleShooting_label=Troubleshooting
>+notification_showTroubleShooting_accesskey=T
(3) Since your first patch already landed (and wasn't backed out), we've already got the first change here (to the label). This followup patch should apply to current mozilla-central, which means it only needs to modify the second line. (The label should already have "T" in it.)
Assignee | ||
Comment 28•10 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #27)
> Comment on attachment 8506388 [details] [diff] [review]
> bug-1058140-fix2.patch
>
> Thanks! Three drive-by nits:
> ># HG changeset patch
> ># Parent df5248069d98160fdcd4fb0b4bcd1f0a41a2670f
> ># User anirudhgp <anirudh.gp@gmail.com>
> >Changed "Troubleshoo_t_ing" to "Troubleshooting" in a UI in web IDE
> ^-------.
> (1) I think you meant to underline this "T" in the commit message
>
> (2) Commit message should include bug number and generally speculatively
> include "r=whoever"
>
> So I think this wants to say something like:
>
> Bug 1058140 followup: Change Changed "Troubleshoo_t_ing" to
> "_T_roubleshooting" in a UI in web IDE. r=jryans
>
>
> >+++ b/browser/locales/en-US/chrome/browser/devtools/webide.properties
> >-notification_showTroubleShooting_label=troubleshooting
> >-notification_showTroubleShooting_accesskey=t
> >+notification_showTroubleShooting_label=Troubleshooting
> >+notification_showTroubleShooting_accesskey=T
>
> (3) Since your first patch already landed (and wasn't backed out), we've
> already got the first change here (to the label). This followup patch
> should apply to current mozilla-central, which means it only needs to modify
> the second line. (The label should already have "T" in it.)
Ok thanks ill keep those points in mind! Yeah i had forgotten to build the code before applying the patch again so both changes were shown in this diff. Do i need to resubmit the patch again with only the second line changed or is it fine?
(In reply to anirudh.gp from comment #28)
> Ok thanks ill keep those points in mind! Yeah i had forgotten to build the
> code before applying the patch again so both changes were shown in this
> diff. Do i need to resubmit the patch again with only the second line
> changed or is it fine?
Yes, please attach a new patch to fix these issues. A large part of getting started is learning how to work through these kinds of workflow steps, so I think it's best for you to update it.
Attachment #8506388 -
Flags: review?(jryans)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(anirudh.gp)
It probably also preferable to solve this new issue in a separate bug for accurate release tracking, since mozilla-central in now Gecko 36, but the first patch landed in Gecko 35.
Assignee | ||
Comment 31•10 years ago
|
||
Sorry guys i was unwell the past week. I'll re-submit the patch to this ASAP. Thanks.
Flags: needinfo?(anirudh.gp)
I have fixed the accesskey as part of bug 1096310, so there's no more to do here.
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•