GNOME Bugzilla – Bug 778685
Sort network list in particulat VPNs
Last modified: 2017-05-17 13:21:14 UTC
If a user has a large number of networks (e.g. a lot of VPNs) then the network list becomes hard to navigate. In this case it is important to sort the networks by name. The attached patch keeps the current order by type but adds the title into the sort column to solve the issue.
Created attachment 345834 [details] [review] network: Re-render the title whenever it changes The title of network connections may change if the user edits it or if it was changed programatically by another program. This worked fine, but the UI was not updated. This patch ensures network list in the UI is updated. Reported-by: Oliver Haessler <ohaessle@redhat.com> https://bugzilla.redhat.com/show_bug.cgi?id=1404665
Created attachment 345835 [details] [review] network: Include the network name in the sort string The list of networks is sorted by connection type. If a user has e.g. a lot of VPN connections, then the unsorted list is hard to browse. So include the name of the network connection in the sort string and update it whenever the title changes. Reported-by: Oliver Haessler <ohaessle@redhat.com> https://bugzilla.redhat.com/show_bug.cgi?id=1404665
Review of attachment 345834 [details] [review]: > Subject: [PATCH] network: Re-render the title whenever it changes The title of what? > https://bugzilla.redhat.com/show_bug.cgi?id=1404665 Add "See " at the beginning of the line to soothe git-bz. ::: panels/network/cc-network-panel.c @@ +835,3 @@ nm_device_get_udi (device)) == 0) { gtk_list_store_remove (GTK_LIST_STORE (model), &iter); + g_signal_handlers_disconnect_by_func (object_tmp, That's not needed, the object this relies on will be unref'ed just below. @@ +985,3 @@ -1); + g_signal_connect (proxy, "notify::title", + G_CALLBACK (panel_net_object_notify_title_cb), panel); Unnecessary, the proxy panel never changes title.
Review of attachment 345835 [details] [review]: ::: panels/network/cc-network-panel.c @@ +509,3 @@ + prefix = panel_device_to_sortable_string (net_device_get_nm_device (NET_DEVICE (object))); + } else if (NET_IS_PROXY (object)) { + prefix = "9"; This would require the locale to sort numbers before strings, and we don't know what names the devices might have. You need to use a GtkTreeModelSort instead.
Fair enough, need to fix the message to mention that it is the title of network connections. Disconnecting the signal handler might not be neccessary. Though is there anything speaking against doing so? It e.g. makes sense if the object is still referenced from somewhere else and stays alive for longer (obviously cannot happen in this case). The code *has* to react to title changes for network connections. Title changes may happen when the user modifies a VPN connection or when it is modified programatically through some other utility. About the sorting, are you saying that instead of building on top of the existing string based sorting you would like the code to be refactored to set a sort function on the GtkListStore?
(In reply to Benjamin Berg from comment #5) > Fair enough, need to fix the message to mention that it is the title of > network connections. > > Disconnecting the signal handler might not be neccessary. Though is there > anything speaking against doing so? It e.g. makes sense if the object is > still referenced from somewhere else and stays alive for longer (obviously > cannot happen in this case). It's unnecessary code, so we shouldn't add it if it's not necessary... > The code *has* to react to title changes for network connections. Title > changes may happen when the user modifies a VPN connection or when it is > modified programatically through some other utility. Yes, and? It's only the proxy panel that I asked not to monitor, nothing else. > About the sorting, are you saying that instead of building on top of the > existing string based sorting you would like the code to be refactored to > set a sort function on the GtkListStore? It needs to be on top of the existing sorting, yes.
(In reply to Bastien Nocera from comment #6) > (In reply to Benjamin Berg from comment #5) > > Fair enough, need to fix the message to mention that it is the title of > > network connections. > > > > Disconnecting the signal handler might not be neccessary. Though is there > > anything speaking against doing so? It e.g. makes sense if the object is > > still referenced from somewhere else and stays alive for longer (obviously > > cannot happen in this case). > > It's unnecessary code, so we shouldn't add it if it's not necessary... I would have argued that it might become necessary at some point in the future. In a number of (python) projects I have been bitten exactly because such disconnects which is why I generally prefer to always disconnect handlers. Anyway, I do agree it is not necessary in this particular case. > > The code *has* to react to title changes for network connections. Title > > changes may happen when the user modifies a VPN connection or when it is > > modified programatically through some other utility. > > Yes, and? It's only the proxy panel that I asked not to monitor, nothing > else. Right, fair enough. Personally I would still do it for consistency, but I can add a comment instead to say that a connect is not necessary.
Created attachment 346250 [details] [review] network: Remove unused "title" column from devices liststore The title column is not used as the title is fetched from the net object on the fly when it is needed.
Created attachment 346251 [details] [review] network: Re-render connection title whenever it changes The title of network connections may change if the user edits it or if it was changed programatically by another program. This worked fine, but the UI was not updated. This patch ensures the network list in the UI is updated. Reported-by: Oliver Haessler <oliver@redhat.com>
Created attachment 346252 [details] [review] network: Include the connection title in the sort string The list of networks is sorted by connection type. If a user has e.g. a lot of VPN connections, then the unsorted list is hard to browse. To fix this, include the title of the connection in the sort order and ensure the list is kept sorted when a title is changed. Reported-by: Oliver Haessler <oliver@redhat.com>
This has been sitting here for almost three months now. Could someone review the patches please?
Review of attachment 346252 [details] [review]: ::: panels/network/cc-network-panel.c @@ +533,3 @@ + return cat_a - cat_b; + + return g_utf8_collate (net_object_get_title (obj_a), net_object_get_title (obj_b)); we're leaking obj_a and obj_b's references here @@ +549,3 @@ + /* gtk_tree_model_row_changed would not cause the tree store to resort, + * so instead set the object to itself. */ this is odd, gtk bug?
Review of attachment 346251 [details] [review]: lgtm
Review of attachment 346250 [details] [review]: ok
(In reply to Rui Matos from comment #12) > Review of attachment 346252 [details] [review] [review]: > > ::: panels/network/cc-network-panel.c > @@ +533,3 @@ > + return cat_a - cat_b; > + > + return g_utf8_collate (net_object_get_title (obj_a), > net_object_get_title (obj_b)); > > we're leaking obj_a and obj_b's references here Oh, good catch! > @@ +549,3 @@ > > + /* gtk_tree_model_row_changed would not cause the tree store to > resort, > + * so instead set the object to itself. */ > > this is odd, gtk bug? This assumption is entirely reasonable for any immutable type but falls apart of objects. Honestly, I am not sure whether I should consider this a GTK+ bug or not.
Created attachment 352014 [details] [review] network: Include the connection title in the sort string The list of networks is sorted by connection type. If a user has e.g. a lot of VPN connections, then the unsorted list is hard to browse. To fix this, include the title of the connection in the sort order and ensure the list is kept sorted when a title is changed. Reported-by: Oliver Haessler <oliver@redhat.com> --- Added g_autoptr for the freeing and a bug reference for GtkListStore.
(In reply to Benjamin Berg from comment #11) > This has been sitting here for almost three months now. Could someone review > the patches please? FWIW, all this code will be removed when we redesign the Network panel. You would also avoid the "row-changed" dance if you used a GtkTreeModelSort as mentioned in comment 4.
(In reply to Bastien Nocera from comment #17) > (In reply to Benjamin Berg from comment #11) > > This has been sitting here for almost three months now. Could someone review > > the patches please? > > FWIW, all this code will be removed when we redesign the Network panel. Tough luck then. It still fixes issues in the stable branches though and as far as I can tell, it is sane to land it. > You would also avoid the "row-changed" dance if you used a GtkTreeModelSort > as mentioned in comment 4. Why do you want to add a GtkTreeModelSort into the mix if the underlying storage already implements GtkTreeSortable? Maybe I am missing something obvious, but the only possible advantage I can see right now is working around bug #782737. Other than that we would just keep hold of two GtkTreeModel's for a single consumer. That said, it is trivial to change the code should that be desirable.
Review of attachment 352014 [details] [review]: fine to push to master and 3.24 after fixing that ::: panels/network/cc-network-panel.c @@ +518,3 @@ +{ + g_autoptr(NetObject) *obj_a = NULL; + g_autoptr(NetObject) *obj_b = NULL; g_autoptr() requires that you don't use * here
Pushed to master and gnome-3-24. Attachment 346250 [details] pushed as 93fc508 - network: Remove unused "title" column from devices liststore Attachment 346251 [details] pushed as f70ac1d - network: Re-render connection title whenever it changes Attachment 352014 [details] pushed as 2c95c6a - network: Include the connection title in the sort string