Vojtech Szocs has posted comments on this change.
Change subject: webadmin: Adding "Select All" checkbox header to CellTable
......................................................................
Patch Set 1: (7 inline comments)
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/EntityModelCellTable.java
Line 224: public Boolean getValue(EntityModel object) {
Line 225: return getSelectionModel().isSelected(object);
Line 226: }
Line 227: };
Line 228: addColumn(checkColumn,
SafeHtmlUtils.fromSafeConstant("<br/>")); //$NON-NLS-1$
Do we really want to use "<br/>" as empty header's HTML value? (I've seen this
used in other places as well.)
Shouldn't we use empty string (CommonApplicationConstants.empty), or
non-breaking space (" "), instead?
Line 229: } else if (getSelectionModel() instanceof
MultiSelectionModel) {
Line 230: checkColumn = new Column<EntityModel, Boolean>(
Line 231: new CheckboxCell(true, false)) {
Line 232: @Override
Line 246: }
Line 247:
Line 248: @Override
Line 249: public Boolean getValue() {
Line 250: if (listModel == null ||
EntityModelCellTable.this.listModel.getItems() == null) {
Is "EntityModelCellTable.this" necessary in the second part of condition?
Line 251: return false;
Line 252: }
Line 253: return
getCheckValue(listModel.getItems(), getSelectionModel());
Line 254: }
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/IVdcQueryableCellTable.java
Line 116: return getCheckValue(listModel.getItems(),
getSelectionModel());
Line 117: }
Line 118: };
Line 119: addColumn(checkColumn, selectAllHeader);
Line 120: setColumnWidth(checkColumn, CHECK_COLUMN_WIDTH,
Unit.PX);
I'd recommend to use "else" block here, just like in EntityModelCellTable, and
remove the "setColumnWidth" call.
Line 121: }
Line 122: } else {
Line 123: checkColumn = new Column<IVdcQueryable, Boolean>(
Line 124: new RadioboxCell(true, false)) {
Line 127: return getSelectionModel().isSelected(object);
Line 128: }
Line 129: };
Line 130: }
Line 131: if (!showSelectAllCheckbox) {
"checkColumn" will never be null in IVdcQueryableCellTable. I'd recommend
removing the "if (!showSelectAllCheckbox)" block, and call "setColumnWidth"
method instead. This is just like in EnityModelCellTable, but without "if
(checkColumn != null)" condition.
Line 132: addColumn(checkColumn,
SafeHtmlUtils.fromSafeConstant("<br/>")); //$NON-NLS-1$
Line 133: setColumnWidth(checkColumn, CHECK_COLUMN_WIDTH, Unit.PX);
Line 134: }
Line 135: }
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/header/SelectAllCheckBoxHeader.java
Line 11:
Line 12: public abstract class SelectAllCheckBoxHeader<T> extends
Header<Boolean> {
Line 13:
Line 14: public SelectAllCheckBoxHeader() {
Line 15: super(new CheckboxCell(true, true));
I like this, the CheckboxCell constructor uses handlesSelection=true. I think
this is better approach than using handlesSelection=false and rendering
checkbox HTML every time, like we do it in our CheckboxHeader.
Line 16: setUpdater(new ValueUpdater<Boolean>() {
Line 17: @Override
Line 18: public void update(Boolean value) {
Line 19: selectionChanged(value);
Line 44:
Line 45: boolean allSelected = true;
Line 46: for (T entity : items) {
Line 47: if (!selectionModel.isSelected(entity)) {
Line 48: allSelected = false;
I'd replace this with "return false" instead of iterating through remaining
elements.
Line 49: }
Line 50: }
Line 51: return allSelected;
Line 52: }
Line 47: if (!selectionModel.isSelected(entity)) {
Line 48: allSelected = false;
Line 49: }
Line 50: }
Line 51: return allSelected;
I'd replace this with "return true" :-)
Line 52: }
--
To view, visit http://gerrit.ovirt.org/8282
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f9f303143ecb322a6d325a56c3b97a23b3253c6
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
