Add ShortcutsRegistry #103456
Add ShortcutsRegistry #103456
Conversation
flutter-dashboard
bot
added
the
framework
gspencergoog
force-pushed
the
shortcuts_registry
branch
9 times, most recently
from
979a118 to
4b66ac1
Compare
|
The analyzer seems unhappy with this one. |
gspencergoog
force-pushed
the
shortcuts_registry
branch
3 times, most recently
from
d905384 to
f4e8507
Compare
|
Looks like the doc bot is unhappy. |
|
Doc bot should be happy now. |
There was a problem hiding this comment.
One thing that's a little odd about this setup is the disconnect between getter and setter. The getter doesn't just return what you've set with the setter...
There was a problem hiding this comment.
Resolved that by not having ShortcutRegistry be a ShortcutManager, and linking the updates in a listener in the ShortcutRegistrar.
gspencergoog
force-pushed
the
shortcuts_registry
branch
from
1141689 to
6926b12
Compare
|
Okay, PTAL. I got around the overriding of the I think we might want to deprecate and remove the |
gspencergoog
force-pushed
the
shortcuts_registry
branch
from
01e1c13 to
388b343
Compare
gspencergoog
force-pushed
the
shortcuts_registry
branch
from
388b343 to
5ddcdbc
Compare
There was a problem hiding this comment.
I believe this is still stating the wrong thing.
| /// [ShortcutRegistryToken.replaceAll] and supply the token returned by | ||
| /// [ShortcutRegistry.addAll]. |
There was a problem hiding this comment.
nit: you no longer have to supply the token.
There was a problem hiding this comment.
Good point. Fixed.
| /// | ||
| /// When the token is no longer needed, [dispose] should be called, and the | ||
| /// token should no longer be used. | ||
| class ShortcutRegistryToken { |
There was a problem hiding this comment.
Wondering: would ShortcutRegistryEntry be a better name for this?
There was a problem hiding this comment.
Yeah, that works too. I'll change it. I also considered "memento", since that's the name of the pattern, but it's esoteric.
| shortcuts: registry.shortcuts, | ||
| manager: manager, |
There was a problem hiding this comment.
This is a little strange in the (existing) Shortcuts API: You can pass a manager basically saying "hey, I want to manage the shortcuts manually" - but then the Shortcuts widget goes behind your back and modifies the manager...
(I was initially confused because you only update the manager when the shortcuts change, but they never get initially configured on it in this class)
There was a problem hiding this comment.
Yes, I agree. The only use case I can see is if you supply some custom subclass of ShortcutManager, and you need to find it so you can talk to it. But you can do that yourself, and it really doesn't have anything to do with the official one.
I'll deprecate the of and maybeOf in another PR.
I am in favor of this. The use case for that API seems unclear and actually directly interacting with the manager seems dangerous as it conflicts with what the Shortcuts widget does with it. |
|
This pull request is not suitable for automatic merging in its current state.
|
This reverts commit f6c3ee3.
| /// ``` | ||
| /// {@end-tool} | ||
| @protected | ||
| bool debugAssertNotDisposed() { |
There was a problem hiding this comment.
This is a breaking change, breaking all classes that implement ChangeNotifier.
i.e. I'm using an alternative ChangeNotifier implementation that doesn't throw when disposed multiple times.
It could be implemented as extension instead
This reverts part of the change made in #103456 to expose a debug check for subclasses of ChangeNotifier to avoid code duplication. Instead of making debugAssertNotDisposed a public instance function, it is now a public static function. It makes it harder to call, slightly, but it means that everyone who implemented ChangeNotifier instead of extending it doesn't get broken.
This reverts part of the change made in flutter#103456 to expose a debug check for subclasses of ChangeNotifier to avoid code duplication. Instead of making debugAssertNotDisposed a public instance function, it is now a public static function. It makes it harder to call, slightly, but it means that everyone who implemented ChangeNotifier instead of extending it doesn't get broken.
4 participants
Description
This adds a
ShortcutsRegistryforShortcutActivatortoIntentmappings that can be modified from its descendants.This is so that descendants can make shortcuts dynamically available to a larger portion of the app than just their descendants. This is a precursor needed by the new
MenuBar, for instance, so that the menu bar itself can be placed where it likes, but the shortcuts it defines can be in effect for most, if not all, of the UI surface in the app. For example, the "Ctrl-Q" quit binding would need to work even if the focused widget wasn't a child of theMenuBar.This just provides the shortcut to intent mapping, the actions activated by the intent are described in the context where they make sense. For example, defining a "Ctrl-C" shortcut mapped to a "CopyIntent" should perform different functions if it happens while a
TextFieldhas focus vs when a drawing has focus, so those different areas would need to define different actions mapped to "CopyIntent". A hypothetical "QuitIntent" would probably be active for the entire app, so would be mapped in anActionswidget near the top of the hierarchy.Tests