Cleanup: Refactor VSE selection tools keymap #121846

Richard Antalik · 2024-05-16T02:33:13+02:00

Richard Antalik commented

2024-05-16 02:33:13 +02:00

Split single keymap definition into timeline and preview as well as LCS
and RCS definitions.

This improves readability of keymap code. No changes on user level.

Split single keymap definition into timeline and preview as well as LCS and RCS definitions. This improves readability of keymap code. No changes on user level.

Richard Antalik added 1 commit 2024-05-16 02:33:25 +02:00

Cleanup: Refactor VSE selection tools keymap 40f6b123b1

Split single keymap definition into timeline and preview as well as LCS
and RCS definitions.

Richard Antalik added 1 commit 2024-05-16 02:34:12 +02:00

Remove cursor set from timeline, fix missing solo preview 1220fe0600

Richard Antalik requested review from Sergey Sharybin 2024-05-16 02:37:43 +02:00

Sergey Sharybin requested changes 2024-05-17 14:04:21 +02:00

Dismissed

Sergey Sharybin left a comment

This improves readability of keymap.

Readability in code? In the keymap editor?
What are the expected implications for users?

Applying this patch against the current main seems to replace RMS with LMS, and also have the following Python error in the console:

Traceback (most recent call last):
  File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bpy/utils/__init__.py", line 711, in keyconfig_set
    execfile(filepath)
  File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bpy/utils/__init__.py", line 110, in execfile
    mod_spec.loader.exec_module(mod)
  File "<frozen importlib._bootstrap_external>", line 940, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/presets/keyconfig/blender.py", line 383, in <module>
    load()
  File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/presets/keyconfig/blender.py", line 376, in load
    keyconfig_data = keyconfig_data_oskey_from_ctrl_for_macos(keyconfig_data)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bl_keymap_utils/platform_helpers.py", line 50, in keyconfig_data_oskey_from_ctrl_for_macos
    return keyconfig_data_oskey_from_ctrl(keyconfig_data_src, filter_fn=filter_fn)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bl_keymap_utils/platform_helpers.py", line 13, in keyconfig_data_oskey_from_ctrl
    item_op, item_event, item_prop = item_src
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: not enough values to unpack (expected 3, got 0)

> This improves readability of keymap. Readability in code? In the keymap editor? What are the expected implications for users? --- Applying this patch against the current main seems to replace RMS with LMS, and also have the following Python error in the console: ``` Traceback (most recent call last): File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bpy/utils/__init__.py", line 711, in keyconfig_set execfile(filepath) File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bpy/utils/__init__.py", line 110, in execfile mod_spec.loader.exec_module(mod) File "<frozen importlib._bootstrap_external>", line 940, in exec_module File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/presets/keyconfig/blender.py", line 383, in <module> load() File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/presets/keyconfig/blender.py", line 376, in load keyconfig_data = keyconfig_data_oskey_from_ctrl_for_macos(keyconfig_data) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bl_keymap_utils/platform_helpers.py", line 50, in keyconfig_data_oskey_from_ctrl_for_macos return keyconfig_data_oskey_from_ctrl(keyconfig_data_src, filter_fn=filter_fn) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/sergey/Developer/bf/build/release/bin/Blender.app/Contents/Resources/4.2/scripts/modules/bl_keymap_utils/platform_helpers.py", line 13, in keyconfig_data_oskey_from_ctrl item_op, item_event, item_prop = item_src ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ValueError: not enough values to unpack (expected 3, got 0) ```

