Page MenuHome

Patch: Sticky Keys
AbandonedPublic

Authored by Julian Eisel (Severin) on Oct 22 2014, 12:04 AM.

Details

Summary

Patch: Sticky Keys

Differential Revision for sticky keys. Design feedback should go into T42339, please, don't add non-code feedback here. You can also find some testbuilds in there.

Note: Currently only backend implementation. I've decided to leave the pie integration out for now (a few things still need to be discussed for it and we better do that separately). If you want to use the Pie integration you need this patch and the updated ui_pie_menus_official.py

Implementation Notes

I've basically implemented the proposal I made in T41867. Some further comments for review:

  • I had to move the KM_CLICK/KM_HOLD logic so that it's called (almost) from the main loop because it needs a time presicion of a few milliseconds. I left the KM_DBL_CLICK logic in wm_event_add_ghostevent though. Not nice but I guess we have to deal with it.
  • I moved the Double Click UserPref options to a different place, as it's not mouse-only
  • After discussing it with @Antony Riakiotakis (psy-fi), I changed all occurrences of KM_CLICK to KM_PRESS as this seems to not bring any conflicts
  • The additional events sent by the clicktest brought up some glitches that I had to fix individually
  • Occurrences of event->val != KM_PRESS had to be changed to "event->val == KM_RELEASE" but that should be fine because now the only values that may be sent on a key event are KM_PRESS or KM_RELEASE
  • I found a few minor things that could be cleaned up and marked them as XXXs (all off-topic) - will have a look at them after this is committed (to avoid merge conflicts)

Diff Detail

Repository
rB Blender
Branch
arcpatch-D840_1

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Although I cannot really speak for the implementation, there's a few things that bother me. So, have my opinion :)

source/blender/editors/interface/resources.c
2494

There's already such a check a few lines above, don't think it's needed to duplicate.

source/blender/makesdna/DNA_userdef_types.h
529

This is an unnecessary change, rename the one you added.

source/blender/makesdna/DNA_windowmanager_types.h
240

Not sure, but this feels unnecessary as well? It was pretty obvious what val was used for, else we could also list all the defines here... :)

source/blender/windowmanager/WM_types.h
428–430

These could be click_type and click_time?

source/blender/windowmanager/intern/wm_window.c
1107–1114

Inconsistent use of braces.

Julian Eisel (Severin) updated this object.EditedDec 24 2014, 7:18 PM
Julian Eisel (Severin) updated this revision to Diff 3034.

Partially rewritten the patch

New testbuilds are available in T42339

Julian Eisel (Severin) updated this revision to Diff 3192.

Should fix the following issues:

Testing patch against ecc58da8f1d110498e700b804cb44adba1145113, nested pies are not working on click. Nested pies work with swipe and numerical input, but not on click.

http://cl.ly/ZM9u

Antony Riakiotakis (psy-fi) requested changes to this revision.

Some preliminary review. I will do more in depth review with the context around the code later. Still haven't tried the branch.

source/blender/editors/armature/armature_ops.c
273

Why do you need to change this? Generally, doing stuff on button release when not in a modal operator sounds like trouble, ie user can have pressed buttons from modal operation of previous operator.

source/blender/editors/curve/curve_ops.c
234

Same here.

source/blender/editors/interface/interface_handlers.c
9100

Why is this needed? Mousemove will act after next runs of the handler.

source/blender/editors/mesh/mesh_ops.c
406

Same as above (click vs release)

source/blender/editors/screen/screen_ops.c
4115 ↗(On Diff #3192)

Maybe KM_ANY should implicitly reject some click types?

source/blender/editors/space_node/node_relationships.c
636

Better use the define here to make code more readable?

source/blender/windowmanager/WM_types.h
426

Feel free remove it :)

source/blender/windowmanager/intern/wm_event_system.c
3008

Hmmm...you could just change a few lines here instead of moving the whole function.

3168

Keep the feedback, you never know when you'll need it. I would add a click info print in addition here even.

3285

Are those redundant? (code review ommits the code around the diff making things difficult)

source/blender/windowmanager/intern/wm_operators.c
3447

Can you give an example?

source/blender/windowmanager/intern/wm_window.c
1118

If disabled above this will reenable, is this normal?

This revision now requires changes to proceed.Feb 2 2015, 4:17 PM

Thanks @Antony Riakiotakis (psy-fi) for the first review. I'm working on another update that already includes some of the changes you suggested, but I'd like to give it a last haircut first.

source/blender/editors/armature/armature_ops.c
273

Don't quite get what you mean, can you give an example? I tested each case carefully and everythings seems to work fine.

The reason why I changed those is because KM_CLICK is now used as the counterpart of KM_HOLD (as you may know), so I smell bug reports like "Shortcut doesn't work if I keep holding it for a sec" ;P In case this does bring any issues I don't see yet, we can either leave this set to KM_CLICK and hope that we won't get many reports or we use KM_CLICK like it's used currently and add something like KM_TAP for the clicktype test (or we write another hack that satisfies all needs :S).

source/blender/editors/interface/interface_handlers.c
9100

Yep, noticed that as well. It's on my Todo list for the final cleanup.

