KeyPress fix #170

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@baalhiverne
Contributor

baalhiverne commented Dec 14, 2012

keypress now:

  • Understands key modifiers (alt ctrl shift)
  • Uses Shoes v3 naming convention
  • yields symbols when key modifiers and/or special keys
keypress now function as expected
keypress now:
- Understands key modifiers (alt ctrl shift)
- Uses Shoes v3 naming convention
- yields symbols when key modifiers and/or special keys
@PragTob

This comment has been minimized.

Show comment Hide comment
@PragTob

PragTob Dec 14, 2012

Owner

Yay man! Thank you!
Will take a look at it tomorrow (too late now) and then think about testing and stuff... :-)

Owner

PragTob commented Dec 14, 2012

Yay man! Thank you!
Will take a look at it tomorrow (too late now) and then think about testing and stuff... :-)

@ashbb

This comment has been minimized.

Show comment Hide comment
@ashbb

ashbb Dec 16, 2012

Member

@baalhiverne I confirmed this tiny snippet with your baalhiverne:master on my Windows 7.

Shoes.app do
  keypress do |k|
    p k
  end
end

And got the following.

"a"                            #=> a
:""                            #=> Shift
"A"                            #=> Shift + a
:""                            #=> Ctrl
:"control_\u0001"              #=> Ctrl + a
:""                            #=> Alt
:alt_a                         #=> Alt + a
:""                            #=> Ctrl
:control_                      #=> Ctrl + Shift
:"control_shift_\u0001"        #=> Ctrl + Shift + a
:"\n"                          #=> Ebter
:delete                        #=> Delete
:""                            #=> Shift
"!"                            #=> Shift + 1
:backspace                     #=> Backspce
:escape                        #=> Esc

Great work, thanks!

But if we keep compatibility with Shoes 3, keypress doesn't return anything when just only modifier keys are pressed.

Member

ashbb commented Dec 16, 2012

@baalhiverne I confirmed this tiny snippet with your baalhiverne:master on my Windows 7.

Shoes.app do
  keypress do |k|
    p k
  end
end

And got the following.

"a"                            #=> a
:""                            #=> Shift
"A"                            #=> Shift + a
:""                            #=> Ctrl
:"control_\u0001"              #=> Ctrl + a
:""                            #=> Alt
:alt_a                         #=> Alt + a
:""                            #=> Ctrl
:control_                      #=> Ctrl + Shift
:"control_shift_\u0001"        #=> Ctrl + Shift + a
:"\n"                          #=> Ebter
:delete                        #=> Delete
:""                            #=> Shift
"!"                            #=> Shift + 1
:backspace                     #=> Backspce
:escape                        #=> Esc

Great work, thanks!

But if we keep compatibility with Shoes 3, keypress doesn't return anything when just only modifier keys are pressed.

@PragTob

This comment has been minimized.

Show comment Hide comment
@PragTob

PragTob Dec 16, 2012

If I understand this correctly than this and the following code block could also be a class of it's own that inherits from :Swt::KeyListener - right?
So like

module Shoes
  module Swt
    class ShoesKeyListener < ::Swt::KeyListener
      # bla
     end
  end
end

If so I'd prefer that and I would like for it to be in it's own file :-)

If I understand this correctly than this and the following code block could also be a class of it's own that inherits from :Swt::KeyListener - right?
So like

module Shoes
  module Swt
    class ShoesKeyListener < ::Swt::KeyListener
      # bla
     end
  end
end

If so I'd prefer that and I would like for it to be in it's own file :-)
@PragTob

This comment has been minimized.

Show comment Hide comment
@PragTob

PragTob Dec 16, 2012

Owner

Thanks for looking at it @ashbb so there still seems to be a problem we should fix with control + a etc. and ignoring just modifier keys.

About testing if I understand the code correctly we could just stub the argument - e in this case. so kind of like:

key = mock(:key, keyCode: 55, stateMask: WHATEVER_THE_STATE_MASK_LOOKS_LIKE)

Then call the method and see if the correct result is returned. That should be done for some major key combinations, like the ones ashbb tested - I'm happy to help you with that if you like :-)
I don't know if it'd be possible to simulate real key presses somehow... hmm maybe I'll look into that another time.

One other thing, I'm not quite sure but I think we settled with calling all methods ruby style (key_code instead of keyCode) ( @wasnotrice @ashbb can you confirm?).

Thank you very much for your work on this @baalhiverne <3
Tobi

Owner

PragTob commented Dec 16, 2012

Thanks for looking at it @ashbb so there still seems to be a problem we should fix with control + a etc. and ignoring just modifier keys.

About testing if I understand the code correctly we could just stub the argument - e in this case. so kind of like:

key = mock(:key, keyCode: 55, stateMask: WHATEVER_THE_STATE_MASK_LOOKS_LIKE)

Then call the method and see if the correct result is returned. That should be done for some major key combinations, like the ones ashbb tested - I'm happy to help you with that if you like :-)
I don't know if it'd be possible to simulate real key presses somehow... hmm maybe I'll look into that another time.

One other thing, I'm not quite sure but I think we settled with calling all methods ruby style (key_code instead of keyCode) ( @wasnotrice @ashbb can you confirm?).

Thank you very much for your work on this @baalhiverne <3
Tobi

@davorb

This comment has been minimized.

Show comment Hide comment
@davorb

davorb Dec 29, 2012

Member

Awesome work @baalhiverne !

Could you just change some of the methods to use snake_case, like instead of stateMask change it to state_mask and so on. That would be great! Thanks!

Member

davorb commented Dec 29, 2012

Awesome work @baalhiverne !

Could you just change some of the methods to use snake_case, like instead of stateMask change it to state_mask and so on. That would be great! Thanks!

@ghost ghost assigned PragTob Jun 17, 2013

@PragTob

This comment has been minimized.

Show comment Hide comment
@PragTob

PragTob Jun 17, 2013

Owner

just FYI I'm working on this on my fixKeyPress Branch: https://github.com/PragTob/shoes4/tree/keypressFix

Some bugs still remain.. for now shift + left_arrow doesn't give shift_left for instance but so far I got this nicely specced out. So this is close to opening a PR and getting merge. Hope I get it done today but gotta run now.

Cheers,
Tobi

Owner

PragTob commented Jun 17, 2013

just FYI I'm working on this on my fixKeyPress Branch: https://github.com/PragTob/shoes4/tree/keypressFix

Some bugs still remain.. for now shift + left_arrow doesn't give shift_left for instance but so far I got this nicely specced out. So this is close to opening a PR and getting merge. Hope I get it done today but gotta run now.

Cheers,
Tobi

@PragTob

This comment has been minimized.

Show comment Hide comment
@PragTob

PragTob Jun 17, 2013

Owner

Work can be seen in #279 so I will close this PR here and we can move the discussion there.

Owner

PragTob commented Jun 17, 2013

Work can be seen in #279 so I will close this PR here and we can move the discussion there.

@PragTob PragTob closed this Jun 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment