Nir Soffer has posted comments on this change.
Change subject: webadmin: Display more accurate size of ISO files
......................................................................
Patch Set 3: Code-Review-1
(14 comments)
First, keeping the broken behavior when real file size is unknown is wrong.
Second, the code can be written in a cleaner and more readable way. Last, the
commit message can be improved. See the inline comments.
....................................................
Commit Message
Line 9: Up until now DiskSizeRenderer which is used to render all
image/disk/file
Line 10: sizes had 3 limitations:
Line 11: 1. Size appeared only in GB.
Line 12: 2. All numbers were integers.
Line 13: 3. Size smaller than 1GB appeared as '<1GB'
What is wrong with the old behavior? What do we have to change this?
Line 14:
Line 15: From now on, it supports MB, KB, and numbers with floating point.
Line 16:
Line 17: This patch uses new capability when displaing sizes of ISO files and
floppies
Line 11: 1. Size appeared only in GB.
Line 12: 2. All numbers were integers.
Line 13: 3. Size smaller than 1GB appeared as '<1GB'
Line 14:
Line 15: From now on, it supports MB, KB, and numbers with floating point.
This does not explain why you did this. Something like:
Now DiskSizeRenderer render file size in human readable format, as used by
common applications and expected by users.
The details are available in the code, there is no need to describe it here.
Line 16:
Line 17: This patch uses new capability when displaing sizes of ISO files and
floppies
Line 18: under Storage -> Images.
Line 19:
Line 14:
Line 15: From now on, it supports MB, KB, and numbers with floating point.
Line 16:
Line 17: This patch uses new capability when displaing sizes of ISO files and
floppies
Line 18: under Storage -> Images.
This repeats the what you tell above about DiskSizeRenderer.
Line 19:
Line 20: Change-Id: I95961fde2256a44b8474d8f41562bc0e33b0ad4a
Line 21: Bug-Url: https://bugzilla.redhat.com/1005889
Line 15: From now on, it supports MB, KB, and numbers with floating point.
Line 16:
Line 17: This patch uses new capability when displaing sizes of ISO files and
floppies
Line 18: under Storage -> Images.
Line 19:
You don't tell here the important fact that unless configured to use new
format, the renderer uses the old format.
Line 20: Change-Id: I95961fde2256a44b8474d8f41562bc0e33b0ad4a
Line 21: Bug-Url: https://bugzilla.redhat.com/1005889
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/renderer/DiskSizeRenderer.java
Line 12: GIGABYTE;
Line 13: }
Line 14:
Line 15: public enum Format {
Line 16: DEFAULT,
DEFAULT does not describe the meaning of the constants. How about "LEGACY"?
Line 17: FORMATTED
Line 18: }
Line 19:
Line 20: private final DiskSizeUnit unit;
Line 13: }
Line 14:
Line 15: public enum Format {
Line 16: DEFAULT,
Line 17: FORMATTED
Same problem. How about "HUMAN_READABLE"?
Line 18: }
Line 19:
Line 20: private final DiskSizeUnit unit;
Line 21: private final Format format;
Line 47:
Line 48: double sizeInGB = getSizeInGigabytes(size);
Line 49:
Line 50: if (format == Format.DEFAULT) {
Line 51: return (sizeInGB >= 1) ?
Double.valueOf(sizeInGB).longValue() + " GB" : "< 1 GB"; //$NON-NLS-1$
//$NON-NLS-2$
Extract method renderLegacySize() - don't mix different levels of abstractions
in the same method.
Line 52: } else {
Line 53: return format(sizeInGB);
Line 54: }
Line 55: }
Line 58: double sizeInGB = -1;
Line 59:
Line 60: switch (unit) {
Line 61: case BYTE:
Line 62: sizeInGB = size.doubleValue() / Math.pow(1024, 3);
Add constant for Math.pow(1024, 3)
Line 63: break;
Line 64: case GIGABYTE:
Line 65: sizeInGB = size.doubleValue();
Line 66: break;
Line 68:
Line 69: return sizeInGB;
Line 70: }
Line 71:
Line 72: private String format(double sizeInGB) {
Rename to renderHumanReadableSize()
The class already uses "render", and you cannot change the framework. Lets use
consistent terms.
Line 73: if (sizeInGB == 0) {
Line 74: // sizeInGB = 0 means that Engine is working with old VDSM
which doesn't return the correct size, so it
Line 75: // should display the size as 'less than 1 GB' for
backward compatibility
Line 76: return "< 1 GB"; //$NON-NLS-1$
Line 73: if (sizeInGB == 0) {
Line 74: // sizeInGB = 0 means that Engine is working with old VDSM
which doesn't return the correct size, so it
Line 75: // should display the size as 'less than 1 GB' for
backward compatibility
Line 76: return "< 1 GB"; //$NON-NLS-1$
Line 77: }
Displaying "< 1 GB" for the value 0 is a wrong - you cannot keep this behavior
for backward compatibility.
If size is really 0, display "0 bytes".
If size is 0 because the size is not available, fix the code that call this, so
you can render CONSTANTS.unAvailablePropertyLabel(); 0 is a valid file size and
cannot be used to mark invalid value.
Line 78:
Line 79: NumberFormat fmt = NumberFormat.getFormat("####.##");
//$NON-NLS-1$
Line 80: if (sizeInGB >= 1) {
Line 81: return fmt.format(sizeInGB) + " GB"; //$NON-NLS-1$
Line 77: }
Line 78:
Line 79: NumberFormat fmt = NumberFormat.getFormat("####.##");
//$NON-NLS-1$
Line 80: if (sizeInGB >= 1) {
Line 81: return fmt.format(sizeInGB) + " GB"; //$NON-NLS-1$
Add constant for "GB"
Line 82: }
Line 83:
Line 84: fmt = NumberFormat.getFormat("####.#"); //$NON-NLS-1$
Line 85: double sizeInMB = sizeInGB * 1024;
Line 83:
Line 84: fmt = NumberFormat.getFormat("####.#"); //$NON-NLS-1$
Line 85: double sizeInMB = sizeInGB * 1024;
Line 86: if (sizeInMB >= 1) {
Line 87: return fmt.format(sizeInMB) + " MB"; //$NON-NLS-1$
Add constant for "MB"
Line 88: }
Line 89:
Line 90: double sizeInKB = sizeInMB * 1024;
Line 91: return fmt.format(sizeInKB) + " KB"; //$NON-NLS-1$
Line 87: return fmt.format(sizeInMB) + " MB"; //$NON-NLS-1$
Line 88: }
Line 89:
Line 90: double sizeInKB = sizeInMB * 1024;
Line 91: return fmt.format(sizeInKB) + " KB"; //$NON-NLS-1$
Add constant for "KB"
Line 92: }
Line 88: }
Line 89:
Line 90: double sizeInKB = sizeInMB * 1024;
Line 91: return fmt.format(sizeInKB) + " KB"; //$NON-NLS-1$
Line 92: }
Why do you use different format for different units? this is not consistent and
not what users expect. Use same format, e.g. ###.## for all units.
--
To view, visit http://gerrit.ovirt.org/19550
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I95961fde2256a44b8474d8f41562bc0e33b0ad4a
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
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
