Call OnChange with every updateText, fixes #4117 #4121

mbaklor · 2023-08-02T07:59:19Z

Description:

This adds a call to entry.OnChanged() every time entry.updateText is called.
While I was at it I also added a call to e.Validate where I didn't see one.

Fixes #4117

Checklist:

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

coveralls · 2023-08-02T08:06:01Z

coverage: 65.282% (+0.007%) from 65.275% when pulling d21ec3e on mbaklor:entry-callback into 6f2c727 on fyne-io:develop.

Jacalz

Thanks for the PR. This does indeed fix the issue but I noticed that introduces some unwanted side effects that I think we need to handle. Some of the functions that now validate and run OnChanged does unfortunately later on call them again. I left some comments inline about what we need to look out for.

Jacalz · 2023-08-02T19:01:30Z

widget/entry.go

 	e.propertyLock.Unlock()

+	e.Validate()
+	if changed && e.OnChanged != nil {
+		e.OnChanged(provider.String())


You should avoid calling provider.String() multiple times. If you look at the code you will see that it allocates an entirely new string each time. You probably want to save the string to a variable. Have a look at TypedRune to see how it is handled there.

np, fixed in next commit

Jacalz · 2023-08-02T19:20:42Z

widget/entry.go

 	if e.CursorRow == e.selectRow && e.CursorColumn == e.selectColumn {
 		e.selecting = false
 	}
 	e.propertyLock.Unlock()
+	e.Validate()


This leads to unwanted side effects. Pressing tab inside a multiline entry calls validation twice but we also validate the text even in all cases where we know that nothing has changed (i.e. moving the cursor using tab keys but also pressing keys like home, page up, end etc.). I don't think doing validation that often will be good if users have slow validators.

Jacalz · 2023-08-02T19:28:29Z

widget/entry.go

-	e.updateText(provider.String())
+	changed := e.updateText(provider.String())
+
+	e.Validate()


This change also leads to verifying twice (potentially running OnChanged twice as well, I would assume) when selecting something and then pressing another key using TypedKey or TypedRune to replace the text.

Jacalz · 2023-08-02T19:35:21Z

FYI @mbaklor. I think the Fixes # needs to be on a separate line for GitHub to understand that it should be closed. I updated the text for you :)

mbaklor · 2023-08-02T21:13:57Z

Very fair, I actually threw the validate in all those cases as an afterthought because I think hitting backspace should validate as well. I'll take a look tomorrow and remove unnecessary ones and think how best to validate backspace 😁👍

mbaklor · 2023-08-03T21:38:19Z

okay as you can see I'm very good at using the comments thing in the code lol, I just tested (manually, I should've probably written a test but I only thought of checking after the commit 😅) and looks like everything now calls validate and onchanged only once

andydotxyz · 2023-08-05T08:29:08Z

I haven't tested myself, but at a glance I would be concerned that highlighting text and the entering a key may validate and fire handlers twice - once when clearing selection and another when entering the new text.
The "clearSelection" was created as a helper so be sure to check all the places it is called.

mbaklor · 2023-08-05T21:44:34Z

Within TypedRune there's a little snippet to clear OnChanged before erasing selection, so I figured it was fine, I'll go over where eraseSelection is called again just to be sure

mbaklor · 2023-08-09T19:58:59Z

okay so there were some repetitions of the callbacks from eraseSelection, so I removed the callbacks from there and into where it wasn't called when not in eraseSelection. Hope this was the correct choice, lmk if that's not the solution

Jacalz · 2023-08-15T19:03:41Z

There are a lot of test failures (crashes). Would you mind having a look at those?

mbaklor · 2023-08-15T21:40:39Z

oh my god I should not push commits at 1:00 AM (not that now isn't 1:00 AM but it is what it is)

yes I didn't check that OnChanged exists before calling it. fixed now. commit incoming 😅

andydotxyz

Looks great, thanks so much

Jacalz

Thanks. This is looking great. I just released that we still are allocating new strings in this code so I added some suggestions inline to remedy the issue.

Sorry for the long review process. This will be ready to land very soon, I promise :)

Jacalz · 2023-08-18T11:22:05Z

widget/entry.go