scripts/presets/keyconfig/keymap_data/blender_default.py

						
				@ -8719,1 +8719,3 @@

				def km_sequencer_editor_tool_generic_select(params, *, fallback):

				def km_sequencer_editor_tool_generic_select_timeline_rcs(params, fallback):

				    return [

				        ("sequencer.select", {"type": 'LEFTMOUSE', "value": 'PRESS'},

Sergey Sharybin commented

2024-05-17 14:00:03 +02:00

Is it indeed intended to be LEFTMOUSE in RCS codepath?

Is it indeed intended to be `LEFTMOUSE` in RCS codepath?

Richard Antalik commented

2024-05-19 13:58:25 +02:00

No, this was meant to be implemented by #109522, but forgot to remove this item.

iss marked this conversation as resolved

scripts/presets/keyconfig/keymap_data/blender_default.py

						
				@ -8720,0 +8719,4 @@

				def km_sequencer_editor_tool_generic_select_timeline_rcs(params, fallback):

				    return [

				        ("sequencer.select", {"type": 'LEFTMOUSE', "value": 'PRESS'},

				         {"properties": [("handles_only", True), ("wait_to_deselect_others", True)]}),

Sergey Sharybin commented

2024-05-17 14:01:42 +02:00

I am not sure where the handles_only and wait_to_deselect_others are coming from. They were not used before this patch. I am not sure why splitting keymaps for preview and timeline need to change properties of keymap items.

I am not sure where the `handles_only` and `wait_to_deselect_others` are coming from. They were not used before this patch. I am not sure why splitting keymaps for preview and timeline need to change properties of keymap items.

Richard Antalik commented

2024-05-19 13:59:28 +02:00

Same item as above, so mistakenly left there from #109522, will remove this.

iss marked this conversation as resolved

Richard Antalik added 1 commit 2024-05-19 14:02:31 +02:00

Remove keymap change from future f6f3959d28

Richard Antalik commented

2024-05-19 15:05:11 +02:00

This improves readability of keymap.

Readability in code? In the keymap editor?
What are the expected implications for users?

Applying this patch against the current main seems to replace RMS with LMS, and also have the following Python error in the console:

I was able to reproduce this with RCS keymap, will check why this happens.

> > This improves readability of keymap. > > Readability in code? In the keymap editor? > What are the expected implications for users? > > --- > > Applying this patch against the current main seems to replace RMS with LMS, and also have the following Python error in the console: I was able to reproduce this with RCS keymap, will check why this happens.

Richard Antalik added 1 commit 2024-05-19 15:27:02 +02:00

Fix unpacking condition

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

0d4b0420ac

Richard Antalik commented

2024-05-19 15:34:31 +02:00

@Sergey I have fixed the issue with python error, at least it does not show up in console. In linux I got error in keymap_init_from_data and looking at the code it is still unpacking a tuple, so I am not sure if _template_items_tool_select() can return empty one if cursor_operator is None is true. Would have to look at this later still, since I am quite tired.

@Sergey I have fixed the issue with python error, at least it does not show up in console. In linux I got error in `keymap_init_from_data` and looking at the code it is still unpacking a tuple, so I am not sure if `_template_items_tool_select()` can return empty one if `cursor_operator is None` is true. Would have to look at this later still, since I am quite tired.

Richard Antalik commented

2024-05-21 17:19:36 +02:00

Just a note, I have checked the keymap, and it looks OK, so this issue should be resolved, i.e. there is no dangling empty tupple/list.

Sergey Sharybin commented

2024-05-21 18:08:06 +02:00

While the default keymap appears to be working fine, the Industry Compatible keymap does no longer work as expected: it looses the box selection behavior on tweak. I did not check if the Blender 2.7 keymap has any regressions.

Which means, there are technically changes on the user level, as people who create and share keymaps needs to adjust to it. Is there a way to avoid such breaking change?

@dfelinto What is our policy now about keymap? Can we be implementing changes which we find needed to more efficiently move forward, but which break keymaps shared on the extensions platform?

While the default keymap appears to be working fine, the `Industry Compatible` keymap does no longer work as expected: it looses the box selection behavior on tweak. I did not check if the `Blender 2.7` keymap has any regressions. Which means, there are technically changes on the user level, as people who create and share keymaps needs to adjust to it. Is there a way to avoid such breaking change? @dfelinto What is our policy now about keymap? Can we be implementing changes which we find needed to more efficiently move forward, but which break keymaps shared on the extensions platform?

Richard Antalik commented

2024-05-22 11:48:21 +02:00

While the default keymap appears to be working fine, the Industry Compatible keymap does no longer work as expected: it looses the box selection behavior on tweak. I did not check if the Blender 2.7 keymap has any regressions.

Ah I did not know how these keymaps are generated or managed.

Which means, there are technically changes on the user level, as people who create and share keymaps needs to adjust to it. Is there a way to avoid such breaking change?

Hmm I see that we have versioning for keymap, not sure if this goes through custom keymaps. My understanding was that custom keymaps are not updated and we don't provide support for them... Will see, if this can be resolved with versioning.

> While the default keymap appears to be working fine, the `Industry Compatible` keymap does no longer work as expected: it looses the box selection behavior on tweak. I did not check if the `Blender 2.7` keymap has any regressions. Ah I did not know how these keymaps are generated or managed. > Which means, there are technically changes on the user level, as people who create and share keymaps needs to adjust to it. Is there a way to avoid such breaking change? Hmm I see that we have versioning for keymap, not sure if this goes through custom keymaps. My understanding was that custom keymaps are not updated and we don't provide support for them... Will see, if this can be resolved with versioning.

Sergey Sharybin commented

2024-05-22 12:36:44 +02:00

@blender-bot package

Blender Bot commented

2024-05-22 12:36:47 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR121846) when ready.

Sergey Sharybin commented

2024-05-22 12:41:14 +02:00

@iss Before looking into potentially tricky and fragile versioning logic, update the built-in keymaps. They should not be relying on versioning.

For the breaking aspect of this PR, we need to:

Get some feedback from people who are looking into the Extension platform.
Check with some people who has a modified keymap, to see how the new build behaves for them.

@fsiddi Maybe you can give the answer from the extensions platform perspective? There are many other ways to break custom keymaps, so I am not sure how practical it is to worry about possible regressions every time we need to touch a keymap.

@iss Before looking into potentially tricky and fragile versioning logic, update the built-in keymaps. They should not be relying on versioning. For the breaking aspect of this PR, we need to: - Get some feedback from people who are looking into the Extension platform. - Check with some people who has a modified keymap, to see how the new build behaves for them. @fsiddi Maybe you can give the answer from the extensions platform perspective? There are many other ways to break custom keymaps, so I am not sure how practical it is to worry about possible regressions every time we need to touch a keymap.

Richard Antalik added 1 commit 2024-05-22 13:34:40 +02:00

Ipdate industry compatible keymap. faa476d0f3

Richard Antalik commented

2024-05-22 13:45:29 +02:00

@Sergey Sure, I have updated industry compatible keymap. 2.7x keymap uses default RCS keymap with few changes, so that seems to work.

I have looked into versioning, because I was completely unfamiliar with it, and I am still quite confused. From the code it seems that it only runs, when keymap is imported from .py file, but also when I tried to iterate over keyconfig_data I got 1 item that seems to be flagged as modified. That is not what I am looking for. Or I am missing something here.

But will wait for feedback, since this patch may not be a way forward.

@Sergey Sure, I have updated industry compatible keymap. 2.7x keymap uses default RCS keymap with few changes, so that seems to work. I have looked into versioning, because I was completely unfamiliar with it, and I am still quite confused. From the code it seems that it only runs, when keymap is imported from .py file, but also when I tried to iterate over keyconfig_data I got 1 item that seems to be flagged as modified. That is not what I am looking for. Or I am missing something here. But will wait for feedback, since this patch may not be a way forward.

Sergey Sharybin approved these changes 2024-05-22 17:05:06 +02:00

Sergey Sharybin left a comment

I've talked to Francesco. The keymaps are not a short-term plan for the Extensions platform. That makes it easier for us to move forward.

I only did quick tests, and it seems fine. Reading the code also seems fine. If you've tested it good, lets move forward!

I've talked to Francesco. The keymaps are not a short-term plan for the Extensions platform. That makes it easier for us to move forward. I only did quick tests, and it seems fine. Reading the code also seems fine. If you've tested it good, lets move forward!

Richard Antalik referenced this issue from a commit

2024-05-22 18:47:03 +02:00

Cleanup: Refactor VSE selection tools keymap

Richard Antalik merged commit 8c53a18c48 into main

2024-05-22 18:47:03 +02:00

Richard Antalik deleted branch readable-keymap

2024-05-22 18:47:06 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Cleanup: Refactor VSE selection tools keymap #121846