Closed
Bug 954362
Opened 10 years ago
Closed 10 years ago
Add keyboard shortcuts to menu
Categories
(Instantbird Graveyard :: Other, defect)
Instantbird Graveyard
Other
Tracking
(Not tracked)
RESOLVED
FIXED
1.1
People
(Reporter: bugzilla, Assigned: mmkmou)
References
Details
(Whiteboard: [1.1-blocking])
Attachments
(2 files, 2 obsolete files)
4.55 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
4.27 KB,
patch
|
Details | Diff | Splinter Review |
*** Original post on bio 929 by mmkmou <mmkmou AT mmkmou.net> at 2011-07-15 19:04:00 UTC *** Some Menu Item have not skeyboard shortcuts
Reporter | ||
Comment 1•10 years ago
|
||
*** Original post on bio 929 as attmnt 745 by mmkmou AT mmkmou.net at 2011-07-15 19:09:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8352487 [details] [diff] [review] patch to add shortcut to the menu *** Original change on bio 929 attmnt 745 by mmkmou AT mmkmou.net at 2011-07-15 23:49:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352487 -
Flags: review?
Comment 3•10 years ago
|
||
Comment on attachment 8352487 [details] [diff] [review] patch to add shortcut to the menu *** Original change on bio 929 attmnt 745 at 2011-07-16 00:02:13 UTC *** Overall, this looks like it would work. Thanks for working on it! :-) I'm commenting mostly on the coding style details that you need to improve. Before checking-in (the next version of) the patch I'll try it to verify that it actually works (but I assume you have done so already) and check that all the new command keys seem to make sense and don't seem to conflict (I assume you have done so too already). >diff -r 54edc171e4ce instantbird/content/menus.xul >--- a/instantbird/content/menus.xul Wed Jul 13 02:01:42 2011 +0200 >+++ b/instantbird/content/menus.xul Fri Jul 15 18:57:56 2011 +0000 >@@ -21,6 +21,7 @@ > # the Initial Developer. All Rights Reserved. > # > # Contributor(s): >+# Mouhamadou Moustapha CAMARA a.k.a mmkmou <mmkmou@mmkmou.net> <http://mmkmou.net> The format for this line is 2 spaces (+ '# ' here), then full name (no need for the IRC nickname), then <email address> (no need for the website URL). >- </commandset> >+ <command id="joinchat" oncommand="menus.joinChat()"/> >+ <command id="addbuddy" oncommand="menus.addBuddy()"/> >+ <command id="addons" oncommand="menus.addons()"/> >+ </commandset> Maybe prefix the id with cmd_ ? And please remove the additional space before </commandset> :). > > <keyset id="mainkeyset"> > <key id="accountsetup" key="&account.commandkey;" command="accountmanager" modifiers="accel,shift"/> > <key id="errorConsoleKey" key="&errorConsoleCmd.commandkey;" oncommand="menus.errors();" modifiers="accel,shift"/> > <key id="key_quitApplication" key="&quitApplicationCmdMac.key;" command="cmd_quitApplication" modifiers="accel"/> >+ <key id="joinchatkey" key="&joinChat.commandkey;" command="joinchat" modifiers="accel"/> >+ <key id="addbuddykey" key="&addBuddy.commandkey;" command="addbuddy" modifiers="accel"/> >+ <key id="addonskey" key="&addonManager.key;" command="addons" modifiers="accel"/> Maybe use camelCase for the ids there? (like errorConsoleKey) >@@ -91,14 +98,14 @@ >- oncommand="menus.preferences();"/> >+ oncommand="menus.preferences();" /> Please revert this change. > <menupopup id="helpMenuPopup" onpopupshowing="menus.displayUpdateStatus();"> >- <menuitem id="updatesMenuItem" label="&checkForUpdates;" oncommand="menus.updates()"/> >+ <menuitem id="updatesMenuItem" label="&checkForUpdates;" oncommand="menus.updates()" /> Same here. >diff -r 54edc171e4ce instantbird/locales/en-US/chrome/instantbird/instantbird.dtd >--- a/instantbird/locales/en-US/chrome/instantbird/instantbird.dtd Wed Jul 13 02:01:42 2011 +0200 >+++ b/instantbird/locales/en-US/chrome/instantbird/instantbird.dtd Fri Jul 15 18:57:56 2011 +0000 >@@ -17,11 +17,21 @@ > <!ENTITY available "Available"> >+<!ENTITY available.commandkey "v"> >+<!ENTITY available.accesskey "v"> > <!ENTITY unavailable "Unavailable"> >+<!ENTITY unavailable.commandkey "u"> >+<!ENTITY unavailable.accesskey "U"> > <!ENTITY offline "Offline"> >+<!ENTITY offline.commandkey "o"> >+<!ENTITY offline.accesskey "O"> When possible, try to keep the values aligned. Here do it at least for the 3 lines of each menuitem ;). >@@ -31,6 +41,7 @@ > <!ENTITY contacts.commandkey "c"> > <!ENTITY contacts.accesskey "C"> > <!ENTITY addonManager "Add-ons"> >+<!ENTITY addonManager.key "d"> Alignment. >@@ -40,7 +51,7 @@ > <!ENTITY helpWin.menu "Help"> > <!ENTITY helpWin.accesskey "H"> > <!ENTITY about.menu "About &brandShortName;"> >-<!ENTITY about.accesskey "A"> >+<!ENTITY about.accesskey "A"> Please remove the trailing space you added here.
Attachment #8352487 -
Flags: review? → review-
Reporter | ||
Comment 4•10 years ago
|
||
*** Original post on bio 929 as attmnt 748 by mmkmou AT mmkmou.net at 2011-07-16 01:08:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352490 -
Flags: review?
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8352487 [details] [diff] [review] patch to add shortcut to the menu *** Original change on bio 929 attmnt 745 by mmkmou AT mmkmou.net at 2011-07-16 01:08:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352487 -
Attachment is obsolete: true
Comment 6•10 years ago
|
||
*** Original post on bio 929 at 2011-07-16 02:03:15 UTC *** Just updating the status.
Assignee: nobody → bugzilla
Severity: normal → minor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Comment 7•10 years ago
|
||
Comment on attachment 8352490 [details] [diff] [review] Patch V2 *** Original change on bio 929 attmnt 748 at 2011-07-20 18:22:57 UTC *** Looks good. I'm changing two trivial details before commiting the patch: >diff -r 54edc171e4ce instantbird/locales/en-US/chrome/instantbird/instantbird.dtd >--- a/instantbird/locales/en-US/chrome/instantbird/instantbird.dtd Wed Jul 13 02:01:42 2011 +0200 >+++ b/instantbird/locales/en-US/chrome/instantbird/instantbird.dtd Sat Jul 16 01:05:38 2011 +0000 >@@ -17,7 +17,11 @@ > <!ENTITY quitApplicationCmdMac.key "Q"> > > <!ENTITY addBuddy "Add Buddy..."> >-<!ENTITY joinChat "Join Chat..."> >+<!ENTITY addBuddy.commandkey "b"> >+<!ENTITY addBuddy.accesskey "B"> >+<!ENTITY joinChat "Join Chat..."> >+<!ENTITY joinChat.commandkey "i"> >+<!ENTITY joinChat.accesskey "i"> The value for addBuddy should be aligned too here. >@@ -40,7 +45,7 @@ > <!ENTITY helpWin.menu "Help"> > <!ENTITY helpWin.accesskey "H"> > <!ENTITY about.menu "About &brandShortName;"> >-<!ENTITY about.accesskey "A"> >+<!ENTITY about.accesskey "A"> > <!ENTITY conversation.contextMenu.close "Close Tab"> > <!ENTITY chat.participants "Participants:"> You forgot to revert this change.
Attachment #8352490 -
Flags: review? → review+
Comment 8•10 years ago
|
||
*** Original post on bio 929 at 2011-07-20 23:20:35 UTC *** Pushed this as https://hg.instantbird.org/instantbird/rev/96d28918a3c8 The changes should be visible in the next nightly. Thanks for fixing this! :)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1
Comment 9•10 years ago
|
||
*** Original post on bio 929 at 2011-07-21 16:06:23 UTC *** There's a bug/regression: the menu items aren't disabled when no account is connected (both "Add Buddy" and "Join Chat" disabled) or no MUC-capable account is connected ("Join Chat" disabled). That's the problem I ran into with bug 953859 (bio 418): Disabling the menu entries alone doesn't work here. You need to disable the command instead (which takes care of disabling/enabling the menu items using attribute broadcasting).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•10 years ago
|
||
*** Original post on bio 929 at 2011-07-21 16:29:16 UTC *** (In reply to comment #7) > There's a bug/regression: the menu items aren't disabled when no account is > connected (both "Add Buddy" and "Join Chat" disabled) or no MUC-capable account > is connected ("Join Chat" disabled). > > That's the problem I ran into with bug 953859 (bio 418): > Disabling the menu entries alone doesn't work here. You need to disable the > command instead (which takes care of disabling/enabling the menu items using > attribute broadcasting). 18:22:46 - Mic (Mozilla): [...] Having one command and observing account connect/disconnect events (checking if there are any connected accounts/MUC capable accouns) and updating this command .. 18:23:14 - Mook_as: sounds about right, yes. 18:23:23 - Mook_as: or write a controller that does the same thing.
Comment 11•10 years ago
|
||
*** Original post on bio 929 as attmnt 803 at 2011-09-02 10:44:00 UTC *** I'm annoyed by the regression mentioned in comment 7. The menu items are broken, we can't ship with them in that state :(. The attached patch puts the disabled attribute on the commands instead of the menuitems, which allows correct interactions with the menus. However, the keyboard shortcut don't work right, as they won't be enabled/disabled until the user opens the file menu. If nobody comes up with a correct patch, I think we will ship with that workaround. The solution using a controller that Mook proposed on IRC seems like the way to go, but more work to implement it.
Updated•10 years ago
|
Whiteboard: [1.1-blocking]
Comment 12•10 years ago
|
||
*** Original post on bio 929 as attmnt 858 at 2011-10-04 10:47:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Comment 13•10 years ago
|
||
Comment on attachment 8352545 [details] [diff] [review] Workaround *** Original change on bio 929 attmnt 803 at 2011-10-04 10:47:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352545 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
*** Original post on bio 929 at 2011-10-04 17:17:13 UTC *** Pushed attachment 8352601 [details] [diff] [review] (bio-attmnt 858) as https://hg.instantbird.org/instantbird/rev/60cbf7f3336b
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•10 years ago
|
Assignee: bugzilla → mmkmou
You need to log in
before you can comment on or make changes to this bug.
Description
•