Page MenuHome
Differential D840

Patch: Sticky Keys
AbandonedPublic
Actions

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

Details

Reviewers
Campbell Barton (campbellbarton)
Antonis Ryakiotakis (psy-fi)
Maniphest Tasks
T42339: Design: Sticky Keys (+ Integration Into Pies)
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 @Antonis Ryakiotakis (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
Julian Eisel (Severin) updated this revision to Diff 2744.Oct 22 2014, 12:04 AM
Julian Eisel (Severin) updated this object.
Julian Eisel (Severin) added reviewers: Antonis Ryakiotakis (psy-fi), Campbell Barton (campbellbarton).
Julian Eisel (Severin) set the repository for this revision to rB Blender.
Julian Eisel (Severin) added a project: User Interface.
Julian Eisel (Severin) mentioned this in T42339: Design: Sticky Keys (+ Integration Into Pies).
Julian Eisel (Severin) added a subscriber: Sean Olson (liquidape).
Julian Eisel (Severin) added a task: T42339: Design: Sticky Keys (+ Integration Into Pies).Oct 22 2014, 12:07 AM
Kévin Dietrich (kevindietrich) added a subscriber: Kévin Dietrich (kevindietrich).Oct 22 2014, 12:18 PM

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.

TARDISMaker added a subscriber: TARDISMaker.Oct 22 2014, 8:37 PM
Paweł Łyczkowski (plyczkowski) added a subscriber: Paweł Łyczkowski (plyczkowski).Dec 1 2014, 11:11 AM
Julian Eisel (Severin) updated this revision to Diff 3034.EditedDec 24 2014, 7:18 PM
Julian Eisel (Severin) updated this object.

Partially rewritten the patch

New testbuilds are available in T42339

Julian Eisel (Severin) updated this object.Dec 24 2014, 7:27 PM
Julian Eisel (Severin) updated this revision to Diff 3192.Jan 18 2015, 2:25 PM
Julian Eisel (Severin) added subscribers: Todor Imreorov (blurymind), Sebastian Koenig (sebastian_k).

Should fix the following issues:

Jonathan Williamson (carter2422) added a subscriber: Jonathan Williamson (carter2422).Jan 18 2015, 4:19 PM

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

Antonis Ryakiotakis (psy-fi) requested changes to this revision.Feb 2 2015, 4:17 PM
Antonis Ryakiotakis (psy-fi) edited edge metadata.

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
Julian Eisel (Severin) added a comment.Feb 2 2015, 5:37 PM

Thanks @Antonis Ryakiotakis (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.

Antonis Ryakiotakis (psy-fi) added inline comments.Feb 2 2015, 6:45 PM
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) updated this revision to Diff 3490.Feb 12 2015, 7:19 PM
Julian Eisel (Severin) retitled this revision from Patch: Sticky Keys (+ Integration Into Pies) to Patch: Sticky Keys.
Julian Eisel (Severin) updated this object.
Julian Eisel (Severin) edited edge metadata.
Campbell Barton (campbellbarton) edited edge metadata.Feb 12 2015, 9:27 PM

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) updated this revision to Diff 3498.EditedFeb 12 2015, 10:36 PM
Julian Eisel (Severin) edited edge metadata.

Fix issue pointed out by @Campbell Barton (campbellbarton)

Julian Eisel (Severin) added a comment.Feb 13 2015, 12:33 AM

@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 ;)

Campbell Barton (campbellbarton) commandeered this revision.Apr 2 2015, 8:08 AM
Campbell Barton (campbellbarton) edited reviewers, added: Julian Eisel (Severin); removed: Campbell Barton (campbellbarton).
Campbell Barton (campbellbarton) updated this revision to Diff 3887.Apr 2 2015, 8:10 AM

Update to sync with 2.74, also use doxy comments

Campbell Barton (campbellbarton) updated this revision to Diff 3888.Apr 2 2015, 8:13 AM

remove orig files (added accidentally)

Campbell Barton (campbellbarton) updated this revision to Diff 3889.Apr 2 2015, 8:15 AM

remove orig files

Campbell Barton (campbellbarton) updated this revision to Diff 3890.Apr 2 2015, 8:23 AM

use KM_NOTHING

Campbell Barton (campbellbarton) added a comment.Apr 2 2015, 9:13 AM

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

