Jakub Niedermertl 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? Done 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(); Done Line 146: } Line 147: } Line 148: }; Line 149: } Line 169: defaultButton == null > did you mean defaultIcon == null? yes, I did 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 b Done 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" w Done 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 renam Done 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 mis I don't handle it. This should never happen and the exception would indicate either error in engine or mistake in frontend code. 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 This happens when someone asks for icon with ID null, which is nonsenses. So either there is an error in engine or someone use this in a bad way. 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 same. Provided that there is no bug, it should never happen 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: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@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
