Closed
Bug 420371
Opened 16 years ago
Closed 11 years ago
rename quitApplicationCmdMac.key to reflect reuse on Linux
Categories
(Firefox :: Menus, defect)
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)
2.46 KB,
patch
|
mconley
:
review+
mconley
:
feedback+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Whiteboard: [good first bug]
Updated•11 years ago
|
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"?
Updated•11 years ago
|
Flags: needinfo?(mconley)
Comment 2•11 years ago
|
||
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)
(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 4•11 years ago
|
||
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+
Updated•11 years ago
|
Assignee: nobody → jlboezeman
Fixed, built, tested on Linux (Ubuntu derivative) machine.
Attachment #725593 -
Attachment is obsolete: true
Attachment #725610 -
Flags: feedback?
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Updated•11 years ago
|
Keywords: checkin-needed
(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?
Comment 9•11 years ago
|
||
(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!
Comment 10•11 years ago
|
||
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
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9fb3df70550a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in
before you can comment on or make changes to this bug.
Description
•