GNOME Bugzilla – Bug 722405
Use GNetworkMonitor to show an info bar warning if using a remote doc and network is down
Last modified: 2014-02-02 21:24:54 UTC
- have a GNetworkMonitor instance in GeditApp - add an utility GeditApp::get_network_available that wraps g_network_monitor_get_network_available () - use the utility in the same place where we stat() local files to check remote files - show an appropriate info bar
Created attachment 266975 [details] [review] GNetworkMonitor used to check that network is down. close button added to close the info bar.
Review of attachment 266975 [details] [review]: ::: gedit/gedit-app.h @@ -55,2 +55,3 @@ /*< private > */ GeditAppPrivate *priv; + GNetworkMonitor *monitor; The members should go in GeditAppPrivate: as the name says, the whole point of that struct is to hide implementation details from the public .h ::: gedit/gedit-io-error-info-bar.c @@ -479,1 +479,5 @@ +GtkWidget * +gedit_network_unavailable_info_bar_new (GFile *location) +{ + GtkWidget *info_bar; Use tabs to indent, spaced to align... it is not that difficult and this is becoming a bit repetitive :-) @@ -480,0 +480,73 @@ +GtkWidget * +gedit_network_unavailable_info_bar_new (GFile *location) +{ ... 70 more ... Small error with the empty line ::: gedit/gedit-tab.c @@ +1673,3 @@ + +void + { leave a space before "(" (also in a few places below) @@ +1676,3 @@ + gboolean enable) +{ + gtk_widget_destroy (info_bar); do not align var declarations. I know some of the old code does that, but for new code we prefer not to do it (see the HACKING file for an explanation). @@ +1708,3 @@ + + } + no empty line @@ +1712,3 @@ +} + + This part of the code should go in gedit-app.c We want to connect to the signal just once, not in every tab @@ +1852,3 @@ } + do not add empty lines
Created attachment 267492 [details] [review] Bug 722405-Use GNetworkMonitor to show an info bar warning if using a remote doc and network is down Moved code in the gedit-app.c .
Review of attachment 267492 [details] [review]: No, this is not what I meant: * _gedit_tab_foo() belongs in gedit-tab.c/h * creating the network monitor and connecting to its signal belongs in gedit-app
Created attachment 267612 [details] [review] Bug 722405-Use GNetworkMonitor to show an info bar warning if using a remote doc and network is down Done Changes. Used the set_info_bar () for hiding and showing the info bar.
hi, i think this can be optimized and this code has memory leaks : - For every change in network, you iterate all the application tabs, even if they have a local document. Perhaps, we can maintain a list of tabs who have a remote document. - you call _gedit_tab_set_network_available for every tabs, creating an info bar but uses it only for tabs with a remote documents. For local documents, you aren't destroy this info bar ( bar ), first leak. For remote documents but with a network on, you set to Null the actual info bar with set_info_bar (tab, NULL); but again, you never destroy the one you create. In case of a remote document and no network, set_info_bar (tab, bar); is ok because it destroy the old info bar.
(In reply to comment #6) > hi, > > i think this can be optimized and this code has memory leaks : > > - For every change in network, you iterate all the application tabs, even if > they have a local document. Perhaps, we can maintain a list of tabs who have a > remote document. > > - you call _gedit_tab_set_network_available for every tabs, creating an info > bar > but uses it only for tabs with a remote documents. > > For local documents, you aren't destroy this info bar ( bar ), first leak. > We Can crate the info_bar inside the check of the local_document and if it's not then only we create the info_bar. > For remote documents but with a network on, you set to Null the actual info > bar > with set_info_bar (tab, NULL); but again, you never destroy the one you > create. > > In case of a remote document and no network, set_info_bar (tab, bar); is ok > because it destroy the old info bar. When the network is down then the info bar gets created and when we network is up then it gets destroyed . so I think there is no memory leak. info bar gets eventually destroyed. ( but when the network is down forever then it wont)
Created attachment 267622 [details] [review] Bug 722405-Use GNetworkMonitor to show an info bar warning if using a remote doc and network is down creating info bar depending upon the doc is local or remote.
I modified the patch a bit and committed it. Thanks for your work