Vojtech Szocs has posted comments on this change.
Change subject: webadmin,userportal: Improve TextBoxLabelBase
......................................................................
Patch Set 1:
(2 comments)
....................................................
Commit Message
Line 32: using Renderer<? super T> instead of just Renderer<T>
Line 33: at runtime.
Line 34:
Line 35: (Makes me wonder why GWT ValueBoxBase doesn't work
Line 36: with Renderer<? super T> just like GWT ValueLabel.)
This is an excellent point.
> I can't imagine why Parser has to be Covariant and Renderer has to be
> Contravariant.
In GWT class ValueBoxBase<T>, both renderer and parser use <T> [covariance]
In GWT class ValueLabel<T>, there's only renderer which uses <? super T>
[contravariance]
The advantage of contravariance (I think) is flexibility, i.e. both
ValueLabel<Integer> and ValueLabel<Double> can use the same Renderer<Number>
instance.
On the other hand, covariance tends to do the opposite, i.e. reduce flexibility
in favor of more strict typing rules.
Practically speaking, this patch was meant to add renderer contravariance to
avoid typecasts like this:
// NullableNumberTextBoxLabel<T extends Number>
// NullableNumberRenderer extends Renderer<Number>
public NullableNumberTextBoxLabel() {
super((Renderer<T>) new NullableNumberRenderer());
}
Theoretically speaking, I would say that:
* renderer contravariance - isn't necessarily a bad thing, i.e. it allows us
more flexibility in how we want to render the value
* parser contravariance - IMHO *is* a bad thing, i.e. widget working with type
T must be able to parse T so we need covariance here [and this patch preserves
it]
* renderer & parser serve different logical purposes
> Does it mean, renderer of super-type can render sub-types?
Exactly, that's the flexibility of contravariance.
As I wrote above, IMHO for renderer it makes sense but for parser it doesn't.
What do you think Kanagaraj?
Line 37:
Line 38: Change-Id: Ie691685ec6e9e1830da1457a0747901ba833be5d
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/label/BooleanTextBoxLabel.java
Line 10:
Line 11: public BooleanTextBoxLabel(String trueText, String falseText) {
Line 12: super(new BooleanRenderer(trueText, falseText));
Line 13: }
Line 14:
Right, accidental white space :-)
--
To view, visit http://gerrit.ovirt.org/22236
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie691685ec6e9e1830da1457a0747901ba833be5d
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: anmolbabu <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
