-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issues/4893 Finalize shortcut api #4969
Conversation
ShortcutRegistration is a result of adding a shortcut and can be used to both configure and remove a shortcut. ShortcutActions is used to register custom shortcut actions.
ClickNotifier has a method for adding a "click shortcut" and Focusable has a method for adding a "focus shortcut".
Still lacking few features: - visibility check for components - Key designator which follows platform conventions (or an API for making shortcuts conform to platform conventions)
Added comments to ShortcutRegistration constructor
Covers: - click shortcut - focus shortcut - scoped shortcut - global shortcut - shortcut tied to visibility flopping component - shortcut tied to attach/detach flopping component
And renamed a method to better represent its functionality.
The method is an overload of the Registration addShortcut(...) variant and it allows for configuration of both owner and lifecycle owner. Also, in Shortcuts, where there was Component owner before, now is Component lifecycleOwner - when writing the IT tests, I noticed a distinct need to make configuring the lifecycle owner easier. I didn't care about owner over much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 10 files at r1.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
KeyModifier... keyModifiers) { | ||
if (lifecycleOwner == null) { | ||
throw new InvalidParameterException(String.format(NULL, | ||
"lifecycleOwner")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @author Vaadin Ltd. | ||
* @since | ||
*/ | ||
public final class Shortcuts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"lifecycleOwner")); | ||
} | ||
if (command == null) { | ||
throw new InvalidParameterException(String.format(NULL, "command")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
if (key == null) { | ||
throw new InvalidParameterException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
if (key == null) { | ||
throw new InvalidParameterException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
if (key == null) { | ||
throw new InvalidParameterException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.format(Shortcuts.NULL, key)); | ||
} | ||
|
||
final Component _this = (Component) this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
if (key == null) { | ||
throw new InvalidParameterException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
executionRegistration.remove(); | ||
} | ||
// isLifecycleOwnerAttached checks for UI status | ||
executionRegistration = lifecycleOwner.getUI().get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* together. | ||
*/ | ||
private static class CompoundRegistration implements Registration { | ||
Set<Registration> registrations; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
private static String generateEventKeyFilter(Key key) { | ||
// will now allow shortcut to happen without primary key | ||
if (key == null) return "false"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
private void markClean() { | ||
synchronized (this) { | ||
if (!isDirty.get()) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keydownRegistration); | ||
} | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static String generateEventModifierFilter( | ||
Collection<Key> modifiers) { | ||
|
||
if (modifiers.size() == 0) return "true"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert lifecycleOwner != null; | ||
|
||
synchronized (this) { | ||
if (isDirty.get()) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this; | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
private final SerializableConsumer<ExecutionContext> | ||
beforeClientResponseConsumer = executionContext -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prepareForClientResponse(); | ||
} | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 18 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @vaadin-bot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @vaadin-bot from 18 discussions.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
* Adds isModifier(Key) to Key interface * Brings over DX tested ShortcutRegistration and ShortcutActions ShortcutRegistration is a result of adding a shortcut and can be used to both configure and remove a shortcut. ShortcutActions is used to register custom shortcut actions. * Adds shortcut support to ClickNotifier and Focusable interfaces ClickNotifier has a method for adding a "click shortcut" and Focusable has a method for adding a "focus shortcut". * Newest API changes for Shortcuts based on the DX testing. Still lacking few features: - visibility check for components - Key designator which follows platform conventions (or an API for making shortcuts conform to platform conventions) * Shortcuts UI test basefile (old demo file) * Added AltGr to KeyModifiers for completeness (also, Key.ALT != Key.ALT_GRAPH) * Removed char-based APIs from Shortcuts. Added comments to ShortcutRegistration constructor * Small unit tests for shortcut registration and due fixes. * Visibility handling, cleaning, and more unit tests * Updated JavaDocs for ShortcutRegistration * Basic IT tests for Shortcuts Covers: - click shortcut - focus shortcut - scoped shortcut - global shortcut - shortcut tied to visibility flopping component - shortcut tied to attach/detach flopping component * Cleaned some duplicate code from ShortcutRegistration. And renamed a method to better represent its functionality. * Added JavaDocs to Shortcuts.java and an extra method. The method is an overload of the Registration addShortcut(...) variant and it allows for configuration of both owner and lifecycle owner. Also, in Shortcuts, where there was Component owner before, now is Component lifecycleOwner - when writing the IT tests, I noticed a distinct need to make configuring the lifecycle owner easier. I didn't care about owner over much. * Added JavaDocs to UI's shortcut methods * Added JavaDocs for Focusable's shortcut methods * JavaDoc for ClickNotifier's shortcut methods * Fixed IT test since the Shortcuts API had changed a little * Added Shortcuts.java exclusion pattern for serialization test
Adds support for Keyboard Shortcuts to Flow.
Shortcut entry points:
UI
for global shortcutsFocusable
for focus shortcutsClickNotifier
for click shortcutsShortcuts
for registering a more customizable shortcutThere are two method variants for adding shortcuts:
Registration add...(...)
andShortcutRegistration register...(...)
. add-methods perform the most basic of registrations and minimal configurability while register-methods returnShortcutRegistration
which provides a fluent configuration API for the shortcut.ShortcutRegistration
provides methods for fluently configuringKeys
,KeyModifiers
, lifecycleOwner, and client-side event behavior (preventDefault, stopPropagation). The shortcuts recognize two semantics forComponents
: owner and lifecycleOwner.Owner is the
component
to which the shortcut's event listener is tied to. In most cases, this will be an instance ofUI
, making the shortcut operate in the global scope. If anothercomponent
is assigned as the owner, the shortcut will only be triggered, if some child html element of thecomponent
is focused during the key event.LifecycleOwner is a component which controls, when the shortcut is active. If the lifecycleOwner is detached or invisible, the shortcut won't get triggered (in case of the invisibility, the check is on the server-side, so the shortcut will in fact get triggered but not executed).
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)