Vojtech Szocs has posted comments on this change.
Change subject: userportal, webadmin: make column width persistent
......................................................................
Patch Set 4: Code-Review+1
(5 comments)
Nice patch overall, just some minor comments, let me know what you think.
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/AbstractActionTable.java
Line 219: // Default to 'no items to display'
Line 220: this.table.setEmptyTableWidget(new NoItemsLabel());
Line 221:
Line 222: // column resizing persistence -- can be enabled only when
the tableHeader widget is visible
Line 223: if (!showDefaultHeader) {
I forgot about this but maybe we can replace "!showDefaultHeader" expression
(occurs on many places) with a method such as:
boolean isTableHeaderVisible() {
return !showDefaultHeader;
}
What do you think?
Line 224: tableHeader.enableColumnWidthPersistence(clientStorage,
dataProvider.getModel());
Line 225: table.enableColumnWidthPersistence(clientStorage,
dataProvider.getModel());
Line 226: }
Line 227:
Line 221:
Line 222: // column resizing persistence -- can be enabled only when
the tableHeader widget is visible
Line 223: if (!showDefaultHeader) {
Line 224: tableHeader.enableColumnWidthPersistence(clientStorage,
dataProvider.getModel());
Line 225: table.enableColumnWidthPersistence(clientStorage,
dataProvider.getModel());
I think doing "tableHeader.enableColumnWidthPersistence(...)" should be enough
here, since "tableHeader" widget (representing a separate header-only table)
will be the one through which column resizing is realized at runtime.
In other words, when column resizing is enabled (which implies that
"tableHeader" widget is visible), user interacts with "tableHeader" widget
(i.e. resize column) and any effects are mirrored to "table" widget
(representing a separate content-only table). You can see this in
"this.tableHeader = ..." code:
* tableHeader.onResizeEnd calls table.redraw
* tableHeader.resizeColumn calls table.resizeColumn
Now, you might be asking - why are we doing this:
public void enableColumnResizing() {
if (!showDefaultHeader) {
table.enableColumnResizing();
tableHeader.enableColumnResizing();
}
}
instead of just this:
public void enableColumnResizing() {
if (!showDefaultHeader) {
tableHeader.enableColumnResizing();
}
}
And the answer is, we need to call table.enableColumnResizing just because both
table widgets need the same column layout, i.e. resizable columns feature
relies on empty, no-width column always as the last column.
On the other hand, for column width persistence, we don't have any such
restriction so we can just call tableHeader.enableColumnWidthPersistence.
Line 226: }
Line 227:
Line 228: }
Line 229:
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/resize/ColumnResizeCellTable.java
Line 36: private static final int DEFAULT_MINIMUM_COLUMN_WIDTH = 30;
Line 37:
Line 38: private int minimumColumnWidth = DEFAULT_MINIMUM_COLUMN_WIDTH;
Line 39:
Line 40: // Prefix for keys used to store refresh rates of individual data
grids
... used to store widths of individual columns :-)
Line 41: private static final String GRID_COLUMN_WIDTH_PREFIX =
"GridColumnWidth"; //$NON-NLS-1$
Line 42:
Line 43:
Line 44: // Empty, no-width column used with resizable columns feature
Line 224: }
Line 225:
Line 226: @Override
Line 227: public void setColumnWidth(Column<T, ?> column, String width) {
Line 228: if (!initializedColumns.contains(column) &&
columnResizePersistenceEnabled) {
Minor thing: I think it's better if "columnResizePersistenceEnabled" comes
first in this expression (but it's not a big deal since initializedColumns is
empty when columnResizePersistenceEnabled==false):
if (columnResizePersistenceEnabled && !initializedColumns.contains(column)) ...
Line 229: String persistedWidth = readColumnWidth(column);
Line 230: if (persistedWidth != null) {
Line 231: width = persistedWidth;
Line 232: }
Line 324: public void enableColumnWidthPersistence(ClientStorage
clientStorage, GridController gridController) {
Line 325: this.clientStorage = clientStorage;
Line 326: this.gridController = gridController;
Line 327: if (clientStorage != null && gridController != null) {
Line 328: columnResizePersistenceEnabled = true;
+1 to boolean field (columnResizePersistenceEnabled) to guard persistent column
width feature invariant (clientStorage != null && gridController != null)
Line 329: }
Line 330: }
Line 331:
Line 332: protected String getColumnWidthKey(Column<T, ?> column) {
--
To view, visit http://gerrit.ovirt.org/17118
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I761e12de743fc9b5fce9edf52b929200abe25c8c
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Greg Sheremeta <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Greg Sheremeta <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[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
