Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 380301 - Binding lookup must take scheme into account
Summary: Binding lookup must take scheme into account
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.2   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.2 RC2   Edit
Assignee: Paul Webster CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 375762 380322
  Show dependency tree
 
Reported: 2012-05-22 12:18 EDT by Paul Webster CLA
Modified: 2012-05-24 08:31 EDT (History)
4 users (show)

See Also:
bsd: review+
ob1.eclipse: review+

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Webster CLA 2012-05-22 12:18:08 EDT 
When looking up a binding, return the binding for the most specific scheme (child scheme) when found.  Correctly find conflicts when applying model changes by only looking in the matching binding table.

PW
Comment 1 Paul Webster CLA 2012-05-22 13:08:19 EDT 
I have a fix that allows the keybindings to correctly pick up a child scheme overriding a parent scheme.

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=pwebster/bug375762&id=4e9dcd46a011d4bee52385174fed97325b2ff08d

Eric, Oleg, could you please review?

A testcase would be to create a binding for CTRL+5, A in "In Windows", and then a binding to a different command in the emacs scheme in "In dialogs and windows".

The second keybinding should beat the first, even though windows is a child of dialogAndWindows.

John, could I have you look at the fix as well?

PW
Comment 2 Paul Webster CLA 2012-05-22 13:19:02 EDT 
Sorry to have to drag you into keybindings, John, but could you review this?

PW
Comment 3 Oleg Besedin CLA 2012-05-22 14:43:25 EDT 
The patch works for me. 

Couple tiny nitpicks:

- Is it possible to have null schemeIDs inBindingTableManager#compareSchemes() ?
- Typo in #compareSchemes() Javadoc:
	 * Returns an in based on scheme 1 < scheme 2
Comment 4 John Arthorne CLA 2012-05-22 16:48:09 EDT 
I spent the whole day reviewing fixes and still didn't get to this one. Eric if you get to it first tomorrow, go ahead and review. Otherwise I'll try getting to it tomorrow.
Comment 5 Brian de Alwis CLA 2012-05-23 14:40:00 EDT 
I'm not expert in the keybindings, but the changes makes sense in my reading, and I haven't encountered issues in my tests.  So +1 from me.
Comment 7 Paul Webster CLA 2012-05-24 08:31:30 EDT 
In I20120523-1900

PW