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

pass previous value to controller.onChange #124

Closed
wants to merge 2 commits into from

Conversation

Sceat
Copy link

@Sceat Sceat commented Nov 12, 2023

This PR adds the ability to retrieve the previous value in the onChange callback, it's a must have in any watcher system and allows side effects based on the update.

It also remove useless updates caused by the click event and only call onChange if the value really changed.

This PR is a bit quick so it would be nice to point me in the correct direction to also updates any typing/doc/comment to make the PR receivable.


gui
  .add(settings, 'count')
  .name('Count')
  .onChange((count, previous_count) => {
    handle(count)
	if(count > previous_count) console.log('bigger count!')
  })

@georgealways
Copy link
Owner

Hi there, looks like there's two things going on:

It also remove useless updates caused by the click event and only call onChange if the value really changed.

The only controller that should generate an onChange event on click is the function (button) controller. If you're experiencing that someplace else too then please file an issue!

As for passing the previous value to onChange: I'm open to it, but I'd want to receive a few more requests before adding the feature. There's some complexity here too when it comes to color objects and arrays: i.e. should the previous value be deep cloned and stored?

Thanks for this! -g

@Sceat
Copy link
Author

Sceat commented Nov 13, 2023

Yes actually when you drag a slider it triggers the onChange endlessly even when steps don't move

@Sceat
Copy link
Author

Sceat commented Nov 13, 2023

There's some complexity here too when it comes to color objects and arrays: i.e. should the previous value be deep cloned and stored?

to me it should be the exact same value, and we let it be garbage collected

@georgealways georgealways changed the base branch from master to dev November 13, 2023 13:31
@georgealways georgealways changed the title Allows retrieval of previous value and call onChange only if value really changed pass previous value to controller.onChange Nov 13, 2023
@Sceat
Copy link
Author

Sceat commented Nov 15, 2023

Should we move forward on this ?, we could simply have the previous value passed and not the part on multiple updates. I could make 2 separated PRs

regarding cloning, I'd advise against it to stay transparent and let the user handle his code

@Sceat
Copy link
Author

Sceat commented Nov 29, 2023

@georgealways any blocker ?

@georgealways
Copy link
Owner

  • A fix for stepped number controllers generating extraneous onChange events is on the list for the next release.
  • I'd like some more input from others on how prevValue should behave.

I haven't had a chance to look at either of these closely, and I can't make any promises as to when I will. If these are changes you need quickly, I'd recommend using a fork for the time being.

Thanks!

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