Created attachment 271480 [details] [review]
libvirt-machine-props: Fix maximum value of 'Maximum Disk Size'

volume_info.capacity should not be taken into account to calculate the
maximum value of 'Maximum Disk Size', which must be the space available
in the StoragePool (pool_info.available)

(In reply to comment #1)
> Created an attachment (id=271480) [details] [review]
> libvirt-machine-props: Fix maximum value of 'Maximum Disk Size'

Pretty sure this goes way beyond 50 chars: : https://wiki.gnome.org/Git/CommitMessages . maximim -> max and writing not quoting exact string would help shortening it. :) This would be fine:

libvirt-machine-props: Fix max value of max disk

> volume_info.capacity should not be taken into account to calculate the
> maximum value of 'Maximum Disk Size', which must be the space available
> in the StoragePool (pool_info.available)

* Its better to keep description in as much normal English as possible (i-e avoiding identifiers)
  * volume_info.capacity -> Allocated capacity
  * 'in StoragePool' -> 'on storage pool'
* taken -> taking
* ", which must be the" -> ". It should simply be the"

Review of attachment 271480 [details] [review]:

Fine code-wise. :)

Created attachment 271486 [details] [review]
libvirt-machine-props: Fix max value of max disk

Allocated capacity should not be taking into account to calculate the
maximum value of 'Maximum Disk Size'. It should simply be the space
available on storage pool.

Review of attachment 271486 [details] [review]:

Ouch, 'taken' was actually correct here. :) Just fix it before pushing. Sorry for that.

Created attachment 271488 [details] [review]
libvirt-machine-props: Fix max value of max disk

Allocated capacity should not be taken into account to calculate the
maximum value of 'Maximum Disk Size'. It should simply be the space
available on storage pool.

Attachment 271488 [details] pushed as d9f30a0 (gnome-boxes 3.11.92+)

(In reply to comment #7)
> Attachment 271488 [details] pushed as d9f30a0 (gnome-boxes 3.11.92+)

Actually this was wrong and introduced many criticals when going to properties view for existing machines at least:

(gnome-boxes:9848): Gtk-CRITICAL **: gtk_scale_new_with_range: assertion 'min < max' failed

The existing code before this patch was correct as it took into account the existing capacity. E.g if disk image capacity is 2G and remaining space on volume is 1G, we'll putting 1G as max while 2G is min. You see the huge issue with that? :)

Reverted the patch.

Reopening the bug. Please, see the attached patch.

Created attachment 283334 [details] [review]
libvirt-machine-props: Fix max value of max disk

Created attachment 283335 [details] [review]
libvirt-machine-props: Fix max value of max disk

Created attachment 283342 [details] [review]
libvirt-machine-props: Fix max value of max disk

Review of attachment 283342 [details] [review]:

"The existing code is wrong as it considers the maximum storage value as
volume info's capacity + available space in the pool while the proper
value should be volume info's allocation + available space in the pool."

Some mention of what is wrong with existing code and how this new solution is "proper" (I actually don't think its "proper" but just better)? :)

"However it introduces an issue where the minimum storage value (volume
info's capacity) can be bigger or equal the maximum storage value. In
this case we have agreed in showing the current disk size as a "label"
instead of the scale"

* equal -> equal to
* I wouldn't say 'we' w/o indication of who that includes. I'll just write "In this case, lets just show the current capacity as a label and not allow to change the value".

BTW, we probobly want to tell user that they don't have enough disk scace to increase max disk size.

Created attachment 283419 [details] [review]
v2: libvirt-machine-props: Fix max value of max disk

Created attachment 283421 [details] [review]
v3: libvirt-machine-props: Fix max value of max disk

v2: changes from v1:
- Grammar fixes in the commit log
- Show a warning in an InfoBar, telling the users that they don't have enough space in the disk to increase the Maximum Disk Size.
v3: changes from v2:
- Grammar fixes in the commit log

Review of attachment 283421 [details] [review]:

I asked you to provide more details/specifics about whats wrong with existing code and how your code is "proper" but not write a whole story which hurts my brain cause its hard to remember what is what. Can you please make it brief? Just shortly write what is wrong with existing code and how your solution is better (or "proper" if you prefer that term).

Review of attachment 283421 [details] [review]:

::: src/libvirt-machine-properties.vala
@@ +480,3 @@
+                                     "%s".printf (format_size (volume_info.capacity, FormatSizeFlags.DEFAULT)));
+
+                var infobar = new Gtk.InfoBar ();

this needs to be in an additional patch as it has nothing to do with correcting max value itself.

Created attachment 283485 [details] [review]
v4 libvirt-machine-props: Fix max value of max disk

v4: Changes from v3:
- Summarize the commit log
- Move the addition of the warning message to another patch

Created attachment 283486 [details] [review]
libvirt-machine-props: Warn when max disk size cannot be bigger

Review of attachment 283485 [details] [review]:

ACK

Review of attachment 283486 [details] [review]:

* I don't see the need to capitalize "Maximum Disk Size".
* How about "Warn on immutable max disk" as shortlog?

::: src/libvirt-machine-properties.vala
@@ +490,3 @@
+                content.add (image);
+
+                var msg = _("There is not enough space on the disk to increase the Maximum Disk Size.");

Same comment about capitalization here. I'd replace "on the disk" to "on your machine" to avoid confusion with two disk sizes in the sentences. :)

Created attachment 283524 [details] [review]
v2: libvirt-machine-props: Warn on immutable max disk

v2: Changes from v1:
- Don't use capitalized "Maximum Disk Size"
- Changed the commit shortlog as suggested
- s/on the disk/on your machine/ to avoid confusion

Review of attachment 283524 [details] [review]:

Still not completely satisfied with the message we show but i can't come up with something better. ack

Both patches pushed upstream (3.13.90+)