Closed Bug 420371 Opened 12 years ago Closed 7 years ago

rename quitApplicationCmdMac.key to reflect reuse on Linux

Categories

(Firefox :: Menus, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: myk, Assigned: jlboezeman)

References

Details

(Whiteboard: [good first bug][mentor=mconley][lang=xul])

Attachments

(1 file, 1 obsolete file)

The fix for bug 189290 will reuse the quitApplicationCmdMac.key entity for Linux because it's late in the Fx3 cycle to be taking changes to these entities, but ultimately that should be called something like quitApplicationCmdUnix.key or perhaps just quitApplicationCmd.key.
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=mconley][lang=xul]
Hello, I have never contributed to Mozilla and would like to take this bug if it still needs addressing. If it does still need addressing, the patch should look something like: 
-key="&quitApplicationCmdMac.key;"
+key="&quitApplicationCmdUnix.key;"
for all instances of "quitApplicationCmdMac", correct? Also, should this extend to instances that are "quitApplicationCmdMac\..*"  (note that only the "quitApplicationCmdMac" would be replaced, not anything after) or only "quitApplicationCmdMac.key"?
Flags: needinfo?(mconley)
Hey John! Thanks for your interest in this bug!

So I think this is what I think we should do:

Here's what needs to be done:

quitApplicationCmdMac.key usage should be changed to quitApplicationCmdUnix.key in here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#388

We also need to change the place where we define what quitApplicationCmdMac.key is - that's over here:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#539

So that entity name will need to be changed to.

(In reply to John from comment #1)
> Also, should this
> extend to instances that are "quitApplicationCmdMac\..*"  (note that only
> the "quitApplicationCmdMac" would be replaced, not anything after) or only
> "quitApplicationCmdMac.key"?

No, I think quitApplicationCmdMac.label is still necessary. In this case, I think we only care about the keyboard shortcut.

Once you've got a Firefox build up and running (https://developer.mozilla.org/en/docs/Simple_Firefox_build), make those changes, and then execute this command:

./mach build browser

And then restart your browser, making sure the change went through without breaking the keyboard shortcut.

Once you've verified that, we'll need a patch from you. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F is a good primer for that, but I'm happy to help you through it when you get there.

I'm mconley on irc.mozilla.org - come and find me if you have any questions, or send me email.

Thanks!
Flags: needinfo?(mconley)
Attached patch Proposed Patch (obsolete) — Splinter Review
(In reply to Mike Conley (:mconley) from comment #2)
> quitApplicationCmdMac.key usage should be changed to
> quitApplicationCmdUnix.key in here:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> sets.inc#388
> 
> We also need to change the place where we define what
> quitApplicationCmdMac.key is - that's over here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#539

I have done this, built the browser, and tested it on my Linux (ubuntu derivative) machine.

I also noticed from searching the repository that "quitApplicationCmdMac.key" is used twice in webapprt, should these be changed as well?

/browser/locales/en-US/webapprt/webapp.dtd

    line 21 -- <!ENTITY quitApplicationCmdMac.key "Q">

/webapprt/content/webapp.xul

    line 48 -- key="&quitApplicationCmdMac.key;"

I have not changed these in my current patch.
Attachment #725593 - Flags: feedback+
Attachment #725593 - Flags: feedback+ → feedback?
Comment on attachment 725593 [details] [diff] [review]
Proposed Patch

Review of attachment 725593 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good John! Just two suggestions - see below.

(In reply to John from comment #3)
> Created attachment 725593 [details] [diff] [review]
> Proposed Patch
> 
> I also noticed from searching the repository that
> "quitApplicationCmdMac.key" is used twice in webapprt, should these be
> changed as well?
>

No, let's file a separate bug for them.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +535,5 @@
>  <!ENTITY goForwardCmd.commandKey "]">
>  <!ENTITY quitApplicationCmd.label       "Quit"> 
>  <!ENTITY quitApplicationCmd.accesskey   "Q">
>  <!ENTITY quitApplicationCmdMac.label    "Quit &brandShortName;">
> +<!ENTITY quitApplicationCmdUnix.key      "Q">

Nit - please make this line up with the items above by removing one of those space characters.

Also, can you add this comment above:

<!-- LOCALIZATION NOTE(quitApplicationCmdUnix.key): This keyboard shortcut is used by both Linux and OSX builds. -->
Attachment #725593 - Flags: feedback? → feedback+
Assignee: nobody → jlboezeman
Fixed, built, tested on Linux (Ubuntu derivative) machine.
Attachment #725593 - Attachment is obsolete: true
Attachment #725610 - Flags: feedback?
Comment on attachment 725610 [details] [diff] [review]
Proposed Patch v2

John, not your fault but feedback and review requests with no target ("to the wind") tend to go unnoticed.
Attachment #725610 - Flags: feedback? → feedback?(mconley)
Comment on attachment 725610 [details] [diff] [review]
Proposed Patch v2

Sorry for the delay - this looks good to me! r=me, while I'm at it.
Attachment #725610 - Flags: review+
Attachment #725610 - Flags: feedback?(mconley)
Attachment #725610 - Flags: feedback+
(In reply to Mike Conley (:mconley) from comment #7)
> Comment on attachment 725610 [details] [diff] [review]
> Proposed Patch v2
> 
> Sorry for the delay - this looks good to me! r=me, while I'm at it.

Alright, great. Is there anything I need still need to do or am I good?
(In reply to John from comment #8)
> (In reply to Mike Conley (:mconley) from comment #7)
> > Comment on attachment 725610 [details] [diff] [review]
> > Proposed Patch v2
> > 
> > Sorry for the delay - this looks good to me! r=me, while I'm at it.
> 
> Alright, great. Is there anything I need still need to do or am I good?

Nope, I don't believe so. I've set checkin-needed on the bug, so someone will come by and commit it for you soon. Assuming that all goes smoothly, your job is done. :) Just keep an eye on the bug in case it doesn't, for some reason, go smoothly.

Thanks for the patch!
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb3df70550a

Thanks for the patch, John! You'll see this bug resolved in the next day or so once this patch is uplifted to mozilla-central.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fb3df70550a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.