PyAPI: postpone loading add-ons until after key-maps have been defined #110092

Manually merged
Campbell Barton merged 5 commits from ideasman42/blender:pr-postpone-loading-add-ons into main 2023-09-05 03:45:47 +02:00

Loading add-ons after key-maps resolves a problem where add-ons would setup shortcuts before Blender had created the key-maps.
Making add-ons have to declare the key-maps using region & window types matching Blender's internal values.


This PR addresses a pitfall pointed out in #110030.

Loading add-ons after key-maps resolves a problem where add-ons would setup shortcuts before Blender had created the key-maps. Making add-ons have to declare the key-maps using region & window types matching Blender's internal values. ---- This PR addresses a pitfall pointed out in #110030.
Campbell Barton requested review from Demeter Dzadik 2023-07-14 10:19:45 +02:00
Campbell Barton requested review from Brecht Van Lommel 2023-07-14 10:19:55 +02:00
Author
Owner

@blender-bot build

@blender-bot build
Member

I tried the patch but I don't think it's working, or I'm missing the idea.

Here's what I tried:

# Make sure patch is applied by importing a function that doesn't exist without the patch.
from bpy.utils import load_scripts_extended

import bpy
kc = bpy.context.window_manager.keyconfigs.addon
km = kc.keymaps.get('Text') # Returns None, like before.

# Try registering without caring about region_type or space_type. (I thought this was the purpose of the patch)
if not km:
    km = kc.keymaps.new('Text') # Don't specify space_type, which should be 'TEXT_EDITOR'.
kmi = km.keymap_items.new('text.run_script', type='Q', value='PRESS', alt=True) # Runs without error. Hotkey does not work.

# Try the old way.
km = kc.keymaps.new('Text', space_type='TEXT_EDITOR', region_type='WINDOW')
kmi = km.keymap_items.new('text.run_script', type='Q', value='PRESS', shift=True) # Hotkey will work.

print("Done")

# 

And what I found is that with the patch applied, the script actually needs to run twice, in order for the Shift+Q hotkey to start working, which is not the case without the patch.

I tried the patch but I don't think it's working, or I'm missing the idea. Here's what I tried: ```python # Make sure patch is applied by importing a function that doesn't exist without the patch. from bpy.utils import load_scripts_extended import bpy kc = bpy.context.window_manager.keyconfigs.addon km = kc.keymaps.get('Text') # Returns None, like before. # Try registering without caring about region_type or space_type. (I thought this was the purpose of the patch) if not km: km = kc.keymaps.new('Text') # Don't specify space_type, which should be 'TEXT_EDITOR'. kmi = km.keymap_items.new('text.run_script', type='Q', value='PRESS', alt=True) # Runs without error. Hotkey does not work. # Try the old way. km = kc.keymaps.new('Text', space_type='TEXT_EDITOR', region_type='WINDOW') kmi = km.keymap_items.new('text.run_script', type='Q', value='PRESS', shift=True) # Hotkey will work. print("Done") # ``` And what I found is that with the patch applied, the script actually needs to run twice, in order for the Shift+Q hotkey to start working, which is not the case without the patch.
Campbell Barton changed title from PyAPI: postpone loading add-ons until after key-maps have been defined to WIP: PyAPI: postpone loading add-ons until after key-maps have been defined 2023-07-14 12:09:51 +02:00
Author
Owner

Updated the patch with an experimental function to create a key-map based on the default:

km = kc.keymaps.get('Text') # Returns None, like before.

# Try registering without caring about region_type or space_type. (I thought this was the purpose of the patch)
if not km:
    km = kc.keymaps.new('Text') # Don't specify space_type, which should be 'TEXT_EDITOR'.

Can be replaced with

km = kc.keymaps.ensure_from_default("Text")

The key-map uses settings (space type, region type etc...) from bpy.context.window_manager.keyconfigs.default.

Updated the patch with an experimental function to create a key-map based on the default: ``` km = kc.keymaps.get('Text') # Returns None, like before. # Try registering without caring about region_type or space_type. (I thought this was the purpose of the patch) if not km: km = kc.keymaps.new('Text') # Don't specify space_type, which should be 'TEXT_EDITOR'. ``` Can be replaced with ``` km = kc.keymaps.ensure_from_default("Text") ``` The key-map uses settings (space type, region type etc...) from `bpy.context.window_manager.keyconfigs.default`.
Member

That's much better, thank you! But something still seems off:

import bpy

