Vojtech Szocs has posted comments on this change.
Change subject: userportal,webadmin: moved duplicate UICommand creation to
static factory methods
......................................................................
Patch Set 14:
Hi Martin, sorry for replying so late.
Some comments:
> since while it's a strong reference from factory to something, in current
> constructs the factory instance is thrown away immediately, so it does not
> matter to what it points.
What I wanted to say is that we cannot enforce the above-mentioned use of such
factories globally across frontend codebase (create locally-scoped instance
that is thrown away) and therefore we're running a risk of potential memory
leaks due to "bad" use of such factories.
The factory should be designed in a way that even non-standard (or even "bad")
uses will not cause subsequent problems such as memory leaks.
> It's just a "little inefficiency" (and we're probably talking in tens of ns
> grade tops), which is tradeoff for OO design
Since this is GWT client code, that "little inefficiency" will vanish very
early - during GWT compilation phase. But I was concerned about problems caused
by "bad" uses of such factories rather than performance.
> Possible alterations: we should keep in mind, that we are able to, someday,
> support DI.
UiCommon models already support DI via GIN framework & Guice annotations.
You can look at specific model classes and see they have @Inject on their
constructor.
> ad adding "title" parameter to every method: I'm against these methods can
> serve any permutation of input; these methods should serve as a shortcut for
> *common* UICommand instantiations, it should not try to be able to create
> completely anything.
Agreed. Sensible shortcuts shouldn't attempt to cover all use cases.
> But I think we should define our intentions mode declaratively: "I want
> default ok button, give me one." instead of "give me a button, then make it
> default, and yes, also name it OK, alright?", which is longer, more error
> prone, harder to write and forces to copypasting.
Builder-like API ("create button, make it default, etc.") is fine-grained to
cover all use cases. Shortcut-like API can utilize builder-like API.
But in general I agree that declarative style ("I want default OK button") is
more readable and communicates the intention better.
> change the factory so it's not stateful ~ ie. add iCommandTarget to all
> methods. But those methods should not be static.
+1
The factory can be DI-managed and @Inject'ed by user code (typically inside
UiCommon models).
> If needed we can make UICommnand setters make to return this to resemble
> builder. This way we could craft all uncommon cases.
This would shorten the code needed to "prepare" the UICommnand instance (but
not much I think).
--
To view, visit http://gerrit.ovirt.org/33317
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0b3b1344b1a3819e58de4db5093106bedf5291e6
Gerrit-PatchSet: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Mucha <mmu...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