Julian Eisel (Severin) commandeered this revision.Apr 2 2015, 10:45 PM
Julian Eisel (Severin) edited reviewers, added: Campbell Barton (campbellbarton); removed: Julian Eisel (Severin).
Julian Eisel (Severin) updated this revision to Diff 3896.Apr 2 2015, 10:49 PM

Addressing review points, fix issue on some X11 systems

Campbell Barton (campbellbarton) commandeered this revision.Apr 3 2015, 3:23 AM
Campbell Barton (campbellbarton) edited reviewers, added: Julian Eisel (Severin); removed: Campbell Barton (campbellbarton).
Campbell Barton (campbellbarton) updated this revision to Diff 3898.Apr 3 2015, 3:25 AM
  • minor edits
  • rename vars
Campbell Barton (campbellbarton) added inline comments.Apr 3 2015, 3:30 AM
source/blender/editors/space_view3d/view3d_ops.c
389

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

Julian Eisel (Severin) mentioned this in rB53a3850a8a05: Sticky Keys backend.Apr 3 2015, 4:22 PM
Paweł Łyczkowski (plyczkowski) added a comment.Apr 3 2015, 4:48 PM

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?

Julian Eisel (Severin) edited edge metadata.Apr 3 2015, 4:53 PM

@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)

Julian Eisel (Severin) commandeered this revision.Apr 3 2015, 5:20 PM
Julian Eisel (Severin) abandoned this revision.
Julian Eisel (Severin) edited reviewers, added: Campbell Barton (campbellbarton); removed: Julian Eisel (Severin).

Committed rB53a3850a8a0524994

Julian Eisel (Severin) mentioned this in rBS53a3850a8a05: Sticky Keys backend.Apr 7 2015, 3:58 AM
Greg Zaal (gregzaal) mentioned this in T52659: Mute node links (design task).Sep 6 2017, 9:47 AM

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
13 lines
source/
blender/
blenkernel/
2 lines
editors/
armature/
2 lines
curve/
2 lines
gpencil/
2 lines
interface/
4 lines
67 lines
2 lines
4 lines
7 lines
mesh/
2 lines
object/
2 lines
space_console/
6 lines
space_file/
16 lines
space_graph/
2 lines
space_node/
13 lines
space_outliner/
8 lines
space_text/
11 lines
space_view3d/
2 lines
makesdna/
7 lines
3 lines
makesrna/
intern/
8 lines
2 lines
windowmanager/
17 lines
intern/
134 lines
4 lines
2 lines
76 lines

Diff 3898

View Options

release/scripts/startup/bl_ui/space_userpref.py

Loading...
View Options

source/blender/blenkernel/BKE_blender.h

Loading...
View Options

source/blender/editors/armature/armature_ops.c

Loading...
View Options

source/blender/editors/curve/curve_ops.c

Loading...
View Options

source/blender/editors/gpencil/gpencil_paint.c

Loading...
View Options

source/blender/editors/interface/interface_eyedropper.c

Loading...
View Options

source/blender/editors/interface/interface_handlers.c

Loading...
View Options

source/blender/editors/interface/interface_panel.c

Loading...
View Options

source/blender/editors/interface/interface_regions.c

Loading...
View Options

source/blender/editors/interface/resources.c

Loading...
View Options

source/blender/editors/mesh/mesh_ops.c

Loading...
View Options

source/blender/editors/object/object_ops.c

Loading...
View Options

source/blender/editors/space_console/console_ops.c

Loading...
View Options

source/blender/editors/space_file/space_file.c

Loading...
View Options

source/blender/editors/space_graph/graph_ops.c

Loading...
View Options

source/blender/editors/space_node/node_relationships.c

Loading...
View Options

source/blender/editors/space_outliner/outliner_ops.c

Loading...
View Options

source/blender/editors/space_text/text_ops.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_ops.c

Loading...
View Options

source/blender/makesdna/DNA_userdef_types.h

Loading...
View Options

source/blender/makesdna/DNA_windowmanager_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_userdef.c

Loading...
View Options

source/blender/makesrna/intern/rna_wm.c

Loading...
View Options

source/blender/windowmanager/WM_types.h

Loading...
View Options

source/blender/windowmanager/intern/wm_event_system.c

Loading...
View Options

source/blender/windowmanager/intern/wm_keymap.c

Loading...
View Options

source/blender/windowmanager/intern/wm_operators.c

Loading...
View Options

source/blender/windowmanager/intern/wm_window.c

Loading...