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

Add primitive default data support on evendata #4132

Merged
merged 4 commits into from
May 24, 2018
Merged

Conversation

mikotin
Copy link
Contributor

@mikotin mikotin commented May 18, 2018

Added primitive checking on event data value decoding, so that primitive values get default values instead of null.

Fixes issue #4112


This change is Reviewable

Added primitive checking on event data value decoding, so that primitive values get default values instead of null.

Fixes issue #4112
@CLAassistant
Copy link

CLAassistant commented May 18, 2018

CLA assistant check
All committers have signed the CLA.

@denis-anisimov
Copy link
Contributor

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 26 at r1 (raw file):

import javafx.util.Pair;

Is javafx part of JDK nowadays ?
Not sure that this is good to use javafx in the code which should work without it just because of this class.


flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBusUtil.java, line 108 at r1 (raw file):

                    new Pair<>(ReflectTools.convertPrimitiveType(p.getType()),
                            p.getType().isPrimitive()));

Are you sure that you need boolean in the Pair instance and Pair instance at all if you may ask p.getType.isPrimitive() for the type which may be asked directly for isPrimitive() ?

May be do not call ReflectTools.convertPrimitiveType() here but do it where the method is called since you anyway have changed the return value ?


flow-server/src/main/java/com/vaadin/flow/internal/JsonCodec.java, line 233 at r1 (raw file):

    public static <T> T decodeAs(JsonValue json, Class<T> type, boolean allowNull) {
        assert json != null;

What does it return if null is not allowed ?

Is it possible do not create a new method but reuse existing one and check isPrimitive here with the proper logic ?


Comments from Reviewable

@Legioth
Copy link
Member

Legioth commented May 18, 2018

Review status: all files reviewed at latest revision, 3 unresolved discussions.


flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 26 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
import javafx.util.Pair;

Is javafx part of JDK nowadays ?
Not sure that this is good to use javafx in the code which should work without it just because of this class.

javafx will be dropped in Java 11.


Comments from Reviewable

Removed Pair -usage and moved primitive casting and checking
prior to decode -call
@mikotin
Copy link
Contributor Author

mikotin commented May 23, 2018

Review status: 3 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.


flow-server/src/main/java/com/vaadin/flow/internal/JsonCodec.java, line 233 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
    public static <T> T decodeAs(JsonValue json, Class<T> type, boolean allowNull) {
        assert json != null;

What does it return if null is not allowed ?

Is it possible do not create a new method but reuse existing one and check isPrimitive here with the proper logic ?

If null is not allowed and value is null, then it will fall back to get default data, like false for boolean / Boolean.

Can't really do all the conversion logic within the decodeAs -method, for it would either need:

  • casting to primitive value, which will throw something like: java.lang.ClassCastException: Cannot cast java.lang.Integer to int
  • Solving the non-primitive class from primitive class and returning that, but that it is against generics of the method

Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Review status: 3 of 5 files reviewed at latest revision, 2 unresolved discussions.


flow-server/src/main/java/com/vaadin/flow/internal/JsonCodec.java, line 233 at r1 (raw file):

casting to primitive value, which will throw something like: java.lang.ClassCastException: Cannot cast java.lang.Integer to int

of course

Solving the non-primitive class from primitive class and returning that, but that it is against generics of the method

Why ? (ReflectTools.convertPrimitiveType )
The method returns a value.
A primitive value is always castable to its boxed type. So I don't see any issue with that.
You will need to cast the result to T (with a warning from a compiler) but that should be fine, isn't it ?


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

- Added primitives support toJsonCodec.decodeAS
- Added tests for primitives
@mikotin
Copy link
Contributor Author

mikotin commented May 24, 2018

Review status: 3 of 6 files reviewed at latest revision, 1 unresolved discussion.


flow-server/src/main/java/com/vaadin/flow/internal/JsonCodec.java, line 233 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

casting to primitive value, which will throw something like: java.lang.ClassCastException: Cannot cast java.lang.Integer to int

of course

Solving the non-primitive class from primitive class and returning that, but that it is against generics of the method

Why ? (ReflectTools.convertPrimitiveType )
The method returns a value.
A primitive value is always castable to its boxed type. So I don't see any issue with that.
You will need to cast the result to T (with a warning from a compiler) but that should be fine, isn't it ?

Changed as requested, also added tests for new supported types


Comments from Reviewable

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 1 issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MINOR ComponentEventBus.java#L129: Remove useless curly braces around statement rule

@denis-anisimov
Copy link
Contributor

:lgtm:


Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@denis-anisimov denis-anisimov merged commit d39f957 into master May 24, 2018
@denis-anisimov denis-anisimov deleted the 4112-event-data branch May 24, 2018 10:17
@denis-anisimov denis-anisimov added this to the 1.0.0.rc1 milestone May 30, 2018
mshabarov pushed a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132
vaadin-bot pushed a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132
vaadin-bot pushed a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132
vaadin-bot pushed a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132
vaadin-bot pushed a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132
vaadin-bot pushed a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132
vaadin-bot added a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132

Co-authored-by: Ugur Saglam <[email protected]>
vaadin-bot added a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132

Co-authored-by: Ugur Saglam <[email protected]>
vaadin-bot added a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132

Co-authored-by: Ugur Saglam <[email protected]>
caalador pushed a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132

Co-authored-by: Ugur Saglam <[email protected]>
vaadin-bot added a commit that referenced this pull request Nov 29, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132

Co-authored-by: Ugur Saglam <[email protected]>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.2.11.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 22.0.25.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 14.9.3.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.0.0.alpha6 and is also targeting the upcoming stable 24.0.0 version.

MarcinVaadin pushed a commit that referenced this pull request Dec 21, 2022
For hierarchical data, even when the child components are filtered out, the clear method on ArrayUpdater.Update in HierarchicalCommunicationController is never called. This is caused by an if check, in which the start index of previousActive is compared to the newly calculated assumedSize. Since there might be items to clear, the newly calculated size should have no effect on whether we clear the items or not. This check is not present in the DataCommunicator counterpart within the method collectChangesToSend.

This PR removes the aforementioned clear condition. Add test to ensure that the expanded children are cleared properly when filtered out.

Fixes #4132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants