This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 180543 - SQL: Run SQL missing shortcut - need to set ACCELERATOR from editor toolbars
Summary: SQL: Run SQL missing shortcut - need to set ACCELERATOR from editor toolbars
Status: CLOSED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Actions (show other bugs)
Version: 6.x
Hardware: All All
: P3 normal (vote)
Assignee: Jesse Glick
URL:
Keywords: API, API_REVIEW_FAST
Depends on:
Blocks: 152576
  Show dependency tree
 
Reported: 2010-02-10 07:00 UTC by gliesian
Modified: 2011-11-04 17:29 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Start of patch incl. new API; missing tests, usage in editor, apichanges (11.46 KB, patch)
2010-02-13 07:27 UTC, Jesse Glick
Details | Diff
Probably final patch (17.98 KB, patch)
2010-02-17 14:53 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description gliesian 2010-02-10 07:00:40 UTC
All icons in toolbars include the associated shortcut in the tooltip.

Examples of tooltips for icons: 

  New File... (Ctrl+N)
  File Selection (Ctrl+F3)
  Last edit (Ctrl+Q) Note: To follow convention the e should be capitalized.
  Undo (Ctrl+Z)

However, the Run SQL icon does 'not' include the shortcut.  Consider changing the tooltip for the Run SQL icon from:

  Run SQL

  to

  Run SQL(Ctrl+Shift+E)

Thanks.
Comment 1 Jiri Rechtacek 2010-02-11 08:17:14 UTC
Hint for fixing: make sure openide.loaders/Toolbar.addImpl() processes this button.
Comment 2 Jiri Rechtacek 2010-02-12 04:02:24 UTC
I guess it's caused by fix issue 152576, a keystroke for this action found but won't be propagated as Action.ACCERELERATOR_KEY into action.
Comment 3 Jaroslav Tulach 2010-02-12 08:16:28 UTC
Jirko, if you suspect bug 152576 to be responsible for this problem, then you can assign or CC author of that change directly.
Comment 4 Jesse Glick 2010-02-13 07:26:27 UTC
org.openide.awt.Toolbar is not involved; this is an editor toolbar, processed using different infrastructure. It needs to set an accelerator the same way o.o.a.T would. While it would be possible to simply copy the method that does so, it relies on a hack to communicate with core which I would not want to spread into the editor module, so it is better to clean this up with a new small API - please review.

By the way, in this case it would be better to just delete the shortcut registration for this action, since it can be invoked more naturally now by the standard S-F6. (You could probably just add Run File to the editor toolbar, though I have not tried it.)
Comment 5 Jesse Glick 2010-02-13 07:27:02 UTC
Created attachment 94137 [details]
Start of patch incl. new API; missing tests, usage in editor, apichanges
Comment 6 Jaroslav Tulach 2010-02-15 03:07:16 UTC
Y01 Rather than the javadoc warning, throw exception if instantiated by improper subclass.

Y02 keyStrokeForAction shall be protected to clearly state it is supposed to be implemented and not called (except by the infrastructure)
Comment 7 Vitezslav Stejskal 2010-02-15 03:13:23 UTC
VS1: What would be the intended usage in the editor? Do we have to register our own AcceleratorBinding? This may potentially collide with MainMenuAction and its subclasses.

VS2: I assume that AcceleratorBinding is purely for UI purposes, it does not actually bind a shortcut to an action (at least so it seems in the patch).

VS3: In some cases the binding for an action may change - eg. when user selects a different keybinding profile or (not so common) when the action has different shortcuts for different editors (eg. file types) and user switches from one editor to another. How do these changes propagate through the AcceleratorBinding?
Comment 8 Jesse Glick 2010-02-15 13:43:43 UTC
VS1 - no, the static method will be called from the editor's toolbar code. There is only one impl of the service, associated with NbKeymap.


VS2 - correct.


VS3 - they don't; NbKeymap handles this on its own, since the action has already been loaded.
Comment 9 Jesse Glick 2010-02-17 14:53:41 UTC
Created attachment 94259 [details]
Probably final patch
Comment 10 Jesse Glick 2010-02-19 15:30:38 UTC
core-main #21563c7a0f43