flow icon indicating copy to clipboard operation
flow copied to clipboard

feat:Improve Binder change detection

Open onuridrisoglu opened this issue 1 year ago • 6 comments

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.

onuridrisoglu avatar May 31 '24 10:05 onuridrisoglu

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 31 '24 10:05 CLAassistant

CLA assistant check
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.

CLAassistant avatar May 31 '24 10:05 CLAassistant

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.

github-actions[bot] avatar May 31 '24 10:05 github-actions[bot]

@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?

tepi avatar Jun 10 '24 05:06 tepi

@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?

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 avatar Jun 25 '24 13:06 mshabarov

@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?

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.

I did the changes, however my only concern is now withEqualityPredicate should be called for each binding in order to activate this feature.

onuridrisoglu avatar Aug 07 '24 09:08 onuridrisoglu

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).

mshabarov avatar Aug 23 '24 06:08 mshabarov

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

tepi avatar Aug 23 '24 08:08 tepi