Comment 1 Simon Chemouil 2009-10-23 06:04:53 EDT

Created attachment 150355 [details]
A proposed patch for this enhancement request.

The patch implements the request to show the available key bindings for a given command in the QuickAccessDialog. It does not change the API anyhow, and remains in the same spirit (ie, the key sequence is appened to the label, and not in a distinct field).

I also had to change QuickAccessDialog to augment its context in order to have the IBindingService provide the global key shortcuts (this is inspired/copied from the KeyAssistDialog).

It seems to work fine ; this is my first patch for Eclipse so please be nice :).

Comment 2 Remy Suen 2009-10-25 09:57:40 EDT

(In reply to comment #1)
> Created an attachment (id=150355) [details]
> A proposed patch for this enhancement request.

This looks okay to me, Simon. Can you prepare a new patch that sets the 'Patch Root' to the workspace (on the second page of the patch creation wizard)? Also, please update the copyright notice of every file that you have changed with your credentials like the following example...

Remy Suen <remysuen@ca.ibm.com> - Bug 12345 Summary blah blah blah

Comment 3 Boris Bokowski 2009-10-25 14:18:29 EDT

(In reply to comment #1)
> Created an attachment (id=150355) [details]
> A proposed patch for this enhancement request.

Cool! Thanks for the patch, could you also attach a screenshot of how it looks with the key bindings displayed?

Comment 4 Simon Chemouil 2009-10-26 05:24:15 EDT

Created attachment 150494 [details]
Patch revised according to Remy Suen's comment

Thanks for the comment Remy.

I have adapted the patch accordingly.

Comment 5 Simon Chemouil 2009-10-26 05:30:21 EDT

Created attachment 150495 [details]
Screenshot of the patched Quickaccess 

(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=150355) [details] [details]
> > A proposed patch for this enhancement request.
> 
> Cool! Thanks for the patch, could you also attach a screenshot of how it looks
> with the key bindings displayed?

Here it goes.

In most cases we have to resize the Quickaccess popup to see the bindings, because some commands have a rather lengthy name. A nice bonus to adding the binding in the label is that one can filter on the key shortcut, which is quite nice to learn some shortcuts the other way around (maybe the KeyAssist popup is better for that though, since it only shows commands with key bindings).

On a side note, there might be a bug to report with some commands (available in standard Eclipse distributions) that assume they're not called from outside and throw exceptions when called from the Quickaccess popup (the patch does not change anything there -- the bug exists in Galileo).

Comment 6 Paul Webster 2009-10-26 08:33:40 EDT

I wouldn't try and treat the quick access shell as a useful shell.

Instead, you can use org.eclipse.jface.bindings.BindingManager.getActiveBindingsDisregardingContext() or org.eclipse.jface.bindings.BindingManager.getActiveBindingsDisregardingContextFlat() to work find the matching bindings.

PW

Comment 7 Simon Chemouil 2009-10-26 09:26:37 EDT

Created attachment 150505 [details]
Revision according to Paul Webster's comment

(In reply to comment #6)
> I wouldn't try and treat the quick access shell as a useful shell.
> 
> Instead, you can use
> org.eclipse.jface.bindings.BindingManager.getActiveBindingsDisregardingContext()
> or
> org.eclipse.jface.bindings.BindingManager.getActiveBindingsDisregardingContextFlat()
> to work find the matching bindings.

Thanks for the pointer. Here it goes.

I take it I can safely assume the IBindingService will always be a BindingService from within the workbench (since it was registered in Workbench.java). If not, I can make a type check before using it and fallback on the regular, not context-disregarding IBindingService.getActiveBinding() method available.

Also, there is no getBestActiveBindingsDisregardingContextXXX() family of methods, so in this patch I simply return the first binding if there is one. Maybe it would be refactorable into a getBestActiveBindingForCommandInContext(command, context) with context disregarded if null.

Finally, I had adapted the registerShell() method from KeyAssistDialog.java which doesn't look more like an useful shell than the QuickAccessDialog to me. Maybe it should be adapted to use the BindingManager.getActiveBindindsDisregardingContext() methods too.

Comment 8 Simon Chemouil 2009-10-26 09:59:08 EDT

(In reply to comment #7) 
> I take it I can safely assume the IBindingService will always be a
> BindingService from within the workbench (since it was registered in
> Workbench.java). If not, I can make a type check before using it and fallback
> on the regular, not context-disregarding IBindingService.getActiveBinding()
> method available.

Actually I can reply to myself: it is possible for third parties to call PlatformUI.getWorkbench().getServiceLocator().registerService(IBindingService.class, theirServiceImplementation); with theirServiceImplementation likely not inheriting BindingService which is internal.

Obviously, this would result in a ClassCastException...

However, to do that you must implement IBindingService.. which clearly states:
 * @noimplement This interface is not intended to be implemented by clients.
 * @noextend This interface is not intended to be extended by clients.

So I guess it's OK.

Comment 9 Boris Bokowski 2009-11-11 16:37:27 EST

I'm happy with the functionality (looking up key bindings is cool) but not as happy yet with the look. I think the key binding has to be in its own column, or rendered in a way that is less distracting, perhaps in grey similar to the CVS decorations in the package explorer.

Comment 10 Boris Bokowski 2009-11-26 16:16:09 EST

Remy is now responsible for watching bugs in the [QuickAccess] component area.

Comment 11 Remy Suen 2009-12-11 09:47:21 EST

*** Bug 215178 has been marked as a duplicate of this bug. ***

Comment 12 Remy Suen 2010-03-16 11:41:54 EDT

(In reply to comment #9)
> I'm happy with the functionality (looking up key bindings is cool) but not as
> happy yet with the look. I think the key binding has to be in its own column,
> or rendered in a way that is less distracting, perhaps in grey similar to the
> CVS decorations in the package explorer.

Susan, I would prefer colouring it differently over making another column, what do you think?

Comment 13 Remy Suen 2010-04-12 12:10:23 EDT

Created attachment 164579 [details]
Show keybindings for QuicKAccess patch v4

This patch will use the JFace "qualifier" colour for decorating the keybinding.

Comment 14 Remy Suen 2010-04-12 12:14:21 EDT

Created attachment 164581 [details]
Screenshot depicting the behaviour in question.

This is on Windows XP.

Comment 15 Boris Bokowski 2010-04-12 13:42:52 EDT

(In reply to comment #14)
> Created an attachment (id=164581) [details]
> Screenshot depicting the behaviour in question.
> 
> This is on Windows XP.

I like it.

Comment 16 Remy Suen 2010-04-13 07:55:40 EDT

Created attachment 164692 [details]
Show keybindings for QuicKAccess patch v5

Generalize the appending of "qualifier" information.

Comment 17 Remy Suen 2010-04-13 07:57:17 EDT

(In reply to comment #16)
> Created an attachment (id=164692) [details]
> Show keybindings for QuicKAccess patch v5

Patch released to HEAD. Thanks for the original patch, Simon!

Comment 18 Remy Suen 2010-04-13 08:26:13 EDT

Whoops.

Comment 19 Markus Keller 2010-04-14 08:34:32 EDT

In other places where we colorize qualifiers, we also colorize the separator. See e.g. a JAR in the 'JRE System Library' in the Package Explorer: '-' is also gray.

In tooltips, we render key bindings as '<label> (<binding>)'. Do we really need a different notation here?

Comment 20 Remy Suen 2010-04-14 08:42:18 EDT

(In reply to comment #19)
> In tooltips, we render key bindings as '<label> (<binding>)'. Do we really need
> a different notation here?

Mm, I see your point. I was initially thinking of dropping the " - " but kept it in the end. However, there's also the problem that other labels use the " - " string as a separator so I think I'll swap this so that the keybinding are contained within a pair of parentheses.

Comment 21 Remy Suen 2010-04-14 08:47:52 EDT

Fixed in HEAD. Thanks for the tip, Markus.

Comment 22 Markus Keller 2010-04-15 05:07:01 EDT

Looks good.

One last problem: The parentheses should be externalized. Since you can't know where the shortcut will be rendered, you should implement the coloring like e.g.
org.eclipse.jdt.internal.ui.text.correction.proposals.ChangeCorrectionProposal.getStyledDisplayString():

StyledString str= new StyledString(getName());
String shortCutString= CorrectionCommandHandler.getShortCutString(
	getCommandId());
if (shortCutString != null) {
	String decorated= Messages.format(
		CorrectionMessages.ChangeCorrectionProposal_name_with_shortcut,
		new String[] { getName(), shortCutString });
	return StyledCellLabelProvider.styleDecoratedString(
		decorated, StyledString.QUALIFIER_STYLER, str);
}

Comment 23 Remy Suen 2010-04-16 09:01:54 EDT

Created attachment 165091 [details]
Show keybindings for QuickAccess patch v6

(In reply to comment #22)
> One last problem: The parentheses should be externalized.

I decided to reuse CommandMessages.Tooltip_Accelerator here.

> Since you can't know
> where the shortcut will be rendered, you should implement the coloring like
> e.g.
> org.eclipse.jdt.internal.ui.text.correction.proposals.ChangeCorrectionProposal.getStyledDisplayString():

I don't want to swap the code to completely use a StyledCellLabelProvider in M7. What do you think of this patch, Markus? I'm a little iffy about applying all the returned StyleRanges but since I don't know how the string will be externalized, I can't really "pick and choose".

Comment 24 Remy Suen 2010-04-21 21:06:30 EDT

(In reply to comment #23)
> Created an attachment (id=165091) [details]
> Show keybindings for QuickAccess patch v6

Markus, what do you think about this? I'd like to get this corrected for M7 if possible instead of pulling the feature out and then leaving it for 3.7.

Comment 25 Markus Keller 2010-04-22 05:42:01 EDT

(In reply to comment #24)
> (In reply to comment #23)
> > Created an attachment (id=165091) [details] [diff] [details]
> > Show keybindings for QuickAccess patch v6
> 
> Markus, what do you think about this? I'd like to get this corrected for M7 if
> possible instead of pulling the feature out and then leaving it for 3.7.

Looks good, please release.

Comment 26 Remy Suen 2010-04-22 09:06:24 EDT

(In reply to comment #25)
> Looks good, please release.

Released to HEAD. Thank you very much, Markus.

Comment 27 Remy Suen 2010-04-26 08:21:19 EDT

Verified with I20100425-2000 on Windows XP.

Thanks for the assistance, Simon and Markus.

Comment 28 Dani Megert 2010-05-25 09:46:11 EDT

Filed bug 314269 to follow-up on this.

Comment 29 Remy Suen 2010-09-01 22:23:23 EDT

*** Bug 324264 has been marked as a duplicate of this bug. ***