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

Mouse click in an entry updates cursor position twice, with a noticeable delay #2525

Closed
nullst opened this issue Oct 5, 2021 · 4 comments
Closed
Labels
bug Something isn't working

Comments

@nullst
Copy link
Contributor

nullst commented Oct 5, 2021

Describe the bug:

Suppose an entry widget has some text in it. User does a mouse click somewhere in the text and immediately starts typing "qwerty". When I do that, I usually get "ertyqw" because after I type one or two symbols (in this case, "qw") the cursor resets back to the position where I clicked, and the remaining symbols (in this case, "erty") are inserted before "qw".

This behavior is easily visible in a multiline entry: click at the end of some line and quickly press Enter to start a new line. Soon the cursor will return back to the end of the line you clicked. This does not require extreme speed, I can easily trigger this behavior even using the same hand to click the mouse first and press Enter afterwards.

Bug location

In widget/entry.go both callbacks Tapped() and MouseDown() call e.updateMousePointer. For some reason Tapped() is invoked after a delay, allowing user to type some text in between these two callbacks. I'm not sure how input handling works in fyne, but maybe on a desktop machine it is enough to update the cursor only in MouseDown()? For example, by modifying Tapped() as follows:

func (e *Entry) Tapped(ev *fyne.PointEvent) {
	if !e.Disabled() {
		e.requestFocus()
	}
	if fyne.CurrentDevice().IsMobile() && e.selecting {
		e.selecting = false
	}
	if fyne.CurrentDevice().IsMobile() { // Added this line
		e.updateMousePointer(ev.Position, false)
	} // Added this line
}

This fixes the problem on my machine.

To Reproduce:

Steps to reproduce the behaviour:

  1. Open fyne_demo and navigate to "Widget/Entry" page.
  2. Type something in the entry.
  3. Click in the middle of the text you typed and start typing "qwerty".
  4. You will probably see "ertyqw" or something like that instead of the intended result.

Device (please complete the following information):

  • OS: MacOS
  • Version: 10.12 Sierra
  • Go version: go1.15.3 darwin/amd64
  • Fyne version: 6e90820
@nullst nullst added the unverified A bug that has been reported but not verified label Oct 5, 2021
@andydotxyz
Copy link
Member

Good call.
Tapped has a delay because we first need to check it was not a DoubleTap, hence the short delay.
Your proposed fix is a good one, however it could leave the delay on Mobile.
Instead we could move it to only move cursor on MouseDown and TouchDown (that should catch both specific platforms, removing the need for Tapped).

@andydotxyz andydotxyz added bug Something isn't working and removed unverified A bug that has been reported but not verified labels Oct 5, 2021
@andydotxyz andydotxyz added this to the Fixes (v2.1.x) milestone Oct 5, 2021
nullst added a commit to nullst/fyne that referenced this issue Oct 5, 2021
@nullst
Copy link
Contributor Author

nullst commented Oct 5, 2021

Apparently, this modification would break TestEntry_Tapped in widget/entry_test.go since that test uses TapCanvas() to simulate mouse clicks. In fact, the fyne/v2/test package can only call Tapped() on a widget, not device-specific mouse or touch events. I don't know what the proper fix would do: maybe switch the test package to MouseDown()/MouseUp() pairs when on desktop?

UPD: in the file widget/entry_internal_test.go the callbacks Tapped(), DoubleTapped() are also invoked a lot. Should the tests there be rewritten in some way?

@andydotxyz
Copy link
Member

DoubleTapped is separate and shouldn't be touched.
I don't think we should change how the test package works, but it would be reasonable to add a test helper for the entry test that uses MouseDown/Up on desktop and TouchDown/Up when running in mobile mode.

Jacalz pushed a commit that referenced this issue Oct 12, 2021
Fixes #2525 following the strategy suggested there by @andydotxyz .
andydotxyz pushed a commit that referenced this issue Oct 12, 2021
@andydotxyz
Copy link
Member

on release/v2.1.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants