After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 784215 - [review] lr/ui-improvements: some random UI improvements
[review] lr/ui-improvements: some random UI improvements
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: nm-applet
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks: nm-review
 
 
Reported: 2017-06-26 13:28 UTC by Lubomir Rintel
Modified: 2017-07-11 15:57 UTC
See Also:
GNOME target: ---
GNOME version: ---



Comment 1 Piotr Drąg 2017-06-26 13:32:29 UTC
Please don’t update .po files. Damned-lies and translators take care of it in GNOME.
Comment 2 Lubomir Rintel 2017-06-27 09:51:39 UTC
(In reply to Piotr Drąg from comment #1)
> Please don’t update .po files. Damned-lies and translators take care of it
> in GNOME.

Dropped the commit, thanks.

Added some more stuff into the branch, ready for review now.
Comment 3 Thomas Haller 2017-06-28 10:24:07 UTC
`make check` fails


+G_DEFINE_TYPE_WITH_CODE (NMConnectionList, nm_connection_list, GTK_TYPE_DIALOG,
+                         G_ADD_PRIVATE (NMConnectionList))

G_ADD_PRIVATE() is glib 2.38. You'll have to bump the glib requirement, if you intend to use it.



+    /* Initialize the widget template */
+        gtk_widget_class_set_template_from_resource (widget_class,

indentation





+typedef struct {
+    GtkDialog parent;
+    NMConnectionListPrivate *priv;
 } NMConnectionList;

NM_CONNECTION_LIST_GET_PRIVATE() doesn't use the priv field. The priv field could be either dropped, or should be used (as it is a tiny bit more efficient then G_TYPE_INSTANCE_GET_PRIVATE macro).

NM calls this field consistently "_priv", which is required by the _NM_GET_PRIVATE() macro.

Also, if you compare with NetworkManager, the "_priv" field is usually not a pointer, but it's directly embedded in the object struct. It's only a pointer, when the class is subclassed (which NMConnectionEditor is not). For what I consider best-practice see: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4d37f7a1e94f469fb1e3eacde4d2424ebf6ccf0b
(but it's fine with me to just drop priv altogether).




+    data->validation_error = g_strdup (validation_error);
+        update_widget (widget);

indentation (and more, in same file).




In nm-c-e's connection list, when you start typing (or press CTRL+F), you can enter text -- for what I presume is search. But it doesn't work. Either implement it, or disable it.
Also, while the search input field is present, clicking "X" to close nm-c-e doesn't work. Can that be fixed?


A feature request: when editing a connection, accept ESC to "Cancel".


could you comment on bug 783938, otherwise the patch there lgtm and I'd merge it.



Nice improvements overall!!!
Comment 4 Lubomir Rintel 2017-07-03 08:23:04 UTC
(In reply to Thomas Haller from comment #3)
> `make check` fails

Fixed.

> +G_DEFINE_TYPE_WITH_CODE (NMConnectionList, nm_connection_list,
> GTK_TYPE_DIALOG,
> +                         G_ADD_PRIVATE (NMConnectionList))
> 
> G_ADD_PRIVATE() is glib 2.38. You'll have to bump the glib requirement, if
> you intend to use it.

Bumped it. We actually rely on it already, and so does Gtk+ 3.10.

> +    /* Initialize the widget template */
> +        gtk_widget_class_set_template_from_resource (widget_class,
> 
> indentation

Fixed.

> +typedef struct {
> +    GtkDialog parent;
> +    NMConnectionListPrivate *priv;
>  } NMConnectionList;
> 
> NM_CONNECTION_LIST_GET_PRIVATE() doesn't use the priv field. The priv field
> could be either dropped, or should be used (as it is a tiny bit more
> efficient then G_TYPE_INSTANCE_GET_PRIVATE macro).
> 
> NM calls this field consistently "_priv", which is required by the
> _NM_GET_PRIVATE() macro.
> 
> Also, if you compare with NetworkManager, the "_priv" field is usually not a
> pointer, but it's directly embedded in the object struct. It's only a
> pointer, when the class is subclassed (which NMConnectionEditor is not). For
> what I consider best-practice see:
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/
> ?id=4d37f7a1e94f469fb1e3eacde4d2424ebf6ccf0b
> (but it's fine with me to just drop priv altogether).

Removed the extra priv field.

What NetworkManager does doesn't seem to use G_ADD_PRIVATE() and thus G_PRIVATE_OFFSET() and in turn gtk_widget_class_bind_template_child_private() won't work.

> +    data->validation_error = g_strdup (validation_error);
> +        update_widget (widget);
> 
> indentation (and more, in same file).

Fixed.

> In nm-c-e's connection list, when you start typing (or press CTRL+F), you
> can enter text -- for what I presume is search. But it doesn't work. Either
> implement it, or disable it.
> Also, while the search input field is present, clicking "X" to close nm-c-e
> doesn't work. Can that be fixed?
> 
> 
> A feature request: when editing a connection, accept ESC to "Cancel".

Added.

> could you comment on bug 783938, otherwise the patch there lgtm and I'd
> merge it.

Commented. Thought it's merged already since I've seen GTK_WIN_POS_CENTER all around the place.

Updated the branch.
Comment 5 Lubomir Rintel 2017-07-11 15:57:07 UTC
Merged in f84f06a02ac3bfcb17f8550e1060d72fd3d6a2c2