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

DragValue: update value on each key press by default #2880

Merged
merged 4 commits into from
Aug 9, 2023
Merged

DragValue: update value on each key press by default #2880

merged 4 commits into from
Aug 9, 2023

Conversation

Barugon
Copy link
Contributor

@Barugon Barugon commented Apr 6, 2023

Closes #2818.

Can be changed with DragValue::update_while_editing.

Use Response::changed instead of Response::lost_focus (which is problematic).

Copy link

@paul-hansen paul-hansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does mean that it will change the value as you are typing now, which essentially reverts what #2687 wanted. Pinging @harrisonmg to give them a chance to comment.

I would prefer the way this PR has it anyway. Updating the value on every keypress allows you to see the change happening in real time as you type for quickly previewing small adjustments, otherwise you have to press enter and re-focus it every time you want to preview the change. Think about positioning objects precisely in a 3d scene. So I think this is better anyway.

You can always make your own temporary variable and only update it when focus is lost to recreate the functionality #2687 wanted:

// see the hello_world example for the where self.age would come from
let mut temp_value =
    ui.data_mut(|data| *data.get_temp_mut_or(Id::new("my_temp"), self.age));
let response = ui.add(egui::DragValue::new(&mut temp_value));
if response.changed() {
    ui.data_mut(|data| data.insert_temp(Id::new("my_temp"), temp_value));
}
if response.lost_focus() {
    self.age = temp_value;
}

Obviously would want to wrap that in a function or your own widget if you use it a lot.

tldr; I approve of this PR's change, but it should be noted that it is changing the functionality back closer to what it was, but I think it's for the better anyway.

@Barugon
Copy link
Contributor Author

Barugon commented Apr 7, 2023

This does mean that it will change the value as you are typing now

Yes, it's kind of a middle ground. Previously, it would parse the text every frame. With this change, it would only do that work when the TextEdit changes. After looking extensively at the focus logic, it's not possible to use lost_focus as there are too many issues with it. The focus logic needs a lot of work to make it reliable enough before using lost_focus.

@Barugon
Copy link
Contributor Author

Barugon commented Apr 7, 2023

I prefer it updating while editing the value also.

@harrisonmg
Copy link
Contributor

In my case, I'm updating motor controller tunings, so this behavior presents a bit of an issue for that.

Seems like an option might be nice in the future.

@paul-hansen
Copy link

paul-hansen commented Apr 7, 2023

It sounds like the problem you want to solve is that updating your motor controller tunings too frequently causes problems with the motor. Personally I wouldn't see that as a problem that a UI library should be making changes to try to solve. Instead, you should use something like a debounce function when setting the motor's values from anywhere to prevent it from being updated more often than it can support. This would be more robust and would prevent things like users still being able to focus and unfocus inputs more quickly than is supported.

That said, if there's a way to add an option to update the value only on focused lost I wouldn't be opposed to it. It needs to work correctly when tabbing and unfocusing by clicking on other dragvalues/widgets though, which is hard to do because of how DragValue changes it's widget based on if it's focused or not, which causes checks using lost_focus in DragValue's internal logic to be inconsistent.

@Barugon
Copy link
Contributor Author

Barugon commented Apr 7, 2023

In my case, I'm updating motor controller tunings, so this behavior presents a bit of an issue for that.

Seems like an option might be nice in the future.

Is it possible, in your code, to update on lost_focus?

@harrisonmg
Copy link
Contributor

In my case, I'm updating motor controller tunings, so this behavior presents a bit of an issue for that.

Seems like an option might be nice in the future.

Is it possible, in your code, to update on lost_focus?

Maybe, I'll probably stick to an older egui version for now.

Specifically, my issue is that typing "100" results in the gain being set to 1, then 10, then 100. It's the order of magnitude changes, not the frequency.

Anyway, I do support this change especially if it improves consistency.

@emilk
Copy link
Owner

emilk commented Apr 18, 2023

I think we should add an option for this, so users can control what behavior they want. I definitely see the use in both

@harrisonmg
Copy link
Contributor

it's not possible to use lost_focus as there are too many issues with it

@Barugon could you give a few examples of when you found that this fails? I think it would be helpful to drive future improvements to the focus logic if need be.

@Barugon
Copy link
Contributor Author

Barugon commented Apr 18, 2023

it's not possible to use lost_focus as there are too many issues with it

@Barugon could you give a few examples of when you found that this fails? I think it would be helpful to drive future improvements to the focus logic if need be.

Edit the value and then click on another DragValue or press tab and your edits are lost.

There's a video in the issue linked above that shows the problem.

@harrisonmg
Copy link
Contributor

Ok, so nothing beyond what's described in #2142?

If so, seems like this option + fixing 2142 would be perfect for everyone

@Barugon
Copy link
Contributor Author

Barugon commented Apr 18, 2023

Ok, so nothing beyond what's described in #2142?

If so, seems like this option + fixing 2142 would be perfect for everyone

Loosing your edit due to tabbing or clicking on another DragValue (or TextEdit) are actually two separate issues that cause the same problem. The lost focus indication is not consistently updated.

@Barugon
Copy link
Contributor Author

Barugon commented Apr 18, 2023

In #2818 I found the culprit for clicking off but it doesn't fix tabbing.

@tajtiattila
Copy link

@Barugon This seems to fix the main issue in #2877, thank you!

@Barugon
Copy link
Contributor Author

Barugon commented May 29, 2023

@Barugon This seems to fix the main issue in #2877, thank you!

Unfortunately hasn't been merged though.

@Trivaxy
Copy link

Trivaxy commented Jun 12, 2023

Would be nice to see this merged. Some of my users complained that the way it is now is sort of unintuitive and annoying, especially since we're doing rapid data entry on many different DragValues where it's a cycle of "type into box -> click on the next one -> repeat"

@Barugon
Copy link
Contributor Author

Barugon commented Jun 12, 2023

@emilk
Are there more changes you require?

crates/egui/src/widgets/drag_value.rs Outdated Show resolved Hide resolved
@emilk emilk changed the title Fix DragValue editing DragValue: update value on each key press by default Aug 9, 2023
@emilk emilk added the egui label Aug 9, 2023
@emilk emilk merged commit f9f9abf into emilk:master Aug 9, 2023
18 of 19 checks passed
@emilk
Copy link
Owner

emilk commented Aug 9, 2023

Thanks!

@Barugon Barugon deleted the drag_value_editing_fix branch August 10, 2023 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DragValue edit is discarded when clicked off
6 participants