Bug 143911 - differentiate left and right modifier keys in keyPressed event
Summary: differentiate left and right modifier keys in keyPressed event
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows 2000
: P3 enhancement with 5 votes (vote)
Target Milestone: 3.6 M3   Edit
Assignee: Felipe Heidrich CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on:
Blocks: 289752
  Show dependency tree
 
Reported: 2006-05-26 05:31 EDT by Yanye CLA
Modified: 2009-10-26 12:29 EDT (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yanye CLA 2006-05-26 05:31:04 EDT
Is there any way to separate keycode of the keyboard ctrl left and right??

   In the keyPressed event:
    ¿¿¿¿ Event.keyCode == SWT.CTRL_LEFT or SWT.CTRL_RIGHT ????
Comment 1 Grant Gayed CLA 2006-05-29 13:40:25 EDT
SN maybe a left/right indication could be included in the detail field?
Comment 2 Korney Gozman CLA 2006-11-22 00:36:42 EST
Do you mean as in Control-Shift-Left,Right in sourc view while editing text?
v3.2.0?
Comment 3 cloudor CLA 2006-11-22 06:08:50 EST
No. We want to distinguish CTRL/ALT/SHIFT/WIN keys on the left and right in the keyboard.
Comment 4 Lina Kemmel CLA 2009-09-16 04:13:52 EDT
(In reply to comment #2)
> Do you mean as in Control-Shift-Left,Right in sourc view while editing text?
> v3.2.0?

Control-Shift-Left/Right issue is a consequence (or if one likes a particular case) of this problem, caused by the lack of differentiation of Left Shift and Right Shift keystrokes.
Comment 5 Felipe Heidrich CLA 2009-09-21 16:35:31 EDT
I'm not sure what is the right API for this problem.
using SWT.CTRL_LEFT / SWT.CTRL_RIGHT on keycode can't not be done, it break current clients.

using SWT.LEFT/SWT.RIGHT on detail is a possibility, but it doesn't work well for acceleratos (MenuItem#setAccelerator) and also it doens't work for Event#stateMask.

SSQ, any suggestions ?
Comment 6 Shensis Alexander CLA 2009-09-22 07:49:14 EDT
One always can investigate the current key state by code like this:
public void keyPressed(KeyEvent e) {
 if(e.stateMask == SWT.CTRL) {
   if(e.keyCode == SWT.SHIFT) {
    if (OS.GetKeyState(160) < 0)  //VK_LSHIFT
       //do something
    else if (OS.GetKeyState(161) < 0)  //VK_RSHIFT
       //do something else					}
   }
 } 
}
Comment 7 Felipe Heidrich CLA 2009-09-22 08:43:44 EDT
The implementation is not the problem.

We need to define a API to allow the application code to identify left control/right control, left shift/right shift, left alt/right alt.

Comment 6 uses win23 native calls, that obviously cannot be used by application code.
Comment 8 Shensis Alexander CLA 2009-09-23 06:11:03 EDT
I only intended to show possible workaround that user might implement, it certainly may be expanded for
multiplatform usage (GTK) by means of reflection.

As to the problem solution I dare say the only way (taking in account the backward compatibility issue) could be to extend the 'event' class by adding some 'extended' field that would contain the bitwise flags making possible to distinguish between right amd left keys.
Comment 9 Felipe Heidrich CLA 2009-09-29 11:14:52 EDT
I was able to find OS support to implement this request on win32, cocoa, gtk, and motif.

Carbon I don't think it is possible, this is from the header file Events.h:
enum {
  activeFlag                    = 1 << activeFlagBit,
  btnState                      = 1 << btnStateBit,
  cmdKey                        = 1 << cmdKeyBit,
  shiftKey                      = 1 << shiftKeyBit,
  alphaLock                     = 1 << alphaLockBit,
  optionKey                     = 1 << optionKeyBit,
  controlKey                    = 1 << controlKeyBit,
  rightShiftKey                 = 1 << rightShiftKeyBit, /* Not supported on Mac OS X.*/
  rightOptionKey                = 1 << rightOptionKeyBit, /* Not supported on Mac OS X.*/
  rightControlKey               = 1 << rightControlKeyBit /* Not supported on Mac OS X.*/
};

Note all right key are marked as not supported. I tried testing it on Leopard and the bit is never set.
Comment 10 Markus Keller CLA 2009-10-01 09:03:07 EDT
In HEAD, the Javadoc of Event#stateMask says: "The location mask is set when the same key event can be generated by different keys in the keyboard."

But the SWT.LOCATION_MASK bit that gets set in the Event#stateMask only describes the location of the key that has been pressed in that stroke, but not for the whole key event. E.g. in the keyDown events that are sent for Left_Ctrl+A, only the first event for key CTRL contains the location, but the one for key 'a' has the normal CTRL stateMask (and is thus indistinguishable from the second keyDown event for Right_Ctrl+A, although different keys have been pressed).

The Javadoc should explicitly mention that the location mask only applies to the key given in the keyCode field, but not to other modifiers in the stateMask. And this blurb should also be copied to the stateMask fields of all TypedEvents that are initialized from Event#stateMask.

Furthermore, I think it would be good to add an entry in the migration guide, since this may cause subtle problems in client code that e.g. assumes that the stateMask of a key event only contains modifiers (or mouse buttons).
Comment 11 Felipe Heidrich CLA 2009-10-01 09:33:22 EDT
(In reply to comment #10)
> In HEAD, the Javadoc of Event#stateMask says: "The location mask is set when
> the same key event can be generated by different keys in the keyboard."
> But the SWT.LOCATION_MASK bit that gets set in the Event#stateMask only
> describes the location of the key that has been pressed in that stroke, but not
> for the whole key event. E.g. in the keyDown events that are sent for
> Left_Ctrl+A, only the first event for key CTRL contains the location, but the
> one for key 'a' has the normal CTRL stateMask (and is thus indistinguishable
> from the second keyDown event for Right_Ctrl+A, although different keys have
> been pressed).

Yes, that is correct. A couple reasons for that:
- platform supported is limited
- we don't have more bits left for modifiers

> The Javadoc should explicitly mention that the location mask only applies to
> the key given in the keyCode field, but not to other modifiers in the
> stateMask. And this blurb should also be copied to the stateMask fields of all
> TypedEvents that are initialized from Event#stateMask.

Agreed and Agreed, but not all TypedEvents, only KeyEvent, Traversal, and Verify. MouseEvent nevers gets location set in it.

> Furthermore, I think it would be good to add an entry in the migration guide,
> since this may cause subtle problems in client code that e.g. assumes that the
> stateMask of a key event only contains modifiers (or mouse buttons).

Clients should already been using BUTTOM_MASK and MODIFIERS_MASK. I hope location mask doesn't break anyone.
I can add this to our release notes/new and worth. But we (swt) don't have a migration guide, do you know who takes care of it for the platform/eclipse ?

(Note, I had a bit of an emergency yesterday and had to leave before finishing to release the code, mac is not finished yet).
Comment 12 Markus Keller CLA 2009-10-01 10:49:15 EDT
(In reply to comment #11)
Sounds good.

> Clients should already been using BUTTOM_MASK and MODIFIERS_MASK. I hope
> location mask doesn't break anyone.

I quickly looked at our usages but didn't find any breakages so far. Patterns like "if ((event.stateMask & SWT.CTRL) != 0)" are safe. I will do a more thorough pass.

A pattern that will fail is "if (event.stateMask == 0)", which used to work fine for clients that want to process keyDowns but only if no modifier or mouse button is down. Such clients will miss modifiers and keys from the keypad. They should have written "if ((stateMask & SWT.MODIFIER_MASK & SWT.BUTTON_MASK) != 0)", but I could imagine there are some who didn't (although I don't know an example).


> I can add this to our release notes/new and worth. But we (swt) don't have a
> migration guide, do you know who takes care of it for the platform/eclipse ?

That's in /org.eclipse.platform.doc.isv/porting/3.6/incompatibilities.html . Use an entry from the 3.5 folder as template for the new entry. Everyone with access to the platform docs can release stuff there. I can do that if you don't have commit rights.
Comment 13 Felipe Heidrich CLA 2009-10-01 13:48:56 EDT
Fixed in HEAD > 20091001

Released the code for cocoa and did all could for carbon.
Improved the doc in Event.java and copied to to KeyEvent (TraverseEvent and VerifyEvent inherit from it).
Also changed Snippet25 to print location information:
http://dev.eclipse.org/viewcvs/index.cgi/~checkout~/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet25.java

Markus, I do not have commit rights for doc (never did).
Do you mind adding an entry in the migration guide for this ?
Do you need to send you the write up ? 

I'm no technical writer, but it would be something along these lines:

---
The stateMask during key events has been extended to include a location mask.
This change affects clients that compare stateMask to zero to determine if no modifiers and no buttons are down, or any modifier or any button is down.
The same result can be safely obtained by the following code:
int mask = SWT.MODIFIER_MASK | SWT.BUTTON_MASK;
boolean noModifierAndNoButton = (event.stateMask & mask) == 0;
boolean anyModifierOrAnyButton = (event.stateMask & mask) != 0;
---
Comment 14 Markus Keller CLA 2009-10-01 15:24:42 EDT
I've added the entry to the migration guide (expanding it a bit). Please check whether that's OK for you.

I would change the Javadoc of the two stateMask fields to this:

	/**
	 * depending on the event, the state of the keyboard modifier
	 * keys and mouse masks at the time the event was generated.
	 * The stateMask can also contain a location mask.
	 * <p>The location mask is set when the same key event can be 
	 * generated by different keys in the keyboard. For example,
	 * a key down event with the key code equal to <code>SWT.SHIFT</code>
	 * can be generated by the left or the right shift key on the keyboard.
	 * The location mask can only be used to determine the location 
	 * of the key code or character in the current event. It does not 
	 * include the location for modifier keys set in the modifiers 
	 * mask.</p>
	 * 
	 * @see org.eclipse.swt.SWT
	 * @see org.eclipse.swt.SWT#MODIFIER_MASK
	 * @see org.eclipse.swt.SWT#LOCATION_MASK
	 * @see org.eclipse.swt.SWT#BUTTON_MASK
	 */
	public int stateMask;
Comment 15 Markus Keller CLA 2009-10-02 07:17:02 EDT
I started with a more thorough pass, and I found a lot of code like this:

    if (event.character == SWT.DEL && event.stateMask == 0) { [..] }
or
    if (e.stateMask == SWT.MOD1 || e.stateMask == SWT.MOD2) { [..] }

I think this will bring us into big trouble, because clients need to inspect every reference to stateMask very carefully, and a lot of updates can be necessary to make code correct again.

Reopening to consider a non-breaking alternative. Couldn't you use another field that is not yet used by KeyEvent, e.g. Event#count or #x or #button, or else add another field for location?
Comment 16 Dani Megert CLA 2009-10-02 07:54:22 EDT
Agree with comment 15.
Comment 17 Felipe Heidrich CLA 2009-10-02 09:01:50 EDT
(In reply to comment #15)
> I think this will bring us into big trouble, because clients need to inspect
> every reference to stateMask very carefully, and a lot of updates can be
> necessary to make code correct again.

stateMask is a bit field, strictly speaking the code was never right.
Furthermore, the javadoc for MODIFIER_MASK and BUTTON_MASK states that new bits can be add in the future.


That said, I don't want to put anyone into trouble. I'll check with SSQ and see what can be done.
Comment 18 Felipe Heidrich CLA 2009-10-02 12:01:13 EDT
Fixed in HEAD > 20091002

See Event#keyLocation, KeyEvent#keyLocation

Markus, please feel free to remove the entry from the migration guide. Thanks.
Comment 19 Markus Keller CLA 2009-10-02 12:51:03 EDT
Thanks, that makes me sleep better;-). Reverted the migration guide.