add multi-window support to accessibility system#32797
add multi-window support to accessibility system#32797kryksyh wants to merge 1 commit intomusescore:masterfrom
Conversation
kryksyh
commented
Mar 26, 2026
kryksyh
commented
- Adds app-wide accessibility root, which owns all windows within the same process
- AccessibilityConfiguration was removed since it is no longer needed
- I signed the CLA
- The title of the PR describes the problem it addresses
- Each commit's message describes its purpose and effects, and references the issue it resolves
- If changes are extensive, there is a sequence of easily reviewable commits
- The code in the PR follows the coding rules
- There are no unnecessary changes
- The code compiles and runs on my machine, preferably after each commit individually
- I created a unit test or vtest to verify the changes I made (if applicable)
kryksyh
force-pushed
the
multi-window_accessibility
branch
5 times, most recently
from
c2517b1 to
55b597b
Compare
| } | ||
|
|
||
| return accessibilityConfiguration()->isAccessibleEnabled(); | ||
| return navigationController()->activeSection() != nullptr; |
There was a problem hiding this comment.
What confuses me is:
- This reveals the logic for enabling accessibility, i.e. the logic described here is not the logic for enabling accessibility specifically in the engraving module, but the logic for the entire application.
- If we need this value somewhere, we will either have to repeat this logic or ask the engraving, which will be strange.
- Now the engraving module depends on and knows about the navigation controller.
I propose to bring back iaccessibilitycontextconfiguration
There was a problem hiding this comment.
I saw that this logic is duplicated in AccessibilityController, we can use it instead of iaccessibilitycontextconfiguration
There was a problem hiding this comment.
I saw that this logic is duplicated in AccessibilityController, we can use it instead of iaccessibilitycontextconfiguration
Yes, you are right. My motivation to remove AccessibilyConfiguration was the fact that isAccessibleEnabled is not a configuration, but a current runtime state.
I replaced it with a call to AcecssibilityController
There was a problem hiding this comment.
I'm more concerned that the configurator for one module is currently using the controller from another module.
I think Igor's approach with iaccessibilitycontextconfiguration is better
There was a problem hiding this comment.
Yes, but isAccessibleEnabled is not a configuration option. It is a state. I can create a separate "AccessibilityState" class if you want.
kryksyh
force-pushed
the
multi-window_accessibility
branch
from
55b597b to
f00e037
Compare
|
|
||
| globalIoc()->registerExport<IAccessibilityConfiguration>(mname, m_configuration); | ||
| globalIoc()->registerExport<IQAccessibleInterfaceRegister>(mname, new QAccessibleInterfaceRegister()); | ||
| globalIoc()->registerExport<IAccessibleAppRootObject>(mname, new AccessibleAppRootObject()); |
There was a problem hiding this comment.
Let's not register the object for export. I understand the controllers and configurators in the export, but the object seems odd.
The rootObject method in AccessibilityController should probably be enough for us.
There was a problem hiding this comment.
I can do that if you really want. I made it a global export, because otherwise it has to be a static member of AccessiblityController. And such hidden statics are easy to misuse.
But it is your call.
There was a problem hiding this comment.
This should be in the ui module, next to the class it belongs to
- Adds app-wide accessibility root, that owns all windows within the same process - AccessibilityConfiguration was removed since it is no longer needed
kryksyh
force-pushed
the
multi-window_accessibility
branch
from
f00e037 to
e0814eb
Compare
3 participants
