flow
flow copied to clipboard
feat:Improve Binder change detection
Description
Fixes #19260
Type of change
- [ ] Bugfix
- [x] Feature
Checklist
- [ ] I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
- [ ] I have added a description following the guideline.
- [ ] The issue is created in the corresponding repository and I have referenced it.
- [ ] I have added tests to ensure my change is effective and works as intended.
- [ ] New and existing tests are passing locally with my change.
- [ ] I have performed self-review and corrected misspellings.
Additional for Feature type of change
- [ ] Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
Test Results
1 132 files ±0 1 132 suites ±0 1h 28m 45s :stopwatch: - 1m 7s 7 356 tests +3 7 306 :white_check_mark: +3 50 :zzz: ±0 0 :x: ±0 7 717 runs - 6 7 657 :white_check_mark: - 6 60 :zzz: ±0 0 :x: ±0
Results for commit 326ebf54. ± Comparison against base commit 999db1c7.
:recycle: This comment has been updated with latest results.
@mshabarov This change will introduce a slight change in behavior of Binder change tracking due to requiring equals to be implemented properly by value types. As well as reverting the tracked change if the value is reverted, which is not currently done (that is, the binding is considered as changed anyway). Do you think we'll need an enable/disable switch for this new change tracking?
Quality Gate passed
Issues
2 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code
@mshabarov This change will introduce a slight change in behavior of Binder change tracking due to requiring
equalsto be implemented properly by value types. As well as reverting the tracked change if the value is reverted, which is not currently done (that is, the binding is considered as changed anyway). Do you think we'll need an enable/disable switch for this new change tracking?
I'd propose to set the equality predicate null by default (or some other value that marks it not defined), that means the change detection feature is not enabled by default. Then if this predicate is set by BindingBuilder::withEqualityPredicate, then this feature is enabled. In this way this would be backwards compatible.
@mshabarov This change will introduce a slight change in behavior of Binder change tracking due to requiring
equalsto be implemented properly by value types. As well as reverting the tracked change if the value is reverted, which is not currently done (that is, the binding is considered as changed anyway). Do you think we'll need an enable/disable switch for this new change tracking?I'd propose to set the equality predicate
nullby default (or some other value that marks it not defined), that means the change detection feature is not enabled by default. Then if this predicate is set byBindingBuilder::withEqualityPredicate, then this feature is enabled. In this way this would be backwards compatible.
I did the changes, however my only concern is now withEqualityPredicate should be called for each binding in order to activate this feature.
Just discussed with @tepi, the Binder-level API could be done in a way:
- Adding a method on Binder's level that enables/disables usage of these predicates on bindings level.
- Then, if enabled, Binder will use a configured predicate for a binding, if it exists, otherwise it uses
equalsas a fallback. If disabled, nothing is used (default).
Just discussed with @tepi, the Binder-level API could be done in a way:
1. Adding a method on Binder's level that enables/disables usage of these predicates on bindings level. 2. Then, if enabled, Binder will use a configured predicate for a binding, if it exists, otherwise it uses `equals` as a fallback. If disabled, nothing is used (default).
@onuridrisoglu fyi, I'll pick this up and do the changes
Quality Gate passed
Issues
7 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code