Vojtech Szocs has posted comments on this change.
Change subject: userportal,webadmin: moved duplicate UICommand creation to
Factory
......................................................................
Patch Set 9:
(2 comments)
Hi Martin, good patch in general (reducing code duplication), below are my
ideas.
First, creating factory instance on every single use is generally problematic
and might lead to memory leaks.
For example:
new UICommandFactory(this).createOkUiCommand("OnSave");
UICommandFactory now holds strong reference to model ("this") and for
short-lived (dialog etc.) models, might prevent the model to be garbage
collected.
If you want to use factory, I'd rather make the factory stateless - make
UICommandFactory methods static. Stateless factories are always better (safer)
than stateful factories.
Second, instead of introducing UICommandFactory, we can improve UICommand
itself with friendly API.
For example:
// inside UICommand class
private final UIConstants uiConstants =
ConstantsManager.getInstance().getConstants();
public UICommand asDefaultCommand() {
return asDefaultCommand(uiConstants.ok());
}
public UICommand asDefaultCommand(String title) {
setIsDefault(true);
setTitle(title);
return this;
}
User code:
UICommand defaultCommand = new UICommand("OnSave").asDefaultCommand();
UICommand defaultCommandWithCustomTitle = = new
UICommand("OnReset").asDefaultCommand(customTitle);
Personally I'm in favor of improving existing UICommand class. What do you
think?
http://gerrit.ovirt.org/#/c/33317/9/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UICommandFactory.java
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UICommandFactory.java:
Line 1: package org.ovirt.engine.ui.uicommonweb;
Line 2:
Line 3: import org.ovirt.engine.ui.uicompat.ConstantsManager;
Line 4:
Line 5: public class UICommandFactory {
All "createXxxCommand" methods could also accept "title" parameter to cover few
cases where command's title needs to be customized, for example:
public UICommand createDefaultOkUiCommand(String name) {
UICommand command = createOkUiCommand(name);
command.setIsDefault(true);
return command;
}
public UICommand createOkUiCommand(String name) {
return createUiCommand(name,
ConstantsManager.getInstance().getConstants().ok());
}
public UICommand createUiCommand(String name, String title) {
UICommand command = new UICommand(name, target);
command.setTitle(title);
return command;
}
Line 6:
Line 7: private final ICommandTarget target;
Line 8:
Line 9: public UICommandFactory(ICommandTarget target) {
http://gerrit.ovirt.org/#/c/33317/9/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/NewClusterPolicyModel.java
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/NewClusterPolicyModel.java:
Line 222: if (!clusterPolicy.isLocked() || commandType ==
CommandType.Clone) {
Line 223: UICommand onSaveCommand = new
UICommandFactory(this).createDefaultOkUiCommand("OnSave"); //$NON-NLS-1$
Line 224: getCommands().add(onSaveCommand);
Line 225: UICommand onResetCommand = new UICommand("OnReset",
this); //$NON-NLS-1$
Line 226:
onResetCommand.setTitle(ConstantsManager.getInstance().getConstants().resetTitle());
Please see my comment in UICommandFactory, it could provide method to specify
command's title, for example:
UICommand onResetCommand = new UICommandFactory(this).createUiCommand(
"OnReset",
ConstantsManager.getInstance().getConstants().resetTitle()
);
Line 227: getCommands().add(onResetCommand);
Line 228: }
Line 229:
Line 230: UICommand cancelCommand = new UICommand("Cancel", this);
//$NON-NLS-1$
--
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: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Mucha <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: [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
