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

Adding triple tap to select current row for widget.Entry #4337

Merged
merged 7 commits into from
Dec 14, 2023

Conversation

dweymouth
Copy link
Contributor

@dweymouth dweymouth commented Oct 21, 2023

Description:

Fixes #4328

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

@dweymouth dweymouth marked this pull request as draft October 21, 2023 19:00
@dweymouth dweymouth marked this pull request as ready for review October 21, 2023 19:11
@coveralls
Copy link

coveralls commented Oct 22, 2023

Coverage Status

coverage: 64.614% (-0.5%) from 65.084%
when pulling f710676 on dweymouth:entry-triple-tap
into 2921c11 on fyne-io:develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Great to have this addition - but I wonder if we can avoid duplicating constants?

widget/entry.go Outdated Show resolved Hide resolved
@dweymouth
Copy link
Contributor Author

Looks like there's still an import cycle in test code. I'll take a look in a bit

Jacalz
Jacalz previously requested changes Nov 7, 2023
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

This is wonderful. What a difference in usability! Thanks :D
Just left one comment on the placement of the consts :)

internal/driver/consts/consts.go Outdated Show resolved Hide resolved
@Jacalz
Copy link
Member

Jacalz commented Dec 10, 2023

I wonder, should we expose an interface for making a widget triple tappable instead of exposing the double tap delay?

@dweymouth
Copy link
Contributor Author

dweymouth commented Dec 10, 2023

We figured that Entry was probably the only widget that needed triple tap. Also personally I'd like the double tap delay to be exposed since I have my list rows in Supersonic implement their own double tap logic (here - so that tap can instantly trigger to select the row of a tracklist while double tap can play a track). Having a widget have both Tappable and DoubleTappable means, by design, it won't get the tap event until after the delay, which is not what I wanted in my case.

Edit: either that - or a way to request for a specific widget that the Tapped callback should be invoked immediately even if it may become a DoubleTap if tapped again

@dweymouth dweymouth requested a review from andydotxyz December 10, 2023 22:18
@andydotxyz
Copy link
Member

Edit: either that - or a way to request for a specific widget that the Tapped callback should be invoked immediately even if it may become a DoubleTap if tapped again

This is tough to be convinced of - as Tapped is an abstract request for the tap event, as you know we have lower level events for when the widget wants to do smarter things. Double tapping, and indeed Triple, are not things that most widgets will be doing (I hope!) as there is no discoverability to these actions.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks, this looks to be in a good place now :)

@dweymouth dweymouth dismissed Jacalz’s stale review December 14, 2023 17:45

no longer relevant

@dweymouth dweymouth merged commit c90d255 into fyne-io:develop Dec 14, 2023
11 of 12 checks passed
@dweymouth dweymouth deleted the entry-triple-tap branch December 14, 2023 17:47
@dweymouth dweymouth added this to the "Elgin" release, late 2023 milestone Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants