Closed Bug 954362 Opened 10 years ago Closed 10 years ago

Add keyboard shortcuts to menu

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: mmkmou)

References

Details

(Whiteboard: [1.1-blocking])

Attachments

(2 files, 2 obsolete files)

*** 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
*** 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 ***
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 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-
Attached patch Patch V2Splinter Review
*** 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?
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
*** 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 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+
*** 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
*** 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 → ---
*** 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.
Attached patch Workaround (obsolete) — Splinter Review
*** 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.
Whiteboard: [1.1-blocking]
*** 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 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
*** 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 ago10 years ago
Resolution: --- → FIXED
Depends on: 955103
Assignee: bugzilla → mmkmou
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: