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

Add an event type like html's change event? #702

Open
AmaiKinono opened this issue Oct 18, 2024 · 5 comments
Open

Add an event type like html's change event? #702

AmaiKinono opened this issue Oct 18, 2024 · 5 comments
Labels
discussion Meta talk and feedback enhancement New feature or request

Comments

@AmaiKinono
Copy link
Contributor

The change event we have for now seems to be more close to the input event in html. This MDN doc explains the difference between input and change:

The input event is fired every time the value of the element changes. This is unlike the change event, which only fires when the value is committed, such as by pressing the enter key or selecting a value from a list of options.

This describes when is change event fired.

I'm thinking whether we should have an event type like change in html, which is useful at times, e.g., to normalize user input, to record an undo checkpoint, etc.

For range sliders, I think (in html) the input event is fired during dragging, and change is fired upon releasing the mouse.

@mikke89 mikke89 added enhancement New feature or request discussion Meta talk and feedback labels Oct 19, 2024
@mikke89
Copy link
Owner

mikke89 commented Oct 19, 2024

Yeah, I agree. Ideally, we would use the same behavior as in HTML, add a new input event for the current behavior, and make change only trigger when committing the value.

I would also add to this that we should only emit any of these events when the user initiates the change, not when changing it programmatically. I believe that is how HTML behaves, and it might also solve some binding loops like in #701, and events being emitted upon initially setting the values.

This would be a breaking change though. Although, I'm not sure how bad of a breaking change it would be, maybe in many/most causes it would still work fine. Needs some testing I think to understand the implications. It might be we'll have to wait to merge it until the next major version change.

@AmaiKinono
Copy link
Contributor Author

we should only emit any of these events when the user initiates the change, not when changing it programmatically.

I'm looking into this, as it seems to be better done before we add the HTML change event (for now I think we could have a name like changeend. We can rename it in the next major release).

I plan to change Core/Elements/ElementFormControl.h:

  • Add a second argument to the virtual method SetValue which is a flag of whether the change comes from the user, and it can have a default value of false.
  • Change SetValue implementations of specific form controls, and modify its call sites when suitable.

Does this look reasonable to you?

@mikke89
Copy link
Owner

mikke89 commented Oct 25, 2024

Yeah, it generally sounds reasonable to me.

I do wonder though if, or to what extent, the emit only on user interaction is a breaking change.

Minor design input: I would prefer a strong enum over a bool. Also, I think it's not ideal to have virtual functions with defaulted parameters. Consider using a protected virtual "impl" function with no defaults, and a public one with the default parameter. Not sure if we actually need to publicly expose the second parameter to users though?

@AmaiKinono
Copy link
Contributor Author

Not sure if we actually need to publicly expose the second parameter to users though?

I did my research. In HTML and GTK, APIs like SetValue in RmlUi does not trigger events (or signals). The programmer can manually trigger an event. This has the advantage of 1. not emitting too many signals for repeated programmatic change, and 2. being more flexible for users.

In Qt, user input and programmatic change both trigger the signals. To prevent programatic change from triggering any signal, we could attach a QSignalBlocker to the widget before the change, and remove it after that.

So we could take one of these 2 ways, and exposing the second parameter is not necessary. Personally I think the HTML/GTK way is better, as there seem to be no convenient place to put an "event blocker" for updating data views.

So,

I do wonder though if, or to what extent, the emit only on user interaction is a breaking change.

I think it's more like a "fix" of incomplete design, by which I mean offer the option of not triggering event for programmatic change. I can see why it's still a breaking change, but it's not a big one.

@mikke89
Copy link
Owner

mikke89 commented Oct 28, 2024

Thanks for doing the research! Yeah, generally, we should follow the HTML behavior unless we have good reasons not to. I think providing SetValue, and then simply let people manually submit events sounds good to me.

I am still not sure whether it's okay to do this in a minor version vs a major version update. I guess one indicator is whether we need to make some changes to our samples as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Meta talk and feedback enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants