Created attachment 271623 [details] [review]
machine: Set the status as the name on name change

Created attachment 271625 [details] [review]
libvirt-machine: Save name on change of "name" property

Created attachment 271626 [details] [review]
libvirt-machine: Save name on change of "name" property

Created attachment 271627 [details] [review]
remote-machine: Save name on change of "name" property

Created attachment 271628 [details] [review]
Add properties-toolbar

Created attachment 271629 [details] [review]
Use properties-toolbar

Created attachment 271630 [details] [review]
Remove name entry from properties

Created attachment 271631 [details] [review]
properties-toolbar: Center the title

Created attachment 271632 [details] [review]
properties-toolbar: Set the "title" style to the title

Created attachment 271633 [details] [review]
editable-entry: Add a way to align all the texts the same way

Created attachment 271634 [details] [review]
machine: Set the status as the name on name change

Review of attachment 271626 [details] [review]:

Some justification in commit log would be nice.

Review of attachment 271627 [details] [review]:

same thing here

Review of attachment 271628 [details] [review]:

I don't think we need to use a different headerbar for this. The title of the GtkHeaderbar could be any widget AFAICT.

If I'm wrong, you still need to provide some justification for this in the commit log.

::: src/properties-toolbar.vala
@@ +31,3 @@
+    /**
+     * Set the name of the current machine when the title entry changed.
+     */

* If its just one liner comment, please use '//'.
* changed -> changes.

