Cleanup: Refactor VSE selection tools keymap #121846

Merged
Richard Antalik merged 5 commits from iss/blender:readable-keymap into main 2024-05-22 18:47:03 +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
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
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
Owner

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) ```
@ -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'},

Is it indeed intended to be LEFTMOUSE in RCS codepath?

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

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

No, this was meant to be implemented by #109522, but forgot to remove this item.
iss marked this conversation as resolved
@ -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)]}),

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.
Author
Member

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

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
Author
Member

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
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
0d4b0420ac
Author
Member

@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.
Author
Member

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.

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.

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?
Author
Member

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.

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

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

@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
Author
Member

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

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 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
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#121846
No description provided.