Skip to content
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

feat(web): introduction of flicks 🐵 #9909

Merged
merged 13 commits into from
Nov 14, 2023

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Oct 31, 2023

Addresses the primary remaining concern of #5029 - the flick gesture.

Note that they don't "feel great" at this stage in development. The key-preview system cuts off quickly during the flick, giving the feeling that "something's off", even though the flicks do work. That'll be addressed in a followup.

It'd probably be wise to make some sort of keyboard to better test the correction-distribution aspect - with actual 'base letters' as flicks, since that would actually show up. Haven't actually done that yet.

User testing will be requested on a follow-up, as key previews are pretty important for getting the feel of flicks right.

@keymanapp-test-bot skip

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Oct 31, 2023
@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Nov 1, 2023
@jahorton jahorton marked this pull request as ready for review November 1, 2023 07:14
@jahorton jahorton requested a review from mcdurdin as a code owner November 1, 2023 07:14
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from minor niggles

const TRIGGER_DIST = this.gestureParams.flick.triggerDist;
const baseDist = Math.min(TRIGGER_DIST, pathStats.netDistance);

// The 1.001: makes the opposite direction ever-so-slightly less likely than the base key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment may belong somewhere else now?

@@ -225,6 +247,36 @@ export const InstantContactResolutionModel: ContactModel = {
}
}

export function flickStartContactModel(params: GestureParams): ContactModel {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function flickStartContactModel(params: GestureParams): ContactModel {
export function FlickStartContactModel(params: GestureParams): ContactModel {

I am not actually sure which direction you want the casing to go because it looks like there are existing inconsistencies in this file. Fixup in the direction you think is best!

Copy link
Contributor Author

@jahorton jahorton Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lowercase: it's configured by a function.
Uppercase: it's a statically-defined object with no configuration of any sort.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's a little obtuse 😁 BasicLongpressContactModel also doesn't seem to fit?

Copy link
Contributor Author

@jahorton jahorton Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll handle this and its related suggestions in #9998. More changes occur in descendant PRs that use the old patterns; #9998 is after all such work has been completed, so it's well suited to take care of all related tasks in a single PR.

}
}

export function flickEndContactModel(params: GestureParams): ContactModel {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function flickEndContactModel(params: GestureParams): ContactModel {
export function FlickEndContactModel(params: GestureParams): ContactModel {

@@ -534,6 +586,61 @@ export function longpressModelWithRoaming(params: GestureParams): GestureModel<a
}
}

export function flickStartModel(params: GestureParams): GestureModel<any> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function flickStartModel(params: GestureParams): GestureModel<any> {
export function FlickStartModel(params: GestureParams): GestureModel<any> {

}
}

export function flickEndModel(params: GestureParams): GestureModel<any> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function flickEndModel(params: GestureParams): GestureModel<any> {
export function FlickEndModel(params: GestureParams): GestureModel<any> {

Comment on lines +1180 to +1182
this.gestureParams.longpress.flickDist = 0.25 * this.currentLayer.rowHeight;
this.gestureParams.flick.startDist = 0.25 * this.currentLayer.rowHeight;
this.gestureParams.flick.triggerDist = 0.75 * this.currentLayer.rowHeight;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these numbers be shifted into constants somewhere?

@mcdurdin mcdurdin modified the milestones: A17S25, A17S26 Nov 13, 2023
Base automatically changed from feat/web/modipress to feature-gestures November 13, 2023 04:04
@jahorton jahorton deleted the feat/web/flick-implementation branch November 14, 2023 08:26
@jahorton jahorton mentioned this pull request Dec 12, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants