feat: allow disabling validation status change listener registration on binder #14158

knoobie · 2022-07-11T07:49:28Z

Description

Add binder-level flag to allow disabling of
validation status change listener registration
for HasValidator fields.

Related to #13940 (comment) and #13940 (comment)

Type of change

Bugfix
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 a 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.

mcollovati · 2022-07-11T12:38:12Z

Hi @knoobie, thanks for the contribution.
Do you mind open an issue that explains the use case for the change?

knoobie · 2022-07-11T12:43:03Z

@mcollovati Based on my discussion here: vaadin/flow-components#3406 (comment) and my comment here: #13940 (comment)

Idea behind this is: first enhancement to allow this change to be toggled by a FeatureFlag. I would love to gradually test this change with multiple applications before I do a big flip on all our forms and applications in a minor release (23.2) and going crazy with problems regarding "not meaningful error messages" and "user has no idea why the field is red".

mcollovati · 2022-07-11T12:50:40Z

@knoobie thanks! Now the context is clear also on the PR

github-actions · 2022-07-11T13:03:21Z

Unit Test Results

  902 files ±0   902 suites ±0 55m 57s ⏱️ + 8m 32s
5 886 tests +2 5 842 ✔️ +1 44 💤 +1 0 ❌ ±0
6 091 runs +5 6 041 ✔️ +1 50 💤 +4 0 ❌ ±0

Results for commit c2981cb. ± Comparison against base commit 65e9ef7.

♻️ This comment has been updated with latest results.

mcollovati · 2022-07-14T12:30:54Z

LGTM, but I would like for one additional review

I wonder if it would be good to be able to enable/disable the feature also for a single Binding

knoobie · 2022-07-14T12:37:17Z

LGTM, but I would like for one additional review

You okay with the name? It was really hard to come up with something.. and even that name feels clumsy..

I wonder if it would be good to be able to enable/disable the feature also for a single Binding

I had the feeling as well, but wanted to get something out first ;) I had multiple questions as well, but neglected them all in my first throw to make it simple. Should this not stop the listener registration and instead stop the execution of the validate() call.. allowing this to be changed on the fly, instead of only once per binder and so on..

mcollovati · 2022-07-14T13:14:40Z

You okay with the name? It was really hard to come up with something.. and even that name feels clumsy..

I agree that the name feels clumsy. I tried to find an alternative but was unsuccessfully. That's why I would like for another review.

mcollovati · 2022-07-14T13:15:36Z

Should this not stop the listener registration and instead stop the execution of the validate() call.. allowing this to be changed on the fly, instead of only once per binder and so on

Interesting point

taefi

@knoobie the whole thing sounds like a good feature! Thanks for the contribution. Just suggested some changes.

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

taefi · 2022-07-20T13:11:59Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

@@ -1685,6 +1687,8 @@ void setIdentity() {

    private boolean validatorsDisabled = false;

+    private boolean validationStatusChangeFieldListenerDisabled = false;


I suggest renaming this field to something like fieldsValidationStatusChangeListenerDisabled, and also the associated getter/setter/javadocs/tests/etc.
To me, It would sound even better and easier to understand if we negate the name and the default value, e.g. we can call it:
private boolean fieldsValidationStatusChangeListenerEnabled = true; instead, so that it can be used without a !.

I can go for the name you prefer :) I used the suffix Disabled to match with the validatorsDisabled above to keep the code the same

I noticed the uniformity of naming, but I guess it is better to reduce the negations in naming and also the logic. The other variables also may experience refactoring at some point in the future to simplify the logic ;)

… change listener registration

sonarqubecloud · 2022-07-20T19:37:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

knoobie · 2022-07-21T08:13:47Z

Reminder: that the PR description probably has to be updated to something more meaningful, before a release is pushed with this encrypted message

knoobie · 2022-07-21T11:33:23Z

@taefi I think there should be no need to backport this to other branches, as the HasValidator interface is only used / implemented in V23.2 components (Edit: don't mind this comment, if you wanna make sure other binder changes can be automatically cherry picked later on)

vaadin-bot · 2022-07-21T12:24:35Z

Hi @knoobie and @taefi, when i performed cherry-pick to this commit to 2.8, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 420498c
error: could not apply 420498c... feat: allow disabling validation status change listener registration on binder
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

vaadin-bot · 2022-07-21T12:24:46Z

Hi @knoobie and @taefi, when i performed cherry-pick to this commit to 2.7, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 420498c
error: could not apply 420498c... feat: allow disabling validation status change listener registration on binder
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

…on binder (#14158) (CP: 9.0) (#14199) Add binder-level flag to allow disabling of validation status change listener registration for HasValidator fields. Related to #13940 (comment) and #13940 (comment) Co-authored-by: Knoobie <[email protected]> Co-authored-by: Marco Collovati <[email protected]>

mcollovati added the Contribution PRs coming from the community or external to the team label Jul 11, 2022

mcollovati self-assigned this Jul 11, 2022

vaadin-bot added the +0.1.0 label Jul 11, 2022

mcollovati requested a review from taefi July 20, 2022 07:44

taefi requested changes Jul 20, 2022

View reviewed changes

feat: add binder-level flag to disable HasValidator validation status…

c2981cb

… change listener registration

mcollovati approved these changes Jul 21, 2022

View reviewed changes

taefi approved these changes Jul 21, 2022

View reviewed changes

taefi added need to pick manually 2.7 need to pick manually 9.0 need to pick manually 2.8 target/23.0 labels Jul 21, 2022

taefi changed the title ~~feat: add binder-level flag to disable HasValidator validation status change listener registration~~ feat: allow disabling validation status change listener registration on binder Jul 21, 2022

taefi added target/2.7 and removed need to pick manually 2.7 need to pick manually 9.0 need to pick manually 2.8 labels Jul 21, 2022

taefi added the target/2.8 label Jul 21, 2022

taefi merged commit 420498c into vaadin:master Jul 21, 2022

vaadin-bot added cherry-picked-23.1 cherry-picked-23.0 need to pick manually 2.8 labels Jul 21, 2022

vaadin-bot added cherry-picked-9.0 need to pick manually 2.7 labels Jul 21, 2022

taefi added cherry-picked-2.7 cherry-picked-2.8 and removed need to pick manually 2.7 need to pick manually 2.8 labels Jul 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow disabling validation status change listener registration on binder #14158

feat: allow disabling validation status change listener registration on binder #14158

knoobie commented Jul 11, 2022 •

edited by taefi

Loading

mcollovati commented Jul 11, 2022

knoobie commented Jul 11, 2022

mcollovati commented Jul 11, 2022

github-actions bot commented Jul 11, 2022 •

edited

Loading

mcollovati commented Jul 14, 2022

knoobie commented Jul 14, 2022

mcollovati commented Jul 14, 2022

mcollovati commented Jul 14, 2022

taefi left a comment

taefi Jul 20, 2022

knoobie Jul 20, 2022

taefi Jul 20, 2022

sonarqubecloud bot commented Jul 20, 2022

knoobie commented Jul 21, 2022

knoobie commented Jul 21, 2022 •

edited

Loading

vaadin-bot commented Jul 21, 2022

vaadin-bot commented Jul 21, 2022

		@@ -1685,6 +1687,8 @@ void setIdentity() {

		private boolean validatorsDisabled = false;

		private boolean validationStatusChangeFieldListenerDisabled = false;

feat: allow disabling validation status change listener registration on binder #14158

feat: allow disabling validation status change listener registration on binder #14158

Conversation

knoobie commented Jul 11, 2022 • edited by taefi Loading

Description

Type of change

Checklist

Additional for Feature type of change

mcollovati commented Jul 11, 2022

knoobie commented Jul 11, 2022

mcollovati commented Jul 11, 2022

github-actions bot commented Jul 11, 2022 • edited Loading

Unit Test Results

mcollovati commented Jul 14, 2022

knoobie commented Jul 14, 2022

mcollovati commented Jul 14, 2022

mcollovati commented Jul 14, 2022

taefi left a comment

Choose a reason for hiding this comment

taefi Jul 20, 2022

Choose a reason for hiding this comment

knoobie Jul 20, 2022

Choose a reason for hiding this comment

taefi Jul 20, 2022

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 20, 2022

knoobie commented Jul 21, 2022

knoobie commented Jul 21, 2022 • edited Loading

vaadin-bot commented Jul 21, 2022

vaadin-bot commented Jul 21, 2022

knoobie commented Jul 11, 2022 •

edited by taefi

Loading

Additional for `Feature` type of change

github-actions bot commented Jul 11, 2022 •

edited

Loading

knoobie commented Jul 21, 2022 •

edited

Loading