(In reply to comment #14)
> Review of attachment 271628 [details] [review]:
> 
> I don't think we need to use a different headerbar for this. The title of the
> GtkHeaderbar could be any widget AFAICT.
> 
> If I'm wrong, you still need to provide some justification for this in the
> commit log.

Since rest of your changes depend on this change, I'll stop reviewing until these comments are addressed.

Comment on attachment 271626 [details] [review]
libvirt-machine: Save name on change of "name" property

>From 318cbde6e9692d6e161fd2fd3c71961fdc44bc42 Mon Sep 17 00:00:00 2001
>From: Adrien Plazas <kekun.plazas@laposte.net>
>Date: Wed, 12 Mar 2014 15:16:02 +0100
>Subject: [PATCH] libvirt-machine: Save name on change of "name" property
>
>If a piece of code want to set the name of a machine, it shouldn't need to know what extra
>actions this particular implementation of "Machine" needs to do to properly set it.
>
>This patch replaces the private try_change_name wich have to be explicitly called to properly
>set the name of a libvirt machine (and can't be accessed directly from outside LibVirtMachine)
>with the private save_name wich is called automatically on every modification of the name.
>
>Prepares the ground for future changes in the machine name setting mechanism.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=692383
>---
> src/libvirt-machine-properties.vala | 20 ++------------------
> src/libvirt-machine.vala            | 16 ++++++++++++++++
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
>diff --git a/src/libvirt-machine-properties.vala b/src/libvirt-machine-properties.vala
>index 2e71d17..e8cf1ab 100644
>--- a/src/libvirt-machine-properties.vala
>+++ b/src/libvirt-machine-properties.vala
>@@ -11,23 +11,6 @@ public LibvirtMachineProperties (LibvirtMachine machine) {
>         this.machine = machine;
>     }
> 
>-    private bool try_change_name (string name) {
>-        try {
>-            var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE);
>-            // Te use libvirt "title" for free form user name
>-            config.title = name;
>-            // This will take effect only after next reboot, but we use pending/inactive config for name and title
>-            machine.domain.set_config (config);
>-
>-            machine.name = name;
>-            return true;
>-        } catch (GLib.Error error) {
>-            warning ("Failed to change title of box '%s' to '%s': %s",
>-                     machine.domain.get_name (), name, error.message);
>-            return false;
>-        }
>-    }
>-
>     private void try_enable_usb_redir () throws GLib.Error {
>         var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE);
> 
>@@ -113,7 +96,8 @@ private string collect_logs () {
>             var property = add_string_property (ref list, _("Name"), machine.name);
>             property.editable = true;
>             property.changed.connect ((property, name) => {
>-                return try_change_name (name);
>+            	machine.name = name;
>+            	return true;
>             });
>             add_string_property (ref list, _("Virtualizer"), machine.source.uri);
>             if (machine.display != null)
>diff --git a/src/libvirt-machine.vala b/src/libvirt-machine.vala
>index 414ee61..ab128e1 100644
>--- a/src/libvirt-machine.vala
>+++ b/src/libvirt-machine.vala
>@@ -176,6 +176,8 @@ else if (force_stopped) {
>         if (state != MachineState.STOPPED)
>             load_screenshot ();
>         set_screenshot_enable (true);
>+        
>+        notify["name"].connect (save_name);
>     }
> 
>     private void update_cpu_stat (DomainInfo info, ref MachineStat stat) {
>@@ -611,4 +613,18 @@ public override void restart () {
> 
>         try_shutdown ();
>     }
>+    
>+    private void save_name () {
>+        try {
>+            var config = domain.get_config (GVir.DomainXMLFlags.INACTIVE);
>+            // Te use libvirt "title" for free form user name
>+            config.title = name;
>+            // This will take effect only after next reboot, but we use pending/inactive config for name and title
>+            domain.set_config (config);
>+        } catch (GLib.Error error) {
>+            warning ("Failed to change title of box '%s' to '%s': %s",
>+                     domain.get_name (), name, error.message);
>+        }
>+    }
>+
> }
>-- 
>1.8.5.3

Created attachment 271708 [details] [review]
libvirt-machine: Save name on change of name property

If a piece of code want to set the name of a machine, it shouldn't need
to know what extra actions this particular implementation of Machine
needs to do to properly set it.

This patch replaces the private try_change_name wich have to be
explicitly called to properly set the name of a libvirt machine (and
can't be accessed directly from outside LibVirtMachine) with the private
save_name wich is called automatically on every modification of the
name.

Prepares the ground for future changes in the machine name setting
mechanism.

Created attachment 271709 [details] [review]
remote-machine: Save name on change of "name" property

Sets the name of the source of a  remote machine as the "name" property
of the machine changed.

Prepares the ground for future changes in the machine name setting
mechanism.

Created attachment 271711 [details] [review]
Add properties-toolbar

Created attachment 271712 [details] [review]
Use properties-toolbar

Avoids cluttering the topbar by relying on a specialized widget.

Created attachment 271714 [details] [review]
Remove name entry from properties

Removes the name entries from the properties pages of libvirt and
remote machines.

They are no more needed as the titlebar serves their purpose.

Created attachment 271716 [details] [review]
properties-toolbar: Center the title

Centers the title entry ofthe properties toolbar.

It is achieved by adding a method to set the alignments of the texts
of an editable entry.

Created attachment 271717 [details] [review]
properties-toolbar: Set the "title" style to the title

The title entry of the properties toolbar now looks like a proper
title.

Created attachment 271718 [details] [review]
editable-entry: Add a way to align all the texts the same way

Modify the method to set the alignment of the texts of an editable
entry to also set the alignment of the entry the same way.

Created attachment 271719 [details] [review]
machine: Set the status as the name on name change

Updates the name displaed in the display's titlebar when the name
changed.

(In reply to comment #17)
> Created an attachment (id=271708) [details] [review]
> libvirt-machine: Save name on change of name property
> 
> If a piece of code want to set the name of a machine, it shouldn't need
> to know what extra actions this particular implementation of Machine
> needs to do to properly set it.
>
> This patch replaces the private try_change_name wich have to be
> explicitly called to properly set the name of a libvirt machine (and
> can't be accessed directly from outside LibVirtMachine) with the private
> save_name wich is called automatically on every modification of the
> name.

I think now you are providing way too much information. There is no need to describe each an every detail of the code in English here.

Just summarize the change and say why its needed. In this particular case the reason is that in a following patch we'll need it for XYZ (make sure you replace XYX accordingly). :)
 

> Prepares the ground for future changes in the machine name setting
> mechanism.

Now this is the important bit but you don't say anything about what kind of future changes will need this change.

Review of attachment 271709 [details] [review]:

For this one you can just c&p the details from previous patch.

(In reply to comment #19)
> Created an attachment (id=271711) [details] [review]
> Add properties-toolbar

I think you totally missed the most important part of the review: comment#14.

Created attachment 271728 [details] [review]
libvirt-machine: Save name on change of the "name" property

If a piece of code want to set the name of a machine, it shouldn't need
to know what extra actions this particular implementation of Machine
needs to do to properly set it.

Replaces try_change_name wich have to be explicitly called to properly
set the name of a libvirt machine with save_name wich is called
automatically on every modification of the name.

Prepares the ground for future changes in the machine name setting
mechanism, we want a simple and public way to set the name of a
machine: its name property.
Needed for bug #692383.

Created attachment 271729 [details] [review]
remote-machine: Save name on change of "name" property

Sets the name of the source of a remote machine as the "name" property
of the machine changed.

Prepares the ground for future changes in the machine name setting
mechanism, we want a simple and public way to set the name of a
machine: its name property.
Needed for bug #692383.

Created attachment 271730 [details] [review]
Add properties-toolbar

Adds the default files of the properties toolbar.

This is needed because the properties toolbar will become more complex
in response to bug #692383, and we should avoid cluttering the topbar
with implementation details of the properties toolbar.

Created attachment 271731 [details] [review]
Use properties-toolbar

Replaces the current implementation of the properties toolbar in
topbar by a separate on to avoid cluttering the topbar.

THe new toolbar can set the name of the machine by clicking on its
title to reaveale a text entry, needed for bug #692383.

Created attachment 271732 [details] [review]
Remove name entry from properties

Removes the name entries from the properties pages of libvirt and
remote machines.

They are no more needed as the titlebar serves their purpose.

Created attachment 271733 [details] [review]
properties-toolbar: Center the title

Centers the title entry of the properties toolbar.

Allows to make the title entry of the properties toolbar look more
like a proper title.

Created attachment 271734 [details] [review]
properties-toolbar: Set the "title" style to the title

The title entry of the properties toolbar now looks like a proper
title (as it should for bug #692383).

Created attachment 271735 [details] [review]
editable-entry: Add a way to align all the texts the same way

Modify the method to set the alignment of the texts of an editable
entry to also set the alignment of the entry the same way, make the
title look more coherent.

Created attachment 271736 [details] [review]
machine: Set the status as the name on name change

Updates the name displayed in the display's titlebar when the name
changed.

Needed for bug #692383.

Created attachment 271763 [details] [review]
libvirt-machine: Save name on change of the "name" property

If a piece of code want to set the name of a machine, it shouldn't need
to know what extra actions this particular implementation of Machine
needs to do to properly set it.

Replaces try_change_name wich have to be explicitly called to properly
set the name of a libvirt machine with save_name wich is called
automatically on every modification of the name.

Prepares the ground for future changes in the machine name setting
mechanism, we want a simple and public way to set the name of a
machine: its name property.
Needed for bug #692383.

Created attachment 271764 [details] [review]
remote-machine: Save name on change of "name" property

Sets the name of the source of a remote machine as the "name" property
of the machine changed.

Prepares the ground for future changes in the machine name setting
mechanism, we want a simple and public way to set the name of a
machine: its name property.
Needed for bug #692383.

Created attachment 271765 [details] [review]
Add the title entry to the properties toolbar

Adds an editable entry as the title of the properties toolbar.

It allows to edit the name of the machine simply by clicking on it, as
needed by bug #692383.

https://bugzilla.gnome.org/show_bug.cgi?id=692383

Conflicts:
	src/properties-toolbar.vala

Created attachment 271766 [details] [review]
Remove name entry from properties

Removes the name entries from the properties pages of libvirt and
remote machines.

They are no more needed as the titlebar serves their purpose.

Created attachment 271767 [details] [review]
properties-toolbar: Center the title

Centers the title entry of the properties toolbar.

Allows to make the title entry of the properties toolbar look more
like a proper title.

Created attachment 271768 [details] [review]
properties-toolbar: Set the "title" style to the title

The title entry of the properties toolbar now looks like a proper
title (as it should for bug #692383).

Created attachment 271769 [details] [review]
editable-entry: Add a way to align all the texts the same way

Modify the method to set the alignment of the texts of an editable
entry to also set the alignment of the entry the same way, make the
title look more coherent.

Created attachment 271770 [details] [review]
machine: Set the status as the name on name change

Updates the name displayed in the display's titlebar when the name
changed.

Needed for bug #692383.

Patches from attachment #271763 [details] to #271770 depends on patches #271755 and #271757 for bug #726252.

Review of attachment 271763 [details] [review]:

ack, I'll change your commits logs though before pushing.

Review of attachment 271764 [details] [review]:

same for this one.

Review of attachment 271765 [details] [review]:

::: src/properties-toolbar.vala
@@ +14,3 @@
+        
+        title_entry = new EditableEntry ();
+        title_entry.editable = true;

why not setup from .ui file?

@@ +17,3 @@
+        
+        title_entry.show ();
+        set_custom_title (title_entry);

you won't have to do this manually either if you setup from .ui file.

@@ +21,3 @@
+        App.app.notify["ui-state"].connect (ui_state_changed);
+        
+        title_entry.editing_done.connect (title_entry_changed);

this can also be done from .ui file.

@@ +30,3 @@
+
+    /**
+     * Set the name of the current machine when the title entry changed.

* redundant doc, its a simple function with simple one line in it so comment makes it less readable rather. Remember, there is always the danger of over-documenting.
* changed -> changes.

@@ +32,3 @@
+     * Set the name of the current machine when the title entry changed.
+     */
+    private void title_entry_changed () {

title_entry_changed -> on_title_entry_changed

@@ +37,3 @@
+    
+    /**
+     * Set the text entry when when entering the properties state.

Same for this comment.

@@ +47,3 @@
+    
+    /**
+     * Set the text entry when the current machine changed.

same here.

@@ +49,3 @@
+     * Set the text entry when the current machine changed.
+     */
+    private void name_changed () {

name_changed -> on_name_changed

::: src/topbar.vala
@@ -46,3 @@
 
-    [GtkChild]
-    private Gtk.HeaderBar props_toolbar;

I would imagine all these changes would have been part of the refactoring patch in bug#726252 ?

Review of attachment 271766 [details] [review]:

looks good

Review of attachment 271767 [details] [review]:

I think this should be part of the patch that adds the PropertiesToolbar.title_entry

Review of attachment 271768 [details] [review]:

This one too!

Review of attachment 271769 [details] [review]:

This too, also all these small changes to style etc can and should be done from .ui file as well.

Review of attachment 271770 [details] [review]:

pretty sure this should also be a part of a previous change. Too many changes to remember, which. :)

Review of attachment 271769 [details] [review]:

wrong status

Review of attachment 271768 [details] [review]:

wrong status

Review of attachment 271767 [details] [review]:

wrong status

Created attachment 272096 [details] [review]
Add the title entry to the properties toolbar

Adds an editable entry as the title of the properties toolbar, centered
and whith the "title" style applyed to make it mimic a title.

It allows to edit the name of the machine simply by clicking on it.

https://bugzilla.gnome.org/show_bug.cgi?id=692383

Conflicts:
	src/properties-toolbar.vala

Comment on attachment 272096 [details] [review]
Add the title entry to the properties toolbar

+        set_custom_title (title_entry);

It is still present because on my machine, it is needed to actually set it as a child, despite of

+    <child type="title">

(In reply to comment #59)
> (From update of attachment 272096 [details] [review])
> +        set_custom_title (title_entry);
> 
> It is still present because on my machine, it is needed to actually set it as a
> child, despite of
> 
> +    <child type="title">

Have you tried seeking help on this on #gtk+ IRC channel? This could be a bug/limitation in gtk+ in which case you want to file a bug and add a FIXME comment here:

// FIXME: Workaround for: URL_OF_BUG

BTW, this is a UI change and I'll be reluctant to ask for an exception this late in the cycle (we are in code-freeze as soon as I roll out the release).

Created attachment 278040 [details] [review]
libvirt-machine: Save name on change of the "name" property

If a piece of code want to set the name of a machine, it shouldn't need
to know what extra actions this particular implementation of Machine
needs to do to properly set it.

Replaces try_change_name wich have to be explicitly called to properly
set the name of a libvirt machine with save_name wich is called
automatically on every modification of the name.

Prepares the ground for future changes in the machine name setting
mechanism, we want a simple and public way to set the name of a
machine: its name property.
Needed for bug #692383.

Created attachment 278041 [details] [review]
remote-machine: Save name on change of "name" property

Sets the name of the source of a remote machine as the "name" property
of the machine changed.

Prepares the ground for future changes in the machine name setting
mechanism, we want a simple and public way to set the name of a
machine: its name property.
Needed for bug #692383.

Created attachment 278042 [details] [review]
Add the title entry to the properties toolbar

Adds an editable entry as the title of the properties toolbar, centered
and whith the "title" style applyed to make it mimic a title.

It allows to edit the name of the machine simply by clicking on it.

Created attachment 278043 [details] [review]
Remove name entry from properties

Removes the name entries from the properties pages of libvirt and
remote machines.

They are no more needed as the titlebar serves their purpose.

Review of attachment 278040 [details] [review]:

* Kudos for trying to put as much info as you can in commit log.

* How about shorter shortlog: Save 'name' prop on change

* 'setting mechanism, we' -> 'setting mechanism. We'.

* 'wich' -> 'which'.

* Duplicate bug references. The URL at the end is enough to indicate this patch is needed for/related to that bug.

::: src/libvirt-machine.vala
@@ +178,3 @@
         set_screenshot_enable (true);
+
+        notify["name"].connect (save_name);

Why can't LibvirtMachineProperties do this? The point of that class is to keep all the code handling LibvirtMachine's settings and keeping this class as simple as possible (its pretty complicated already).

@@ +615,3 @@
     }
+
+    private void save_name () {

the 'try_' prefix was used for a reason: To keep it clear that this can fail.

@@ +618,3 @@
+        try {
+            var config = domain.get_config (GVir.DomainXMLFlags.INACTIVE);
+            // Te use libvirt "title" for free form user name

Might as well fix the typo here while moving the code. :)

@@ +624,3 @@
+        } catch (GLib.Error error) {
+            warning ("Failed to change title of box '%s' to '%s': %s",
+                     domain.get_name (), name, error.message);

If we failed to change the name of machine, we want 'name' property to reflect the old name to not give false illusion to user that change was successful.

Created attachment 283363 [details] [review]
libvirt-machine-properties: Save machine's name on change

This is needed to edit the machine's name in title.

Created attachment 283364 [details] [review]
remote-machine: Save name on change

This is needed to edit the machine's name in title.

Created attachment 283365 [details] [review]
properties-toolbar: Add name title entry

This is needed to edit the machine's name in title.

Created attachment 283366 [details] [review]
Remove name entry from machine properties

This is needed to edit the machine's name in title.

Review of attachment 283363 [details] [review]:

Isn't this just a fix that we want even if we don't want to edit machine's name in title?

If answer to that is 'yes' I'd update the log description to reflect that.

::: src/libvirt-machine-properties.vala
@@ +18,2 @@
     private bool try_change_name (string name) {
+        if (machine.name == name)

do we really need this? Is it really a part of this patch?

Review of attachment 283364 [details] [review]:

ack

Review of attachment 283363 [details] [review]:

What about:

libvirt-machine-properties: Save machine's name on change
This allows to automatically save the machine's name everytime it changes.

Review of attachment 283365 [details] [review]:

"This is needed to edit the machine's name in title."

From what I can tell from the code in the patch and previous patches, we are already doing that, aren't we?

::: src/properties-toolbar.vala
@@ +15,3 @@
                                                                         "go-previous-symbolic";
+
+        set_custom_title (title_entry);

why would you need to call this? <child type="title"> should handle that for you.

Review of attachment 283366 [details] [review]:

"This is needed to edit the machine's name in title."

Not really. We already do that before this patch and this patch is removing name property as there is another way to edit the name.

I think the name should still be kept though and its not wrong to allow multiple ways of editing the name.

Review of attachment 283363 [details] [review]:

Good but i think ever better would be:

With this patch, we now allow editing name through window's title.

Created attachment 283392 [details] [review]
libvirt-machine-properties: Save machine's name on change

With this patch, we now allow editing name through window's title.

Created attachment 283393 [details] [review]
remote-machine: Save name on change

This is needed to edit the machine's name in title.

Created attachment 283394 [details] [review]
properties-toolbar: Add name title entry

This allows to edit the machine's name in the title.

Created attachment 283395 [details] [review]
i-properties-provider: Update name property according to machine's name

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Review of attachment 283392 [details] [review]:

ack

Review of attachment 283393 [details] [review]:

same is true for this patch as previous one, isn't it? Just that previous one was about LibvirtMachine and this is about RemoteMachine. I think log needs update.

Review of attachment 283394 [details] [review]:

Hmm.. so if this patch comes after the previous two, its not true that first patch makes it possible to edit name in title (yet).

::: src/properties-toolbar.vala
@@ +15,3 @@
                                                                         "go-previous-symbolic";
+
+        set_custom_title (title_entry);

As i said in previous review to this patch, we shouldnt' need this. Let me know if/why its needed if so.

Created attachment 283473 [details] [review]
remote-machine: Save name on change

With this patch, we now allow editing name through window's title.

Created attachment 283474 [details] [review]
properties-toolbar: Add name title entry

This allows to edit the machine's name in the title.

Created attachment 283475 [details] [review]
i-properties-provider: Update name property according to machine's name

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Review of attachment 283395 [details] [review]:

ack. For future ref, feel free to use identifiers to keep shortlog short. In thise case Machine.name.

Created attachment 283477 [details] [review]
libvirt-machine-properties: Save machine's name on change

This patch ensures that the machine's name is saved everytime it changes.

Also, this allows editing name through window's title.

Created attachment 283478 [details] [review]
remote-machine: Save name on change

This patch ensures that the machine's name is saved everytime it changes.

Also, this allows editing name through window's title.

Created attachment 283479 [details] [review]
properties-toolbar: Add name title entry

This allows to edit the machine's name in the title.

Created attachment 283480 [details] [review]
i-properties-provider: Update name property according to Machine.name

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Created attachment 283481 [details] [review]
libvirt-machine-properties: Save machine's name on change

This patch ensures that the machine's name is saved everytime it changes.

Also, together with following patches this allows editing name through
window's title.

Created attachment 283482 [details] [review]
remote-machine: Save name on change

This patch ensures that the machine's name is saved everytime it changes.

Also, together with following patches this allows editing name through
window's title.

Created attachment 283483 [details] [review]
properties-toolbar: Add name title entry

This allows to edit the machine's name in the title.

Created attachment 283484 [details] [review]
i-properties-provider: Update name property according to Machine.name

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Review of attachment 283481 [details] [review]:

ack

Review of attachment 283482 [details] [review]:

ack

Review of attachment 283483 [details] [review]:

good otherwise.

::: src/editable-entry.vala
@@ +53,3 @@
     }
 
+    public float text_xalign {

These new API should theoretically be in separate patch.

::: src/properties-toolbar.vala
@@ +36,3 @@
+    private void ui_state_changed () {
+        if (window.ui_state == UIState.PROPERTIES) {
+            window.current_item.notify["name"].connect (() => {

when/where do you disconnect this signal handler?

Review of attachment 283484 [details] [review]:

Good otherwise

::: src/i-properties-provider.vala
@@ +125,3 @@
     }
 
+    public string text {

probably justified to put that in separate patch too. :)

Created attachment 283517 [details] [review]
libvirt-machine-properties: Save machine's name on change

This patch ensures that the machine's name is saved everytime it changes.

Also, together with following patches this allows editing name through
window's title.

Created attachment 283518 [details] [review]
remote-machine: Save name on change

This patch ensures that the machine's name is saved everytime it changes.

Also, together with following patches this allows editing name through
window's title.

Created attachment 283519 [details] [review]
editable-entry: Add text alignment props

This patch will allow to align the title entry's text to look like a
title, and therefore, together with following patches this allows editing
name through window's title.

Created attachment 283520 [details] [review]
properties-toolbar: Add name title entry

This allows to edit the machine's name in the title.

Created attachment 283521 [details] [review]
i-properties-provider: Add 'text' prop

This allows to set a StringProperty's text.

With the next patches, this will allow to keep the properties page's name
entry and the title's name entry consistent.

Created attachment 283522 [details] [review]
libvirt-machine-properties: Update the name property on name change

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Created attachment 283523 [details] [review]
remote-machine: Update the name property on name change

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Review of attachment 283517 [details] [review]:

ack

Review of attachment 283518 [details] [review]:

ack

Review of attachment 283519 [details] [review]:

ack

Review of attachment 283520 [details] [review]:

good otherwise.

::: src/properties-toolbar.vala
@@ +12,3 @@
 
+    private CollectionItem item;
+    private ulong item_name_id = 0;

vala initializes to 0 for you.

@@ +40,3 @@
+        if (window.ui_state == UIState.PROPERTIES) {
+            if (item_name_id != 0)
+                item.disconnect (item_name_id);

so if we never re-enter props, signal never gets disconnected? I think we should disconnect on all other UI states then properties rather.

Review of attachment 283521 [details] [review]:

ack

Review of attachment 283522 [details] [review]:

"Update the name" -> "Update" should be enough for shorlog from context. Good otherwise.

Review of attachment 283523 [details] [review]:

same comment as for prev patch.

Created attachment 283562 [details] [review]
properties-toolbar: Add name title entry

This allows to edit the machine's name in the title.

Created attachment 283563 [details] [review]
i-properties-provider: Add 'text' prop

This allows to set a StringProperty's text.

With the next patches, this will allow to keep the properties page's name
entry and the title's name entry consistent.

Created attachment 283564 [details] [review]
libvirt-machine-properties: Update property on name change

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Created attachment 283565 [details] [review]
remote-machine: Update property on name change

This is necessary to keep the properties page's name entry and the title's
name entry consistent.

Review of attachment 283562 [details] [review]:

good otherwise

::: src/properties-toolbar.vala
@@ +39,3 @@
+    private void ui_state_changed () {
+        if (item_name_id != 0) {
+                item.disconnect (item_name_id);

extra indentation

Review of attachment 283563 [details] [review]:

still ack

Review of attachment 283564 [details] [review]:

ack

Review of attachment 283565 [details] [review]:

ack

Push all with some minor corrections. However some of the patches introduce some new warnings, please fix those:

(gnome-boxes:24570): GLib-GObject-WARNING **: The property GtkMisc:xalign is deprecated and shouldn't be used anymore. It will be removed in a future version.

(gnome-boxes:24570): Gtk-WARNING **: GtkEntry 0x27ea900 is mapped but not child_visible

(gnome-boxes:24570): Gtk-WARNING **: GtkEntry 0x27ea900 is mapped but visible=1 child_visible=0 parent BoxesEditableEntry 0x27ea6b0 mapped=1

Attachment 283517 [details] pushed as a8eeab0 - libvirt-machine-properties: Save machine's name on change
Attachment 283518 [details] pushed as fa7b652 - remote-machine: Save name on change
Attachment 283519 [details] pushed as e60d4c7 - editable-entry: Add text alignment props
Attachment 283562 [details] pushed as b0c71c5 - properties-toolbar: Add name title entry
Attachment 283563 [details] pushed as 75981a0 - i-properties-provider: Add 'text' prop
Attachment 283564 [details] pushed as 5972e46 - libvirt-machine-properties: Update property on name change
Attachment 283565 [details] pushed as 7deb612 - remote-machine: Update property on name change

Shouldn't we mark this bug as solved?

If the warnings still exist would be good to fix them, wouldn't it?

I haven't been able to find any of them, if someone can confirm that they still exist and explain how to reproduce them then I'll investigate.

(In reply to Adrien Plazas from comment #123)
> Shouldn't we mark this bug as solved?

Well I think i fixed them after you completely disappeared and then of course I didn't remember this bug being open. :)