Remove deprecated methods from 3.0 #7333
Conversation
project-bot
bot
added this to External Reviews
in OLD Vaadin Flow ongoing work (Vaadin 10+)
pleku
moved this from External Reviews
to Iteration Reviews
in OLD Vaadin Flow ongoing work (Vaadin 10+)
There was a problem hiding this comment.
Reviewed 23 of 23 files at r1.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/component/page/ExtendedClientDetails.java, line 383 at r1 (raw file):
VaadinSession.getCurrent().getBrowser().isIPhone();
That should include ipod according to the impl in the master branch for OperatingSystem.IOS https://github.com/vaadin/flow/blob/master/flow-server/src/main/java/com/vaadin/flow/shared/BrowserDetails.java#L237
Does it include it now ?
flow-server/src/main/java/com/vaadin/flow/dom/Element.java, line 71 at r1 (raw file):
private
Any specific reason?
Is it used in other place as well ?
I see some unsafety here: the field is immutable but the value is not.
So would be good to initialize the value to immutable value (and get rid of static block) : use a static method which returns the filled HashMap and wrap it using Collections.unmodifiableMap.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/component/page/ExtendedClientDetails.java, line 383 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
VaadinSession.getCurrent().getBrowser().isIPhone();That should include
ipodaccording to the impl in themasterbranch forOperatingSystem.IOShttps://github.com/vaadin/flow/blob/master/flow-server/src/main/java/com/vaadin/flow/shared/BrowserDetails.java#L237Does it include it now ?
Very good find. However, I think it is a fluke. I don't think we say anywhere that we would be supporting any iPods though.
I'll verify
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/component/page/ExtendedClientDetails.java, line 383 at r1 (raw file):
Previously, pleku (Pekka Hyvönen) wrote…
Very good find. However, I think it is a fluke. I don't think we say anywhere that we would be supporting any iPods though.
I'll verify
Done
flow-server/src/main/java/com/vaadin/flow/dom/Element.java, line 71 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
privateAny specific reason?
Is it used in other place as well ?
I see some unsafety here: the field is immutable but the value is not.
So would be good to initialize the value to immutable value (and get rid ofstaticblock) : use a static method which returns the filledHashMapand wrap it usingCollections.unmodifiableMap.
Done (was left by a mistake)
There was a problem hiding this comment.
Reviewed 22 of 22 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeFeatures.java, line 91 at r2 (raw file):
Quoted 5 lines of code…
public static final int SYNCHRONIZED_PROPERTIES = 13; /** * Id for {@link SynchronizedPropertyEventsList}. */ public static final int SYNCHRONIZED_PROPERTY_EVENTS = 14;
Technically this removal requires renumeration : COMPONENT_MAPPING is 15 now but should be 13.
I'm not sure how important it is though.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeFeatures.java, line 91 at r2 (raw file):
Previously, denis-anisimov (Denis) wrote…
public static final int SYNCHRONIZED_PROPERTIES = 13; /** * Id for {@link SynchronizedPropertyEventsList}. */ public static final int SYNCHRONIZED_PROPERTY_EVENTS = 14;Technically this removal requires renumeration : COMPONENT_MAPPING is 15 now but should be 13.
I'm not sure how important it is though.
I'm not sure, but I'm a bit afraid that it might break more existing tests on our side and potentially a lot of tests on customers side that for some reason look at the JSON messages.
As I don't know what changing this might cause, I would not just do it as there is practically no added value.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeFeatures.java, line 91 at r2 (raw file):
Previously, pleku (Pekka Hyvönen) wrote…
I'm not sure, but I'm a bit afraid that it might break more existing tests on our side and potentially a lot of tests on customers side that for some reason look at the JSON messages.
As I don't know what changing this might cause, I would not just do it as there is practically no added value.
Well, this is pure internal functionality. I'm pretty sure it can be changed safely without breaking anything.
The reason of my concern here: it might be that we have some assumptions about the index of the feature and their overall number.
So if we have 16 features e.g. but there is a feature whose index (id here) is 17 it may cause a failure.
But on the other hand, I think we should have tests for this. So if nothing failed then it should be OK.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained, and 1 stale
a discussion (no related file):
Blocking until #7326 is merged and this is rebased on top of it.
Removes or hides remaining API that has been made deprecated in 1.0-2.2. Only exclusion is @VaadinServletConfiguration, which is kept. Fixes #6510
|
SonarQube analysis reported 9 issues 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:
|
There was a problem hiding this comment.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale (waiting on @denis-anisimov)
There was a problem hiding this comment.
Reviewed 14 of 23 files at r1, 21 of 22 files at r2, 4 of 4 files at r3, 3 of 3 files at r4, 3 of 3 files at r5, 16 of 16 files at r6.
Reviewable status:
There was a problem hiding this comment.
Just a note :
getElement().addPropertyChangeListener("value", event, e -> {
});
getElement().addEventListener(event, e -> {
label.setText(
"Server value: " + getElement().getProperty("value"));
});
Looks weird because it's not clear why do we synchronize the "value" property but there is no any listener for it.
The whole idea to have the new method addPropertyChangeListener (with a new signature) was to have a synchronized property along with the listener for it right away (so they are attached).
But here the event listener is added instead of having the property listener .
So the event listener code could have been removed and instead used in the property listener.
It's more a note for myself which explains the unclear usage of empty property listeners.
Reviewed 3 of 3 files at r4, 2 of 3 files at r5, 7 of 16 files at r6.
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 3 files at r5, 9 of 16 files at r6.
Reviewable status:
Removes or hides remaining API that has been made deprecated in 1.0-2.2.
Only exclusion is @VaadinServletConfiguration, which is kept.
Part of #6510
This change is