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

Don't call OnScrolled if offset did not change #2646

Merged
merged 5 commits into from Dec 21, 2021

Conversation

andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Nov 15, 2021

Fixes #1868

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass. <- this is super strange... can't figure out why just yet

@andydotxyz
Copy link
Member Author

I meant this to be a draft PR sorry. I cannot figure out yet why the visual tests fail just because we are not triggering on duplicate OnScroll calls

@Jacalz
Copy link
Member

Jacalz commented Dec 14, 2021

Have you managed to find the source for the error?

@andydotxyz
Copy link
Member Author

Have you managed to find the source for the error?

I'm getting there. Lots of widgets just worked off this false assumption.
I have found and resolved 2 issues. At least one more remains.

@andydotxyz
Copy link
Member Author

Finally got to the bottom of it :)
It was 2 items in general

*) Calling Scroller.Refresh() no longer calls OnScrolled, so we may have to refresh the content directly if required
*) The setting of Scroller.Offset and then calling Refresh() does not call OnScrolled which makes sense. It previously did.

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.

Hmm, I think there might be something wrong here. When adding a few more tabs to the DocTabs in fyne_demo, the scroller jumps back to the start after a while of dragging it to the right.

I would go about replicating it like this:

  • Go to the DocTabs section in fyne_demo.
  • Press the "+" button to add a few tabs and thus get a scroller.
  • Grab hold of the scroller with the mouse and drag it to the right.
  • About 2/3 on the way to the right, the scroller jumps back to the start again.

@andydotxyz
Copy link
Member Author

Hmm, I think there might be something wrong here. When adding a few more tabs to the DocTabs in fyne_demo, the scroller jumps back to the start after a while of dragging it to the right.

This is a good catch thanks @Jacalz, however the issue is on develop too, so not part of the changes here.

@Jacalz
Copy link
Member

Jacalz commented Dec 20, 2021

Hmm, I think there might be something wrong here. When adding a few more tabs to the DocTabs in fyne_demo, the scroller jumps back to the start after a while of dragging it to the right.

This is a good catch thanks @Jacalz, however the issue is on develop too, so not part of the changes here.

Are you sure? I tested on develop and couldn't quite replicate it. I'll have to try again, I guess.

@andydotxyz
Copy link
Member Author

Are you sure? I tested on develop and couldn't quite replicate it. I'll have to try again, I guess.

Quite sure - it seems to glitch when it's about to show the selected tab when scrolling.

Jacalz
Jacalz previously approved these changes Dec 21, 2021
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.

You are entirely correct. Sorry for the fuss.

@andydotxyz andydotxyz changed the base branch from release/v2.1.x to develop December 21, 2021 16:33
@andydotxyz andydotxyz dismissed Jacalz’s stale review December 21, 2021 16:33

The base branch was changed.

@andydotxyz
Copy link
Member Author

Not a problem thanks @Jacalz.
I decided that this was a behaviour change so did not belong on a bugfix release (given the changes we had to make internally).
If you would be OK to re-approve with this change I would be appreciative :)

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.

Looks good twice :)

@andydotxyz andydotxyz merged commit 98314c2 into fyne-io:develop Dec 21, 2021
@andydotxyz andydotxyz deleted the fix/1868 branch December 21, 2021 16:56
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.

None yet

2 participants