wm = bpy.context.window_manager
kc = wm.keyconfigs.addon
km = kc.keymaps.ensure_from_default("Text")
km.keymap_items.new('text.run_script', type='Q', value='PRESS', shift=True)

print("hi")

When first executing this script, it prints "hi", but the hotkey doesn't appear in the preferences UI yet, and doesn't work either. When running the script again, the hotkey appears, and starts functioning. Could that be to do with having changed the order of things?

That's much better, thank you! But something still seems off: ```python import bpy wm = bpy.context.window_manager kc = wm.keyconfigs.addon km = kc.keymaps.ensure_from_default("Text") km.keymap_items.new('text.run_script', type='Q', value='PRESS', shift=True) print("hi") ``` When first executing this script, it prints "hi", but the hotkey doesn't appear in the preferences UI yet, and doesn't work either. When running the script again, the hotkey appears, and starts functioning. Could that be to do with having changed the order of things?
Author
Owner

This is strange, in my tests the resulting hot-key worked as expected.

This is strange, in my tests the resulting hot-key worked as expected.
Member

This is strange, in my tests the resulting hot-key worked as expected.

You seem to be right, I think I made a mistake! Tried again, and it seems to work on first try. Not quite sure what was happening earlier, maybe I was going crazy.

> This is strange, in my tests the resulting hot-key worked as expected. You seem to be right, I think I made a mistake! Tried again, and it seems to work on first try. Not quite sure what was happening earlier, maybe I was going crazy.
Brecht Van Lommel requested changes 2023-07-17 19:51:22 +02:00
Brecht Van Lommel left a comment
Owner

There is now a WM_init -> wm_homefile_read_post call before loading add-ons, which in turn calls:

  • wm_read_callback_post_wrapper, which does a load post handler. An add-on may rely on this being called once on startup.
  • wm_file_read_post, which does some app template stuff, more handlers, depsgraph evaluation, etc.

Maybe a safer change is to move some of this code from creator.c to the end of WM_init, right before wm_homefile_read_post?

  RE_engines_init_experimental();

  WM_keyconfig_init(C);

#ifdef WITH_FREESTYLE
  FRS_init();
  FRS_set_context(C);
#endif

  WM_init_scripts_extended_once(C);

  wm_homefile_read_post(C, params_file_read_post);
There is now a `WM_init` -> `wm_homefile_read_post` call before loading add-ons, which in turn calls: * `wm_read_callback_post_wrapper`, which does a load post handler. An add-on may rely on this being called once on startup. * `wm_file_read_post`, which does some app template stuff, more handlers, depsgraph evaluation, etc. Maybe a safer change is to move some of this code from `creator.c` to the end of `WM_init`, right before `wm_homefile_read_post`? ``` RE_engines_init_experimental(); WM_keyconfig_init(C); #ifdef WITH_FREESTYLE FRS_init(); FRS_set_context(C); #endif WM_init_scripts_extended_once(C); wm_homefile_read_post(C, params_file_read_post); ```
@ -190,3 +190,3 @@
def load_scripts(*, reload_scripts=False, refresh_scripts=False):
def load_scripts(*, reload_scripts=False, refresh_scripts=False, extended=True):

Maybe renameextended to extensions?

To me that seems like a logical name given the future direction for naming such things extensions, while "extended" is different enough that it sounds like it could be something different.

Maybe rename`extended` to `extensions`? To me that seems like a logical name given the future direction for naming such things extensions, while "extended" is different enough that it sounds like it could be something different.
ideasman42 marked this conversation as resolved
Campbell Barton force-pushed pr-postpone-loading-add-ons from aa2fbbbbc0 to 2bdffbe774 2023-08-30 09:45:31 +02:00 Compare
Campbell Barton requested review from Brecht Van Lommel 2023-08-30 09:46:01 +02:00
Brecht Van Lommel requested changes 2023-08-30 10:04:59 +02:00
Brecht Van Lommel left a comment
Owner

You only moved WM_init_scripts_extended_once to WM_init, not all the functions I suggested. In particular, WM_keyconfig_init now runs after WM_init_scripts_extended_once. Was this intentional?

You only moved `WM_init_scripts_extended_once` to `WM_init`, not all the functions I suggested. In particular, `WM_keyconfig_init` now runs after `WM_init_scripts_extended_once`. Was this intentional?
Author
Owner

No, not intentional, it's a bit late here, I'll update the patch & double-check tomorrow.

No, not intentional, it's a bit late here, I'll update the patch & double-check tomorrow.
Campbell Barton force-pushed pr-postpone-loading-add-ons from 2bdffbe774 to ecc61aaf8e 2023-08-31 07:51:51 +02:00 Compare
Author
Owner

