GNOME Bugzilla – Bug 688333
We don't show recommended size in ram/disk properties
Last modified: 2016-03-31 14:00:32 UTC
Split off from bug 678123 As per the mockups we should be showing the recommended size for both disk and ram. I don't know how to get this information though.
Created attachment 235259 [details] [review] Add LibvirtMachineProperties Refactor properties handling code of LibvirtMachine to a separate class/module. This change reduces LibvirtMachine LOC by 59%.
Created attachment 235260 [details] [review] libvirt-machine-properties: Specify type of list content
Created attachment 235261 [details] [review] Custom classes for size & string properties Main motivation for this was me being unable to convince myself to add yet more arguments to IPropertiesProvider.add_size_property() as too many arguments goes against good OO practices[1]. So I think its better to have a custom class for size property and replace optional arguments of IPropertiesProvider.add_size_property() by fields/props/signals of SizeProperty class. StringProperty then just makes sense to have for consistency. [1] http://stackoverflow.com/questions/4630470/how-many-arguments-is-a-reasonable-number-big-objects-vs-atomic-arguments-oop
Created attachment 235262 [details] [review] util: compare_cpu_architectures() should support 'all' arch This is going to be needed in the following patches when we use this function for comparing CPU arch against minimum/recommended resources.
Created attachment 235263 [details] [review] os-database: Make use of compare_cpu_architectures() Make use of compare_cpu_architectures() function when finding minimum/recommended resources for an OS.
Created attachment 235264 [details] [review] os-database: Add get_recommended_resources_for_os() Add a method to retrieve recommended resources for an OS.
Created attachment 235265 [details] [review] size-property: Use bytes rather than KB Use of KB as the unit didn't turn out to be as good an idea as I originaly thought. It only forces a lot of conversion everwhere, which serves contrary to my intentions when I chose KB.
Created attachment 235266 [details] [review] size-property: Use 64MB for all size properties The 1MB step we use for RAM is too small and 1GB step we use for storage is too big. 64MB seems to me like a nice step value for both properties. This also implies that minimum RAM is now 64MB.
Created attachment 235267 [details] [review] size-property: API to mark recommended size
Created attachment 235268 [details] [review] libvirt-machine-properties: add_*_property() return Property add_*_property() methods now return ref to the property they add.
Created attachment 235269 [details] [review] libvirt-machine-properties: Mark recommended resources Mark recommended RAM and storage when known.
Review of attachment 235259 [details] [review]: Its nice to be able to split out this code like this, but its kinda weird how it uses inheritance to do it. You end up with a useless in-between class only to separate the class into two files. If we truly want to separate these, then we should make the properties of a machine a separate object rather than something the machine inherits. Like, LibvirtMachine could inherit from IPropertiesProvieder, but that would be a very very thin interface that just had a method to get a Properties object.
Review of attachment 235260 [details] [review]: ack
Review of attachment 235261 [details] [review]: looks nice
Review of attachment 235262 [details] [review]: What about 'all' arch as arch1? Will that never happen? In general i think this function needs more docs, i always get confused as to which one of "arch1" and "arch2" is what, as its a very assymetrical function.
Review of attachment 235263 [details] [review]: Looks ok to me, but shouldn't OsinfoFilter have something like this?
Review of attachment 235264 [details] [review]: ::: src/os-database.vala @@ +113,3 @@ + public Resources? get_recommended_resources_for_os (Os os, string architecture) { + var prefs = new string[0]; why not just use string[] prefs = { architecture, ARCHITECTURE_ALL};
Review of attachment 235265 [details] [review]: ack
Review of attachment 235266 [details] [review]: I disagree. I think 64MB is a bit to coarse for disk sizes. 256 or 512 MB seems more reasonable sizes there. 64MB seems ok for disk though.
Review of attachment 235267 [details] [review]: ::: src/i-properties-provider.vala @@ +60,3 @@ + public uint64 recommended { + set { + // FIXME: Better way to ensure recommended mark is not too close to min and max marks? What happens if they are? 1GB seems quite large for the ram case...
Review of attachment 235268 [details] [review]: ack
Review of attachment 235269 [details] [review]: ack
Review of attachment 235266 [details] [review]: Alex, do you mean '64MB seems ok for RAM though'? There is one 'disk' too much in your comment. Ideally, we would start the scale with smaller increments, but I guess that's not really possible (ie have 1MB, 2, 4, 8. 16, 32, 64MB and then move in 64MB increments, or even move in 64MB increments until 512MB/1GB, and then move in 256MB increments).
Review of attachment 235266 [details] [review]: yeah, thats what i mean.
Review of attachment 235259 [details] [review]: IMO an intermediate class is not bad at all. All abstract classes are useless anyway. :) Having said that, what you say isn't a bad idea either, although it wouldn't remove as many LOC from libvirt-machine.vala as this patch does. :)
Review of attachment 235263 [details] [review]: I'm not sure its generally useful.
Review of attachment 235264 [details] [review]: ::: src/os-database.vala @@ +113,3 @@ + public Resources? get_recommended_resources_for_os (Os os, string architecture) { + var prefs = new string[0]; I just didn't dare to check if vala allows me to put a dynamic string in intializer like that. :) Its probably possible so I'll try..
Review of attachment 235267 [details] [review]: ::: src/i-properties-provider.vala @@ +60,3 @@ + public uint64 recommended { + set { + // FIXME: Better way to ensure recommended mark is not too close to min and max marks? Labels look ugly if they are close and especially in worse case they can get mingled and unreadable. Although its usually the 'min' that is problematic and especially in case of disk as min is same as current size. :( Now that I think of it, if we justify the recommended label to right, we wouldn't need as big a value as 1 GB.
Review of attachment 235262 [details] [review]: Although arch1 is not going to be 'all' in existing use, I'll modify this patch to accommodate for that eventuality. I'd love to document this function but I really suck at documenting and in this particular case, my mind just gets stuck on how to explain this. Help welcome!
Review of attachment 235267 [details] [review]: ::: src/i-properties-provider.vala @@ +60,3 @@ + public uint64 recommended { + set { + // FIXME: Better way to ensure recommended mark is not too close to min and max marks? Actually I can't justify no labels as we are using Gtk.Scale.add_mark() API for min/max and that API doens't provide any way to do any styling to the underlying labels.
Created attachment 235846 [details] [review] Add LibvirtMachineProperties v2: LibvirtMachineProperties now a standalone object that LibvirtMachine uses rather than inheriting from it.
Created attachment 235847 [details] [review] libvirt-machine-properties: Specify type of list content v2: Just a rebase
Created attachment 235848 [details] [review] Custom classes for size & string properties v2: Just rebased.
Created attachment 235849 [details] [review] util: compare_cpu_architectures() should support 'all' arch v2: Accomodate for possibility of arch1 being 'all' as well.
Created attachment 235850 [details] [review] os-database: Make use of compare_cpu_architectures() v2: rebased
Created attachment 235851 [details] [review] os-database: Add get_recommended_resources_for_os() v2: Adjusted according to comment#17.
Created attachment 235852 [details] [review] size-property: Use bytes rather than KB v2: rebased.
Created attachment 235853 [details] [review] libvirt-machine-properties: 64M as min & step for RAM v2: Use different min and step for ram and storage.
Created attachment 235854 [details] [review] libvirt-machine-properties: 256M as step for storage The 1G step value we use for storage is too big. Decrease it to 256M.
Created attachment 235855 [details] [review] size-property: API to mark recommended size v2: Only rebase. I tried to have the limits not so hardcoded but failed to achieve that so far. I think to get things moving, we should just go with +-1G limits for now. Most modern OSs will have recommended RAM in GBs anyway.
Created attachment 235856 [details] [review] libvirt-machine-properties: add_*_property() return Property v2: rebased.
Created attachment 235857 [details] [review] libvirt-machine-properties: Mark recommended resources v2: Rebased.
Review of attachment 235846 [details] [review]: This wasn't quite what I meant. This keeps the function calls on the LibvirtMachine and just retargets them to another object. What i meant was changing Boxes.IPropertiesProvider such that it has a call to get an object of style IProperties. I.e. its basically your change, but the retargeting would happen on the caller side. However, that could be easily done after this patch is applied, so *ack*.
Review of attachment 235847 [details] [review]: ack
Review of attachment 235848 [details] [review]: ack
Review of attachment 235849 [details] [review]: ack
Review of attachment 235850 [details] [review]: ack
Review of attachment 235851 [details] [review]: ack
Review of attachment 235852 [details] [review]: ack
Review of attachment 235853 [details] [review]: ack
Review of attachment 235854 [details] [review]: ack
Review of attachment 235855 [details] [review]: ack
Review of attachment 235856 [details] [review]: ack
Review of attachment 235857 [details] [review]: ack
Attachment 235846 [details] pushed as e176e57 - Add LibvirtMachineProperties Attachment 235847 [details] pushed as 478ddf9 - libvirt-machine-properties: Specify type of list content Attachment 235848 [details] pushed as 5c25ce4 - Custom classes for size & string properties Attachment 235849 [details] pushed as c2ac7bf - util: compare_cpu_architectures() should support 'all' arch Attachment 235850 [details] pushed as a1d7857 - os-database: Make use of compare_cpu_architectures() Attachment 235851 [details] pushed as 63cc23d - os-database: Add get_recommended_resources_for_os() Attachment 235852 [details] pushed as 9b51f54 - size-property: Use bytes rather than KB Attachment 235853 [details] pushed as 4a7ee09 - libvirt-machine-properties: 64M as min & step for RAM Attachment 235854 [details] pushed as a14701e - libvirt-machine-properties: 256M as step for storage Attachment 235855 [details] pushed as a47a1ae - size-property: API to mark recommended size Attachment 235856 [details] pushed as 43884e1 - libvirt-machine-properties: add_*_property() return Property Attachment 235857 [details] pushed as 7da90d4 - libvirt-machine-properties: Mark recommended resources