Vojtech Szocs has posted comments on this change.
Change subject: webadmin,userportal: Upgrade GWT SDK and related modules
......................................................................
Patch Set 1: (5 inline comments)
First of all, *big* thanks to Alex and Tomas for going over this monstrous
patch and providing review comments. I really appreciate it, guys!
> I noticed that most places where you refer to the GinjectorProvider which is
> deprecated there is no @suppresswarning('deprecation')
Yes, this is intentional.
Code for setting up ClientGinjectorProvider static injection (SystemModule) is
marked with @SuppressWarnings("deprecation") because that's GIN infra code.
However, code *using* ClientGinjectorProvider isn't marked to suppress
deprecation warnings, which expresses the fact that using
ClientGinjectorProvider is not a good thing and we should improve the code to
get rid of ClientGinjectorProvider usage.
> Also I would try to see if we can inject some of the stuff from
> GinjectorProvider into the objects if the access is not static.
Yes, in other words refactor code to reduce/eliminate ClientGinjectorProvider
usage in order to get rid of deprecation warnings.
This is actually the top priority item mentioned in
[http://www.ovirt.org/Features/GWT_Platform_Upgrade] - GWTP non-essential
changes section: CodeCleanup - Get rid of deprecated ClientGinjectorProvider
classes in WebAdmin & UserPortal
I'd rather have this kind of refactoring done in a separate patch (this patch
is quite big already).
> About 20% of the affected files seem to be just import location changes, I
> don't mind, but other people might complain.
Yes, I did "organize imports" in my IDE globally on most GUI modules. Import
declaration re-ordering is indeed a non-significant change and can be discarded
before final merge.
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/EntityModelCellTable.java
Line 283: }
Line 284: }
Line 285:
Line 286: M getListModel() {
Line 287: return asEditor().flush();
Exactly, HasDataListModelEditorAdapter.flush() just returns "listModel"
reference so there's no extra overhead to calling getListModel() multiple times.
Line 288: }
Line 289:
Line 290: @Override
Line 291: public HasEditorDriver<M> asEditor() {
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/IVdcQueryableCellTable.java
Line 32: * Table row data type.
Line 33: * @param <M>
Line 34: * List model type.
Line 35: */
Line 36: public class IVdcQueryableCellTable<T, M extends ListModel> extends
ColumnResizeCellTable<T> implements IsEditorDriver<M> {
Yes, I wanted to avoid the extra change of renaming this class, but I can
rename it as part of this patch, too.
This class is similar to EntityModelCellTable with following difference:
* EntityModelCellTable is Editor of ListModel object that contains EntityModel
items
* IVdcQueryableCellTable is Editor of ListModel object that contains arbitrary
(? extends Object) items
I can rename IVdcQueryableCellTable to ListModelObjectCellTable, what do you
think?
(Maybe we should also consider dropping IVdcQueryableCellTable and using
EntityModelCellTable instead.)
Line 37:
Line 38: private static final int DEFAULT_PAGESIZE = 1000;
Line 39: private static final int CHECK_COLUMN_WIDTH = 27;
Line 40:
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/RxTxRateColumn.java
Line 17: @Override
Line 18: protected Double[] getRawValue(T object) {
Line 19: Double rate = object != null ? getRate(object) : null;
Line 20: Double speed = object != null ? getSpeed(object) : null;
Line 21: return new Double[] { rate, speed };
I did this change in order to fix NPE where object == null while testing
UserPortal.
> shouldn't getRate and getSpeed just return null when object is null? why
> check here?
Because all getRate/getSpeed implementations never check for null and I wanted
to minimize the amount of changes.
> I would let it here - there are lots of implementations of getRate and
> getSpeed so the null check would have to be in each of them. This is better I
> would say.
This was my motivation as well :-)
Line 22: }
Line 23:
Line 24: /**
Line 25: * Returns the Rx/Tx transfer rate.
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/main/view/tab/extended/SideTabExtendedVirtualMachineView.java
Line 95: getTableResources(),
Line 96: ClientGinjectorProvider.getEventBus(),
Line 97: new UserPortalRefreshManager(modelProvider,
Line 98: ClientGinjectorProvider.getEventBus(),
Line 99: ClientGinjectorProvider.getClientStorage()));
Right, ClientGinjectorProvider classes are marked as @Deprecated which means we
should get rid of them in near future.
Not suppressing deprecation warnings in code using ClientGinjectorProvider
classes is therefore intentional. In other words, it expresses the fact that
using ClientGinjectorProvider classes is not a good thing and we should improve
the code to get rid of deprecation warnings (instead of just suppressing them).
This is also mentioned in [http://www.ovirt.org/Features/GWT_Platform_Upgrade]
- in GWTP non-essential changes section, the top priority item is: CodeCleanup
- Get rid of deprecated ClientGinjectorProvider classes in WebAdmin & UserPortal
Line 100: }
Line 101:
Line 102: @Override
Line 103: protected Resources getTableResources() {
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/widget/extended/vm/TooltipCell.java
Line 47: String tooltip = provider.getTooltip(value);
Line 48: if (tooltip == null) {
Line 49: tooltip = ""; //$NON-NLS-1$
Line 50: }
Line 51: // TODO(vszocs) consider using SafeHtmlTemplates instead of
building HTML manually
It's not related to GWT(P) upgrade, but I encountered NPE when
provider.getTooltip(value) == null while testing UserPortal.
This change is a quick fix for NPE mentioned above.
Line 52: sb.appendHtmlConstant("<div id=\"" //$NON-NLS-1$
Line 53: +
ElementIdUtils.createTableCellElementId(elementIdPrefix, columnId, context)
Line 54: + "\" title=\"" //$NON-NLS-1$
Line 55: + SafeHtmlUtils.htmlEscape(tooltip)
--
To view, visit http://gerrit.ovirt.org/16739
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica797e68e2f56fd3f231baf36f0ac03c5df5c3f2
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: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
