-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[nodes] ExpressionTextField improvements #2892
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2892 +/- ##
========================================
Coverage 79.41% 79.41%
========================================
Files 51 51
Lines 6987 6987
========================================
Hits 5549 5549
Misses 1438 1438 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the clean-up comments, I have a few notes:
mathEvaluator.js(in meshroom/ui/qml/Utils) can be removed from the project since it is (unless I missed something) now fully replaced, and references to it in theqmldircan also be removed.getEvalExpressionis called when the value is validated, when the focus on the text field is lost or when the expression has just been validated. For example, setting the value "10*2" then pressing "Enter" then moving to another attribute will trigger the call 3 times: once with "10*2", immediately after with "20" and then again with "20". The beginning and end call are necessary, but is it avoidable to retrigger the evaluation as soon as it has ended?
Otherwise, I do not have any functional notes, everything is clean, the code is actually simpler than it was in the initial version, so bar the tiny comments, it's ready to be merged.
962b16b to
9e85f17
Compare
I removed one occurence by checking if the text has changed since last evaluation, I'm not sure if it's better but let me know what you think! |
b0f379d to
0291cb9
Compare
cbentejac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a regression that has probably been introduced with #2836: when editing a text field, not pressing "Enter" and clicking somewhere else (thus losing focus), the value of the field is lost (the attribute is then set with value "0"). This wasn't the case in Meshroom 2025.1.0: when losing the focus, the value was still validated and applied to the attribute.
The Component.onCreated slot should also be removed, as it performs an evaluation right after the one triggered by onTextChanged while there could not possibly have been a value change.
0291cb9 to
0450ceb
Compare
The best way I found for now was to call |
0450ceb to
5c650c9
Compare
Description