Not all keyboard shortcuts work

[GoogleCodeExporter]

We used to tie each tool to a number key but that was changed to letter keys.  
However, not all keyboard shortcuts work now (E does not work for Ellipse, R 
does not work for Rectangle).  I'm on OSX.  This should be fixed before 2.6 is 
released - or we should revert back to the number keys (my preference)

Original issue reported on code.google.com by codedr...@gmail.com on 17 Mar 2012 at 5:15

[GoogleCodeExporter]

I think the current keyboard shortcuts are more in line with desktop software, 
preserving muscle memory and they are easier to reach.

The problem in the current trunk has to do with the flyout exception, it is 
coercing an array into a string (not sure if there is any reason) and thus the 
context for the keydown is invalid (it is passing "R,true". I'm submitting a 
patch for this.

Original comment by duopi...@gmail.com on 17 Mar 2012 at 7:12

Attachments:

• keyboard_select_ellipse_and_rect.diff

[GoogleCodeExporter]

Thanks for the patch, duopixel! :)

Merged in r2063.

This is certainly better than the current broken approach, but it would be 
great if the tool remembered which sub-tool you are on (the rectangle, the 
square) instead of going back to [0] all the time.  I'll leave this open in 
case you can also fix that :)

Original comment by codedr...@gmail.com on 17 Mar 2012 at 8:17

[GoogleCodeExporter]

Here is a patch that solves the secondary issue.

Original comment by duopi...@gmail.com on 21 Mar 2012 at 8:47

Attachments:

• tools_selection.diff

[GoogleCodeExporter]

It's a little buggy. It works the first time. Then no matter what I do it keeps 
changing back to Circle.

To reproduce (on Chrome 17):
1. Select Circle tool
2. Press R to change to rect
3. Press E to change to circle
4. Change to free hand circle tool
5. Press E and it will change back to circle, and you can no longer select 
Ellipse tool with the mouse

--
As a side note, you can use jQuery's .data('corupt') to get what you want.

Original comment by asyazwan on 22 Mar 2012 at 2:39

[GoogleCodeExporter]

Sorry about that. This new patch should work. Plus it also implements issue 462 
(shift + key to cycle through tools).

I'm not sure why it doesn't work for me with .data('curopt'), but 
.attr('data-curopt') works without any problem. 

Original comment by duopi...@gmail.com on 22 Mar 2012 at 8:54

Attachments:

• tools_selection.diff

[GoogleCodeExporter]

Thanks a lot duopixel!

A minor nitpick though (bear with me!):
- your patches always seem to have whitespace issue. View in browser and try to 
highlight, you will see other lines use tabs but your lines are mixed with 
spaces. The indentation level is also having 2 extra spaces. In the offchance 
you're using vim (or any editors having similar config name), checkout 
"noexpandtab", "tabstop", "shiftwidth" options.

- if(event == null) event = false; - this line is unnecessary. null evaluates 
to false anyway, if you need to use it. In your case you don't even use it. In 
fact if event IS indeed null, you will get an error because the following line 
calls event.type == "keydown" which will evaluate to false.type and throw an 
error. You will never have this error though, because events will ALWAYS get 
passed as first parameter to function, no matter you do it explicitly or not. 
So yes, this brings us to

- your last change at function(e) { func(e) } - is the same as your previous 
.mouseup(func). func() will still get the event as first parameter no matter 
you explicitly pass it or not

Those are what I thought of when reading your patch to understand what you've 
done, the rest seems okay though. I have edited your patch that address above 
points. Seems to work fine on my Chrome 17 and FF 11. It's in r2067. Thanks 
again!

--
About .data(), I tried it during run-time. From 
http://api.jquery.com/jQuery.data/ : Regarding HTML5 data-* attributes: This 
low-level method does NOT retrieve the data-* attributes unless the more 
convenient .data() method has already retrieved them.

That's probably why... I guess.

Original comment by asyazwan on 22 Mar 2012 at 10:31

• Changed state: Fixed

[GoogleCodeExporter]

Thanks for the pointers, I'm not used to collaborating code, so I can get 
things done but it's often quite ugly. I'll take note and thanks for the code 
review and cleanup.

Original comment by duopi...@gmail.com on 22 Mar 2012 at 4:17

[GoogleCodeExporter]

Thanks for the patches duopixel and thanks for the thorough review asyazwan! :)

Original comment by codedr...@gmail.com on 22 Mar 2012 at 4:42