+	content := e.textProvider().String()
+	if e.OnChanged != nil {
+		e.OnChanged(content)


I realized that this still is creating a new copy of the string. Looking at eraseSelection, it calls e.updateText() which sets the e.Text field and as we are in a lock we can simply reused that instead of recreating the string once more (slow and potentially uses a lot more memory for large strings).

Suggested change

content := e.textProvider().String()

if e.OnChanged != nil {

e.OnChanged(content)

if e.OnChanged != nil {

e.OnChanged(e.Text)

Jacalz · 2023-08-18T11:22:49Z

widget/entry.go

+		content := e.textProvider().String()
+		if e.OnChanged != nil {
+			e.OnChanged(content)


Same comment as above applies here.

Suggested change

content := e.textProvider().String()

if e.OnChanged != nil {

e.OnChanged(content)

if e.OnChanged != nil {

e.OnChanged(e.Text)

Jacalz · 2023-08-20T11:51:50Z

@mbaklor Would you be able to get the last two changes done soon? This is one of the remaining release blockers and it would be good to have it resolved as soon as possible :)

mbaklor · 2023-08-20T13:30:46Z

Yes absolutely, I was planning on getting it done tonight, I'll see if I can get to it earlier but I doubt

Jacalz · 2023-08-20T13:46:26Z

Great. Thanks. There are other blockers that haven't been solved yet so a few hours earlier or later today isn't a problem

mbaklor · 2023-08-20T21:04:08Z

Sorry for the long review process. This will be ready to land very soon, I promise :)

No worries, I'm honestly glad for the long and thorough process, I'm still rather insecure in my golang and happy to have someone else make sure what I'm contributing won't break the library 😉

Jacalz

This looks great now and solves one of the important blocker bugs which is wonderful. Thanks for working on this and congratulations on landing your first change into Fyne :)

fix fyne-io#4117 call OnChange with every updateText

dd817b1

Jacalz requested changes Aug 2, 2023

View reviewed changes

Jacalz changed the title ~~fix #4117 call OnChange with every updateText~~ Call OnChange with every updateText, fixes #4117 Aug 2, 2023

mbaklor added 2 commits August 3, 2023 23:44

call provider.String() only once

14b82c4

validate only if text was changed

2e4804b

hopefully no more duplicate callbacks on entry

01f89d2

check if OnChanged exists on entry before calling

d21ec3e

mbaklor force-pushed the entry-callback branch from ae7d775 to d21ec3e Compare August 15, 2023 21:46

andydotxyz previously approved these changes Aug 18, 2023

View reviewed changes

Jacalz requested changes Aug 18, 2023

View reviewed changes

avoid unneccessary string copies in entry

3b60399

mbaklor dismissed andydotxyz’s stale review via 3b60399 August 20, 2023 20:59

Jacalz approved these changes Aug 20, 2023

View reviewed changes

Jacalz merged commit 8be6541 into fyne-io:develop Aug 20, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call OnChange with every updateText, fixes #4117 #4121

Call OnChange with every updateText, fixes #4117 #4121

mbaklor commented Aug 2, 2023 •

edited by Jacalz

Loading

coveralls commented Aug 2, 2023 •

edited

Loading

Jacalz left a comment

Jacalz Aug 2, 2023 •

edited

Loading

mbaklor Aug 3, 2023

Jacalz Aug 2, 2023

Jacalz Aug 2, 2023

Jacalz commented Aug 2, 2023

mbaklor commented Aug 2, 2023

mbaklor commented Aug 3, 2023

andydotxyz commented Aug 5, 2023

mbaklor commented Aug 5, 2023

mbaklor commented Aug 9, 2023

Jacalz commented Aug 15, 2023

mbaklor commented Aug 15, 2023

andydotxyz left a comment

Jacalz left a comment

Jacalz Aug 18, 2023 •

edited

Loading

Jacalz Aug 18, 2023

Jacalz commented Aug 20, 2023

mbaklor commented Aug 20, 2023

Jacalz commented Aug 20, 2023

mbaklor commented Aug 20, 2023

Jacalz left a comment

Call OnChange with every updateText, fixes #4117 #4121

Call OnChange with every updateText, fixes #4117 #4121

Conversation

mbaklor commented Aug 2, 2023 • edited by Jacalz Loading

Description:

Checklist:

coveralls commented Aug 2, 2023 • edited Loading

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

mbaklor Aug 3, 2023

Choose a reason for hiding this comment

Jacalz Aug 2, 2023

Choose a reason for hiding this comment

Jacalz Aug 2, 2023

Choose a reason for hiding this comment

Jacalz commented Aug 2, 2023

mbaklor commented Aug 2, 2023

mbaklor commented Aug 3, 2023

andydotxyz commented Aug 5, 2023

mbaklor commented Aug 5, 2023

mbaklor commented Aug 9, 2023

Jacalz commented Aug 15, 2023

mbaklor commented Aug 15, 2023

andydotxyz left a comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Jacalz Aug 18, 2023

Choose a reason for hiding this comment

Jacalz commented Aug 20, 2023

mbaklor commented Aug 20, 2023

Jacalz commented Aug 20, 2023

mbaklor commented Aug 20, 2023

Jacalz left a comment

Choose a reason for hiding this comment

mbaklor commented Aug 2, 2023 •

edited by Jacalz

Loading

coveralls commented Aug 2, 2023 •

edited

Loading

Jacalz Aug 2, 2023 •

edited

Loading

Jacalz Aug 18, 2023 •

edited

Loading