GNOME Bugzilla – Bug 720788
Label for VM name does not look editable, but it is
Last modified: 2016-03-31 13:22:00 UTC
This comes from bug #693840. In the Login section of the Customize mode for a VM, there is a label with the VM's name. You can click on that label and edit the VM's name to your liking. However, the VM's name doesn't look at all like it is in an editable entry. It is by chance that I discovered that I could click it, and edit the name. If it is editable, it should look editable, just like any other normal entry :)
Created attachment 264590 [details] Screenshot of problematic label
Created attachment 287878 [details] [review] editable-entry.ui : patch for bug 720788, button relief added
Review of attachment 287878 [details] [review]: Thanks for the patch Chitra. In the commit log, you need to specify what the patch does and why its needed. You can follow the guidelines here: https://wiki.gnome.org/Git/CommitMessages (there is also a checklist). Please see other commits in the repository for real-world examples.
Created attachment 288287 [details] [review] Commit message changed now :)
Created attachment 288289 [details] Screenshot showing VM label editable The button when clicked, lets you change the name of the VM. Relief property to button added.
Review of attachment 288287 [details] [review]: It would be nice if a designer could give some feedback on this. I'm not sure if we want to have a button there at all but just a text field. (At least in the window.) The button does make clear that you can do something with it but it is nothing else than editable content. We have text fields for this, right? Why do we try to reinvent the wheel in a manner that makes us inconsistent with the rest of gnome? ::: data/ui/editable-entry.ui @@ +21,3 @@ <property name="visible">True</property> <property name="receives_default">True</property> + <property name="relief">GTK_RELIEF_NORMAL</property> According to https://developer.gnome.org/gtk3/stable/GtkButton.html#gtk-button-set-relief GTK_RELIEF_NORMAL is the default value. So this line is redundant. Less code is usually better because its less work to maintain ;)
Review of attachment 288287 [details] [review]: > I'm not sure if we want to have a button there at all but just a text field. This is based on (original) design mockups. But true that we probably want designer input if this patch isn't helping anything or we don't know any simple way to solve this bug.
Judging from the general layout and having things like URI and virtualizer in the properties' *login* section, I wouldn't want to locate what design you're referring to. I think for the short term, I'd stick with having an entry. I really don't understand the benefit of having "editable button". This will be properly addressed in the modal properties design as per bug #733367
(In reply to comment #8) > Judging from the general layout and having things like URI and virtualizer in > the properties' *login* section, I wouldn't want to locate what design you're > referring to. Hmm.. I think it might have been just that Marc-Andre used the same widget for 'Add password' button on wizard's setup page and here. > I think for the short term, I'd stick with having an entry. I really don't > understand the benefit of having "editable button". Sure. Chitra, you now know what to do exactly. :) > This will be properly addressed in the modal properties design as per bug > #733367 That bug is only about putting properties in a separate window/dialog so we'll want another bug (or use this one) for changing the contents of properties pages.
Created attachment 290209 [details] [review] editable-entry.ui: remove button, expose just GtkEntry to make label for vm look editable Remove GtkButton, move the button's child 'button_label' to GtkEntry's child. This is to enable the entry to look editable, so that user doesn't click on button to edit.
Created attachment 290210 [details] [review] editable-entry.vala: remove on_button_click signal handler Removed button from boxes editable entry, hence no signal handler needed for the button.
Created attachment 290211 [details] Screenshot showing VM label editable(without clicking on the button) The gtkentry directly shows up without clicking on the button as before.
(In reply to comment #10) > Created an attachment (id=290209) [details] [review] > editable-entry.ui: remove button, expose just GtkEntry to make label for vm > look editable > > Remove GtkButton, move the button's child 'button_label' to > GtkEntry's child. This is to enable the entry to look editable, > so that user doesn't click on button to edit. Although I retained the functionality, I get the following message when I run Boxes after this patch. (gnome-boxes:6157): Gtk-CRITICAL **: gtk_buildable_add_child: assertion 'iface->add_child != NULL' failed
(In reply to comment #11) > Created an attachment (id=290210) [details] [review] > editable-entry.vala: remove on_button_click signal handler > > Removed button from boxes editable entry, hence no signal handler > needed for the button. Removing the GtkButton renders the enum entry BUTTON useless, but not removed it.
Review of attachment 290209 [details] [review]: Firstly, shortlog is supposed to be very short (<= 50 chars) and prefix isn't supposed to be the whole filename. Secondly, you are working on the wrong module. EditableEntry is used elsewhere too and we only want to replace it in one place. So this patch should simply be replacing that particular usage of EditableEntry with a GtkEntry.
Review of attachment 290210 [details] [review]: same here. I think we can simply drop this one.
commit: 32576729cc04627c328c34e69c27b964b4f15c5d i-props-provider: Separate property for editable strings Lets have a separate property for Editable strings and use simple labels for nonmutable string properties. https://bugzilla.gnome.org/show_bug.cgi?id=720788 commit: ae4d4695c7bbb9297cfb8ed04dd66ff6051174b6 i-props-provider: Use entry for editable strings Instead of using our custom EditableEntry that shows a button to edit, lets just use simple text entry. commit: 441946c828fcdca3fd0ebc557dd8dbe07fc90571 i-props-provider: String change always succeeds We are no longer watching for failures in change of an editable string property and none of the users of this API makes use of this functionality anyway so no reason for EditableStringProperty.changed() to return boolean.