source/blender/editors/screen/screen_ops.c
4115 ↗(On Diff #3192)

Good idea!

source/blender/editors/space_node/node_relationships.c
636

You mean like if (event->val != KM_NOTHING) ?

source/blender/windowmanager/intern/wm_event_system.c
3008

I'd even prefer to move this logic to wm_clicktype_set (the one below), cause I don't really see a need for having this being an own function. I can also move it back to it's old place, this was just done for some hacking (like using wm_event_add_mousemove from above). It's also on the final cleanup-todo-list ;)

3168

Hehe, I removed it out of laziness but forgot to add again. I already did so in the current state of the patch though

3285

The check above is for the mouse only, this one is keyboard only.

source/blender/windowmanager/intern/wm_operators.c
3447

Don't remember one to be honest. Would have to check again (and chances are it doesn't conflict anymore)

source/blender/windowmanager/intern/wm_window.c
1118

This is exactly what I want. If the above check returns true, this one does as well, so the time is reset again for the non-modifier key. We could also pack this into one giant if (...), but I think this is more readable. I should add a comment to the above check though.

source/blender/editors/armature/armature_ops.c
273

Then how about KM_PRESS instead? Another solution might be to allow fallback to KM_CLICK if KM_HOLD doesn't find any operators to fire.

source/blender/editors/space_node/node_relationships.c
636

good point, leave as is.

Julian Eisel (Severin) retitled this revision from Patch: Sticky Keys (+ Integration Into Pies) to Patch: Sticky Keys.Feb 12 2015, 7:19 PM
Julian Eisel (Severin) updated this object.
Julian Eisel (Severin) edited edge metadata.
Julian Eisel (Severin) updated this revision to Diff 3490.

Notes...
KM_CLICK isn't always replaceable with KM_PRESS - The changes to the keymap break lasso select in Editmode.

Where Ctrl+Click - adds a new vertex, but Ctrl+LMBDrag selects.

Julian Eisel (Severin) edited edge metadata.EditedFeb 12 2015, 10:36 PM
Julian Eisel (Severin) updated this revision to Diff 3498.

Fix issue pointed out by @Campbell Barton (campbellbarton)

@Campbell Barton (campbellbarton) and me discussed/brainstormed about the current state of the patch a bit more, so here's what we came up with:

There are currently two "major" issues:

  • Add-ons can not easily create own Sticky Keys, they only can set the key value of the operator they're adding to Click or Hold, but they can't modify the value of the operator that is already assinged to that shortcut. An example to describe this a bit better: The Pie Menu addon can assign the Mode selection Pie to Hold Tab, but it can't change the Toggle Edit-mode operator to use Click, as it's needed to make this a working Sticky Key. This isn't a problem with this patch, it's because of how our keymap system works: The Add-on keymaps are created before the default C-coded keymap is created.The only way to work around this currently is by either changing the keymap from the C-core, or by letting the user do the changes manually.
  • To create custom Sticky Keys, the user has to search for both operators he wants to use with the Sticky Keys and has to change the values of them manually. This is far away from a smart UX, so we need to find a way to make this smarter.

We were brainstorming about ways that could solve both issues, but some deeper investigation is needed to see what's tangible code-wise and what is acceptable UX-wise.

Our current proposal is to merge the core functionality of Sticky Keys (this patch) into master, and ship a few Pies that use Sticky Keys by default (with C-coded keymap changes). My personal candidates would be the Mode Pie (Tab key), the Shade Pie (Z key) and maybe the Manipulator Pie (Spacebar). It will still be possible to create custom Stickies, but we have to communicate well on how they are created correctly.

I tried to describe everything in a understandable way which is kinda hard, but feel free to ask ;)

Update to sync with 2.74, also use doxy comments

remove orig files (added accidentally)

Checked over patch again, didn't find any big problems, noted minor suggested changes.

source/blender/windowmanager/intern/wm_window.c
1103

term test is associated with checking (non-destructive operation).

Would call wm_window_event_clicktype_detect or wm_window_event_clicktype_init

1114–1153

Since this runs on every event, (while not a performance bottle-neck), we could early-exit .

since this only depends on KM_PRESS / KM_RELEASE / event->is_key_pressed state.

1146

Can use KM_NOTHING

Addressing review points, fix issue on some X11 systems

  • minor edits
  • rename vars
source/blender/editors/space_view3d/view3d_ops.c
389

This makes a noticeable difference, is it needed? (some users might complain?), is this needed?

Thanks so much for the work here guys.

Add-ons can not easily create own Sticky Keys, they only can set the key value of the operator they're adding to Click or Hold, but they can't modify the value of the operator that is already assinged to that shortcut. An example to describe this a bit better: The Pie Menu addon can assign the Mode selection Pie to Hold Tab, but it can't change the Toggle Edit-mode operator to use Click, as it's needed to make this a working Sticky Key. This isn't a problem with this patch, it's because of how our keymap system works: The Add-on keymaps are created before the default C-coded keymap is created.The only way to work around this currently is by either changing the keymap from the C-core, or by letting the user do the changes manually.
To create custom Sticky Keys, the user has to search for both operators he wants to use with the Sticky Keys and has to change the values of them manually. This is far away from a smart UX, so we need to find a way to make this smarter.

What was the decision regarding these issues?

@Paweł Łyczkowski (plyczkowski), for now addons still can't create stickies without doing really bad hacks or loading changed keymaps. The only way to create stickies now is by changing it in the keymap file or through the UI. We need to find a better way to do this, but we decided to go step by step (and "we" is mainly Cambo and me since we didn't get much feedback on this)