Created attachment 219146 [details] [review]
properties: if edition change is not validate keep editing

If changed cb throws a Boxes.INVALID error, keep editing the state. It
can be cancelled/escaped with Escape key.

We may want to have a (x) icon to indicate the error, that would need
further discussion with UI.

Created attachment 219147 [details] [review]
Use explicit GetXML flag name, bump libvirt-glib dep

Bump libvirt-glib to 0.1.1 for flag names.

Created attachment 219148 [details] [review]
libvirt: allow changing machine name

depends on bug 680168

Review of attachment 219146 [details] [review]:

Looks right otherwise.

::: src/i-properties-provider.vala
@@ +27,3 @@
             } catch (Boxes.Error error) {
                 warning (error.message);
+                if (error is Boxes.Error.INVALID)

IMO having a separate catch for the exact error rather than checking the error type is more cleaner.

Review of attachment 219147 [details] [review]:

If the patch wasn't simple, I wouldn't have understood from log what its about. There is no 'GetXML' identifier involved and no need to say ', bump libvirt-glib dep' in the summary line (the convention is to keep it as short as possible). Change itself is good. :)

Review of attachment 219146 [details] [review]:

Oh and commit log needs some cleaning here as well: s/validate/valid,/ in the summary

Review of attachment 219148 [details] [review]:

https://live.gnome.org/Git/CommitMessages

Looks good apart from this nonfunctional nitpicks. :)

::: src/libvirt-machine.vala
@@ -274,2 +275,5 @@
     }
 
+    public void try_change_name (string name) throws Boxes.Error {
+        try {
+            // we use libvirt "title" for free form user name

In english, first letter is capitalized in a phrase/sentence. :)

@@ -276,0 +277,5 @@
+    public void try_change_name (string name) throws Boxes.Error {
+        try {
+            // we use libvirt "title" for free form user name
... 2 more ...

* this -> This

* Lines are 120 columns in boxes. While breaking lines might make code more readable, its the opposite for human languages IMO.

@@ -276,0 +277,11 @@
+    public void try_change_name (string name) throws Boxes.Error {
+        try {
+            // we use libvirt "title" for free form user name
... 8 more ...

invalid -> Invalid

Review of attachment 219146 [details] [review]:

::: src/i-properties-provider.vala
@@ +27,3 @@
             } catch (Boxes.Error error) {
                 warning (error.message);
+                if (error is Boxes.Error.INVALID)

fixed

Review of attachment 219148 [details] [review]:

changed, although I am not convinced by any of these rules.. anyway.

Attachment 219147 [details] pushed as 1fa5ae4 - Use explicit GetXML flag name, bump libvirt-glib dep
Attachment 219148 [details] pushed as 5499c36 - libvirt: allow changing machine name

(In reply to comment #11)
> Attachment 219147 [details] pushed as 1fa5ae4 - Use explicit GetXML flag name, bump
> libvirt-glib dep

I agree with Zeeshan here that the 'bump libvirt-glib dep' is out of place in the one line summary and that the rest of the log could have been more descriptive.

(In reply to comment #12)
> (In reply to comment #11)
> > Attachment 219147 [details] [details] pushed as 1fa5ae4 - Use explicit GetXML flag name, bump
> > libvirt-glib dep
> 
> I agree with Zeeshan here that the 'bump libvirt-glib dep' is out of place in
> the one line summary and that the rest of the log could have been more
> descriptive.

For future reference: "accepted_commitnow" means that change itself is good but if I pointed out some mistake/issue in commit log, you need to correct that before pushing the commit.

This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.

Dunno if we also want a more direct way to do it (directly from the collection view, through f2 for example, and/or double click)