Closed Bug 1058140 Opened 10 years ago Closed 10 years ago

"troubleshooting" button in WebIDE should be capitalized

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

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)

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.
Mentor: jryans
Whiteboard: [good first bug][lang=html]
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
(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?
Just the "T" at the beginning should be capitalized -- so, "Troubleshooting"
(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.)
Ok i'll do that right away!
Attached patch bug-1058140-fix.patch (obsolete) — Splinter Review
Attachment #8500022 - Flags: review?(jryans)
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)
Thanks! I'll keep that in mind.
(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.)
Ok ill do that from now!
Attached patch bug-1058140-fix.patch (obsolete) — Splinter Review
Attachment #8500022 - Attachment is obsolete: true
Attachment #8500022 - Flags: review?(jryans)
Attachment #8500104 - Flags: review?(jryans)
Can i be assigned to this please
Can I be assigned to this bug please
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+
(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.
https://hg.mozilla.org/integration/fx-team/rev/ed38aba58583
Whiteboard: [good first bug][lang=html] → [good first bug][lang=html][fixed-in-fx-team]
Oh ok thanks!
https://hg.mozilla.org/mozilla-central/rev/ed38aba58583
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
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.
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)
Attachment #8500104 - Attachment is obsolete: true
Flags: needinfo?(anirudh.gp)
Attachment #8506388 - Flags: review?(jryans)
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.)
(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.
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.
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.
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.