Tomas Jelinek has posted comments on this change. Change subject: webadmin: VM Icons - frontend part ......................................................................
Patch Set 12: (9 comments) https://gerrit.ovirt.org/#/c/38601/12/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/IconEditorWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/IconEditorWidget.java: Line 140: @Override Line 141: public void onKeyPress(KeyPressEvent event) { Line 142: if (!event.isAnyModifierKeyDown() Line 143: && event.getNativeEvent().getKeyCode() == KeyCodes.KEY_ENTER Line 144: && event.getUnicodeCharCode() == 0) { why this additional check for the char code? Line 145: event.preventDefault(); Line 146: } Line 147: } Line 148: }; Line 141: public void onKeyPress(KeyPressEvent event) { Line 142: if (!event.isAnyModifierKeyDown() Line 143: && event.getNativeEvent().getKeyCode() == KeyCodes.KEY_ENTER Line 144: && event.getUnicodeCharCode() == 0) { Line 145: event.preventDefault(); I'd say also event.stopPropagation(); Line 146: } Line 147: } Line 148: }; Line 149: } Line 169: defaultButton == null did you mean defaultIcon == null? https://gerrit.ovirt.org/#/c/38601/12/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java: Line 878: @UiField Line 879: @Ignore Line 880: protected DialogTabPanel mainTabPanel; Line 881: Line 882: // ==Icon Tab== no need for this comment - the variable is called iconTab and you can not be sure that someone will not put some other widgets between iconTab and iconEditorWidget Line 883: @UiField Line 884: @Ignore Line 885: protected DialogTab iconTab; Line 886: Line 2135: highAvailabilityTab, Line 2136: resourceAllocationTab, Line 2137: customPropertiesTab, Line 2138: rngDeviceTab, Line 2139: iconTab is it really admin only? Is it advanced enough that the "instanceCreator" will not be allowed to change it? Because I can imaging this to be changed by instanceCreator Line 2140: ); Line 2141: } Line 2142: Line 2143: protected void disableAllTabs() { https://gerrit.ovirt.org/#/c/38601/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java: Line 798: Line 799: return result; Line 800: } Line 801: Line 802: public static <T> List<T> concatTypesafe(List<T>... lists) { I would do this the opposite way around - call this method concat and rename the above to concatUnsafe. I know it is used to concat pool and vm also to concat uicommands. And also, the safe way should be the default and not the unsafe... Line 803: return concat(lists); Line 804: } Line 805: Line 806: public static <T> ArrayList<T> union(ArrayList<ArrayList<T>> lists) https://gerrit.ovirt.org/#/c/38601/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java: Line 3580: final VmIconIdSizePair pair = osIdToDefaultIconIdMap.get(defaultOsId); Line 3581: if (pair != null) { Line 3582: return pair.get(small); Line 3583: } Line 3584: throw new RuntimeException("Icon of default operating system not found."); //$NON-NLS-1$ are you handling this somehow? because die like this because an icon is missing is not too good. You should warn the user that something is wrong but not just fail with a log in JS console. Line 3585: } Line 3586: Line 3587: public Guid getSmallByLargeOsDefaultIconId(Guid largeIconId) { Line 3588: return largeToSmallOsDefaultIconIdMap.get(largeIconId); https://gerrit.ovirt.org/#/c/38601/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/IconCache.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/IconCache.java: Line 88: } Line 89: }); Line 90: } Line 91: Line 92: private void assertNotNull(List list) { when can this happen? Are all the cases so error cases that you really want to die with only a stack in JS console? Line 93: if (list == null) { Line 94: throw new IllegalArgumentException("Argument should not be null."); //$NON-NLS-1$ Line 95: } Line 96: for (Object item : list) { Line 142: } Line 143: Line 144: public void put(Guid id, String icon) { Line 145: if (id == null || icon == null) { Line 146: throw new IllegalArgumentException("Neither 'id' nor 'icon' can be null."); //$NON-NLS-1$ same Line 147: } Line 148: map.put(id, icon); Line 149: reverseMap.put(icon, id); Line 150: } -- To view, visit https://gerrit.ovirt.org/38601 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id56aecccef6aab6604de32e27497991ab3713d8d Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