Updated, although I'd rather keep freestyle & render engine init out of WM_init().
While this probably works OK either way, they don't really fit in WM initialization.

If freestyle moves into WM_init, The callback defined in FRS_init before wm_homefile_read_post runs, although in practice this doesn't matter either way at the moment.

Updated, although I'd rather keep freestyle & render engine init out of `WM_init()`. While this probably works OK either way, they don't really fit in WM initialization. If freestyle moves into WM_init, The callback defined in FRS_init before `wm_homefile_read_post` runs, although in practice this doesn't matter either way at the moment.
Campbell Barton requested review from Brecht Van Lommel 2023-08-31 07:59:31 +02:00
Campbell Barton added 1 commit 2023-08-31 08:10:08 +02:00
Remove ensure_from_default method, this can be added separately
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
b746502e89
Author
Owner

@blender-bot build
The test that failed was unrelated to this PR.

@blender-bot build _The test that failed was unrelated to this PR._
Campbell Barton changed title from WIP: PyAPI: postpone loading add-ons until after key-maps have been defined to PyAPI: postpone loading add-ons until after key-maps have been defined 2023-08-31 10:06:01 +02:00
Brecht Van Lommel approved these changes 2023-09-04 11:21:48 +02:00
Campbell Barton closed this pull request 2023-09-05 03:45:23 +02:00
Campbell Barton reopened this pull request 2023-09-05 03:45:33 +02:00
Campbell Barton manually merged commit 6de294a191 into main 2023-09-05 03:45:47 +02:00
First-time contributor

Is there any chance the old behavior for keymaps will return to the way it used to be in 4.0? It was super useful to be able to change user shortcuts with addons at startup, but now it has stopped working, it is no longer possible without some really bad hacks :(

Is there any chance the old behavior for keymaps will return to the way it used to be in 4.0? It was super useful to be able to change user shortcuts with addons at startup, but now it has stopped working, it is no longer possible without some really bad hacks :(

@Dangry it's not clear to me how this would make it impossible to change user shortcuts. If anything this change should give more control. Consider making a bug report with specific details about what stopped working.

@Dangry it's not clear to me how this would make it impossible to change user shortcuts. If anything this change should give more control. Consider making a bug report with specific details about what stopped working.
First-time contributor

@Dangry it's not clear to me how this would make it impossible to change user shortcuts. If anything this change should give more control. Consider making a bug report with specific details about what stopped working.

@brecht
Hi Breacht,
sorry if I was unclear, might have missed something here, but to the best of my understanding, this patch unfortunately got reverted 2 weeks ago because of a bug, as can be seen here: cdbde7d941.

This means that it is way harder to change user keymaps when an addon is loading at startup, so its not a bug, but how it used to be, since it requires a hack to make it work, just like before 4.0.

This patch was super nice, and it was something as an addon developer I looked forward to a lot in 4.0. I am a little sad to see it gone since it's a very nice quality of life feature when making addons. I really hope it will be reconsidered in the futer!

> @Dangry it's not clear to me how this would make it impossible to change user shortcuts. If anything this change should give more control. Consider making a bug report with specific details about what stopped working. @brecht Hi Breacht, sorry if I was unclear, might have missed something here, but to the best of my understanding, this patch unfortunately got reverted 2 weeks ago because of a bug, as can be seen here: https://projects.blender.org/blender/blender/commit/cdbde7d9413c67ee5cecf9ccbd13fc9d719553c8. This means that it is way harder to change user keymaps when an addon is loading at startup, so its not a bug, but how it used to be, since it requires a hack to make it work, just like before 4.0. This patch was super nice, and it was something as an addon developer I looked forward to a lot in 4.0. I am a little sad to see it gone since it's a very nice quality of life feature when making addons. I really hope it will be reconsidered in the futer!

It didn't get reverted but modified.

It's still unclear to me what you wanted to do and why. The design is that there is a Blender base keymap, then add-ons can add their own items on top of that, and then as the last step user modifications get applied.

Typical add-ons should not be messing with the user modifications, unless it's an add-on specifically about keymap editing.

It didn't get reverted but modified. It's still unclear to me what you wanted to do and why. The design is that there is a Blender base keymap, then add-ons can add their own items on top of that, and then as the last step user modifications get applied. Typical add-ons should not be messing with the user modifications, unless it's an add-on specifically about keymap editing.
Sign in to join this conversation.
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
4 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#110092
No description provided.