Sergey Gotliv has posted comments on this change. Change subject: webadmin: Display more accurate size of ISO files ......................................................................
Patch Set 2: (5 comments) .................................................... Commit Message Line 5: CommitDate: 2013-09-29 14:42:58 +0300 Line 6: Line 7: webadmin: Display more accurate size of ISO files Line 8: Line 9: Till now DiskSizeRenderer which is used to render all image/disk/file Done 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' 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, it supports MB, KB, and numbers with floating point. Done Line 16: Line 17: Using this new capability when displaing sizes of ISO files and floppies Line 18: under Storage -> Images. Line 19: Line 14: Line 15: From now, it supports MB, KB, and numbers with floating point. Line 16: Line 17: Using this new capability when displaing sizes of ISO files and floppies Line 18: under Storage -> Images. Done Line 19: 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 70: } Line 71: Line 72: private String format(double sizeInGB) { Line 73: if (sizeInGB == 0) { Line 74: // sizeInGB = 0 means that Engine working with old VDSM which doesn't return the correct size, so it Done Line 75: // should display the size as 'less than 1 GB' for backward compatibility Line 76: return "< 1 GB"; //$NON-NLS-1$ Line 77: } Line 78: Line 73: if (sizeInGB == 0) { Line 74: // sizeInGB = 0 means that Engine 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: } I introduced another way(algorithm) to render a disk size in this patch. I had 3 different ways to do that: 1.Create another renderer class next to the current. Probably name it AccurateDiskSizeRenderer(which will imply that the current renderer is not accurate :-)) 2.Use Strategy Design Pattern. Cause DiskSizeRenderer to work with 2 strategies: Default and Formatted/Accurate. 3.Create new configuration option for existing DiskSizeRenderer (Format enum) and render according to that configuration option. After discussing with Daniel, I choose an option #3(although #1 and #2 probably more structured, elegant, flexible and easily extendable) But even after choosing #3 I wanted to separate the old(DEFAULT) logic from new one: render() is creating a such separation by using IF/ELSE(I didn't want to use CASE in choice of 2), for DEFAULT format I am using one liner, for others(FORMATTED) I call method "format" which is doing the job. So, I don't see any duplicate logic here: I see unfortunate case where new logic has to care about backward compatibility for specific case where "size = 0", which means Engine works with old VDSM which doesn't provide the size of the disk, so I treat it as "<1GB", but it doesn't mean I want to mix it with the old logic. By the way, if you think that #1 or #2 is better than the current implementation I will re-implement it. Did I convince? 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$ -- 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: 2 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: 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
