GNOME Bugzilla – Bug 737221
Maps looks terrible when no Internet connection is available
Last modified: 2014-10-18 13:26:40 UTC
Maps looks terrible when no Internet connection is available. Each tile is just a weird icon with a red x. Look at the updates panel in gnome-software for an example of how to nicely handle a missing Internet connection.
Yeah, we meant to fix this but I guess it got lost. Anyone itching for fixing this?
Maybe one can look here for ways to determine network-status: https://developer.gnome.org/gio/unstable/GNetworkMonitor.html
I am able to reproduce the issue.Now what is requirement & expectation of bug-fix ?
Cool that you want to help out! This is currently worked on by someone else. So it would be better if you found another bug.
Created attachment 287729 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view. can_reach gives an error with resolving proxy.iiit.ac.in hence still implemented network_available. Have to fix that.
Review of attachment 287729 [details] [review]: Thanks! This will be awesome when done! We also need to think about what to do with the buttons in the headerbar when there is no connection. There is the possibility of binding properties: https://developer.gnome.org/gobject/unstable/GBinding.html Read that. It will be useful for you, not just here. So if we add a property to the main-window for instance "connected" (See routeQuery.js for example of how to add property) and set that to true/false in the checkNetwork function. Then we could bind the sensitive property of the buttons to that property with the INVERT_BOOLEAN flag. That might be one way. You can specify property binding in ui files as well, with attributes on a property tag. bind-source="name-of-bind-source" bind-property="the property on the source to bind" bind-flags="sync-create". But not sure we can apply that here. Thanks! ::: src/application.js @@ +98,2 @@ GtkClutter.init(null); + What has happend here? Some whitespace damage? Please go 'git diff --check' before 'git add' to check for whitespace damage. ::: src/main-window.ui @@ +84,3 @@ <property name="can_focus">False</property> + <child> + <object class="GtkStack" id="stack"> Call it main-stack! @@ +89,3 @@ + <property name="transition-type">crossfade</property> + <child> + <object class="GtkSpinner" id="spinner"> Maybe network-spinner? @@ +95,3 @@ + </child> + <child> + <object class="GtkGrid" id="nonetwork"> I think no-network-view @@ +111,3 @@ + <object class="GtkLabel" id="no-network-conn-message"> + <property name="visible">True</property> + <property name="label">Could not connect to openstreetmap, check your connection and proxy setting</property> This message needs to be translatable, add an attribute: translatable="true", look elsewhere in this file for example. Also maybe this text needs to have some margin to the image? Maybe set margin-top property to something like 10? And the font should probably be increased. Include a screenshot of this view in the next version so designers can look. Also 'setting' => 'settings. ::: src/mainWindow.js @@ +61,3 @@ 'layers-button']); + + Do not add empty lines. @@ +91,3 @@ + Application.monitor.connect('network-changed', + this._Changeview.bind(this) + ); Move this to _initSignals, and change the name of the handler to this._checkNetwork, see below for more. @@ +96,3 @@ + this._mainStack = ui.stack; + this._mainStack.add(this._overlay, "child1"); You do not need to add the name, just this._mainStack.add(this._overlay) is ok. @@ +97,3 @@ + this._mainStack = ui.stack; + this._mainStack.add(this._overlay, "child1"); + this.view2 = ui.nonetwork; Call the variable this._noNetworkView. The underscore is for private variables. @@ +104,3 @@ + else + this._mainStack.set_visible_child(this.view2); + So I think you should move all of this to a function this._checkNetwork and call it last in _init. The same function that will run on the 'network-changed' signal. And do not call get_network_available in the function, instead do something like: let addr = new Gio.NetworkAddress({ hostaname: 'tile.openstreetmap.org', port: 80 }); try { if (Application.monitor.can_reach(addr, null)) this._mainStack.set_visible_child(this._overlay); } catch(e) { this._mainStack.set_visible_child(this._noNetworkView); Utils.debug('Connection failed: ' + e.message); } What do you think?
Review of attachment 287729 [details] [review]: Press the review link to see detailed comments.
Ok great ! Thanks for reviewing. I will fix all the above-mentioned errors and also upload the video for the designer :) ! Thanks again.
Also, you shouls use properties directly when possible = mainStack.visible_child =...
Another thing :-) you should use the async version of can_reach, if there is one, otherrwise it will block, and the spinner wont spin, look at the code in geocodeService.js for how to handle async callbacks
Created attachment 287766 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view. Implements can_reach. Have to shift to can_reach_async. Also have to work on the UI
https://drive.google.com/file/d/0Bzg51XIEYFJOSDA0TF9nMUZmUzg/view?usp=sharing This is the link to the screencast of how gnome-map works when the network switched on and off. Any suggestions are welcome as to what can be done to improve the UI! Thanks :)
(In reply to comment #12) > https://drive.google.com/file/d/0Bzg51XIEYFJOSDA0TF9nMUZmUzg/view?usp=sharing > > This is the link to the screencast of how gnome-map works when the network > switched on and off. Any suggestions are welcome as to what can be done to > improve the UI! Thanks :) Hmm.. I only see one issue with this solution: Maps is rendered completely useless on 'no connection'. libchamplain heavily caches maps tiles so if we have enough tiles to show, I don't think we should cover them up with an error that the user might not be able to fix (happens quite a lot in UK :)). Not sure what could be a good solution though but I have a feeling that this might be better solved in libchamplain.
(In reply to comment #13) > (In reply to comment #12) > > https://drive.google.com/file/d/0Bzg51XIEYFJOSDA0TF9nMUZmUzg/view?usp=sharing > > > > This is the link to the screencast of how gnome-map works when the network > > switched on and off. Any suggestions are welcome as to what can be done to > > improve the UI! Thanks :) > > Hmm.. I only see one issue with this solution: Maps is rendered completely > useless on 'no connection'. libchamplain heavily caches maps tiles so if we > have enough tiles to show, I don't think we should cover them up with an error > that the user might not be able to fix (happens quite a lot in UK :)). Not sure > what could be a good solution though but I have a feeling that this might be > better solved in libchamplain. Yeah, that is a limitation. What it solves is not showing all red tiles and no information at start-up. Something quite a few has experienced. And I have had to tell them to check their system-wide proxy settings. But, really. Atm Maps is pretty useless with no connection. And what we are seeing now (The images with red X's) is libchamplains current solution. They come from an error source, when we fail to get images from the networksource. The caching in libchamplain is there to speed things up, not to make it kind-of work in offline mode. Until we have code to handle offline maps, with knowledge of which region we currently have offline I think letting the users browse around and sometimes getting cached tiles and sometimes a bunch of red X's is giving a sloppy impression. So I think for the moment something like this is for the best. And we could create a bug to get better behavior. I think making changes in libchamplain will be quite non-trivial since it goes against how it is design atm. But we do want offline data at some point. We are just not there yet. So I think strongly yes on having this on start-up. And we can discuss if we want some different effect while running (in the network-changed callback), mayve an in app notification "Network connection dropped". But I am leaning towards just showing this view, since again it is kind of useless without connection and relying on pretty ad-hoc caching seems bad.
Review of attachment 287766 [details] [review]: Thanks! Looks cool! Now to make the code just as pretty! (Nit about commit message, just have the link at the bottom, not Bug:) ::: src/application.js @@ +98,1 @@ GtkClutter.init(null); Do not remove empty lines. ::: src/main-window.ui @@ +84,3 @@ <property name="can_focus">False</property> + <child> + <object class="GtkStack" id="mainstack"> main-stack (will be ui.mainStack) @@ +95,3 @@ + </child> + <child> + <object class="GtkGrid" id="nonetworkview"> no-network-view (will be ui.noNetworkView) @@ +105,3 @@ + <property name="visible">True</property> + <property name="pixel-size">48</property> + <property name="icon-name">network-cellular-offline-symbolic</property> Oh is the cellular one prettier? Might be. Andreas, what do you think? ::: src/mainWindow.js @@ +61,2 @@ 'layers-button']); + Empty line added, please remove. @@ +178,3 @@ + Application.monitor.connect('network-changed', + this._checkNetwork.bind(this) + ); Have this ending parenthesis on the line above, no new line for it. @@ +182,3 @@ + + _checkNetwork: function() { + let addr = new Gio.NetworkAddress({hostname:'tile.openstreetmap.org', port:80}); Have the port on the line below, and add a space after the '{' and before the '}' @@ +184,3 @@ + let addr = new Gio.NetworkAddress({hostname:'tile.openstreetmap.org', port:80}); + try { + if(Application.monitor.can_reach(addr,null)) Space after if => if (Application... And space after the comma => addr, null @@ +185,3 @@ + try { + if(Application.monitor.can_reach(addr,null)) + this._mainStack.set_visible_child(this._overlay); this._mainStack.visible_child = this._overlay; @@ +187,3 @@ + this._mainStack.set_visible_child(this._overlay); + } catch(e) { + this._mainStack.set_visible_child(this._noNetworkView); this._mainStack.visible_child = this._noNetworkView;
Oh and btw, Shipra: libchamplain is the library that provides the map widget that we use. It is in your jhbuild checkout-dir, it's a dependency to us. It fetches tiles from, among other places, openstreetmap and provides some caching as well. https://wiki.gnome.org/action/show/Projects/libchamplain?action=show&redirect=libchamplain
Review of attachment 287766 [details] [review]: ::: src/main-window.ui @@ +112,3 @@ + <property name="visible">True</property> + <property name="margin-top">10</property> + <property name="label" translatable="yes">Could not connect to openstreetmap, check your connection and proxy settings</property> "Could not connect to the internet, check your connection and proxy settings." Notice "the internet" and the ending ".". :) Great Work btw!
Created attachment 287796 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Thanks for all your suggestions and reviews. I have corrected all the errors and submitted another patch! Thanks again :)
Review of attachment 287796 [details] [review]: Thanks, looks much better! For the async version of can_reach you should use it pretty much as forward.search_async in geocodeService.js: ...can_reach_async(addr, null, (function(montitor, res) { [...] }).bind(this)); Some nit picks: You should remove the 'Bug : ' from the last line in the commit log. ::: src/application.js @@ +98,1 @@ GtkClutter.init(null); You are still removing a line here? ::: src/mainWindow.js @@ +176,3 @@ this._viewMovedId = 0; + Application.monitor.connect('network-changed', + this._checkNetwork.bind(this)); Move this to before 'this._viewMovedId = 0;'. Alternative I would welcome a patch that removes 'this._viewMovedId = 0;' since that variable does not seem to be used anywhere. Mattias/Zeeshan, do you know why this is? :) @@ +188,3 @@ + this._mainStack.visible_child = this._noNetworkView; + Utils.debug('Connection failed: '+ e.message); + } Nit: space before '+' => ...failed: ' + e.message);
Created attachment 287803 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Looks very good! I would just do some grammar fixes. Openstreetmap -> OpenStreetMap Check your... -> Please check your... Line break between error message and the suggestions Could not connect to OpenStreetMap Please check your connection and proxy settings
(In reply to comment #15) > @@ +105,3 @@ > + <property name="visible">True</property> > + <property name="pixel-size">48</property> > + <property > name="icon-name">network-cellular-offline-symbolic</property> > > Oh is the cellular one prettier? Might be. Andreas, what do you think? Cellular is very network-type-specific. I would rather go with network-offline-symbolic
(In reply to comment #22) > Looks very good! I would just do some grammar fixes. > Openstreetmap -> OpenStreetMap > Check your... -> Please check your... > Line break between error message and the suggestions > > > Could not connect to OpenStreetMap > Please check your connection and proxy settings The reason for not saying OpenStreetMap there is that we're using lots of different services, and connecting to OSM is just needed for one type of functionality (geocoding). Maybe something like: "Maps need an active internet connection to function properly, but one can't be found. Check your connection and proxy settings."
(In reply to comment #24) > (In reply to comment #22) > > Looks very good! I would just do some grammar fixes. > > Openstreetmap -> OpenStreetMap > > Check your... -> Please check your... > > Line break between error message and the suggestions > > > > > > Could not connect to OpenStreetMap > > Please check your connection and proxy settings > > The reason for not saying OpenStreetMap there is that we're using lots of > different services, and connecting to OSM is just needed for one type of > functionality (geocoding). > And fetching tiles, the code atm tests against tiles.openstreetmap.org. So this also catches the case when the tile server is down, if it ever is. > Maybe something like: > "Maps need an active internet connection to function properly, but one can't be > found. Check your connection and proxy settings." Works for me as well. Tove, the person I live with, suggested something funnyish like how "Firefox does it when there is a problem". Anyone want to spin something of that? :)
Created attachment 287978 [details] [review] Switched to the non-cellular icon. And changed the message that is displayed when there is no network
(In reply to comment #25) > Tove, the person I live with, suggested something funnyish like how "Firefox > does it when there is a problem". Anyone want to spin something of that? :) I like that! So basically a larger icon to the left, and then a larger text header saying something like "Maps is offline!" and then the current text below that. I'll attach a screenshot of the firefox offline page now with some annotations. :)
Created attachment 288020 [details] The no network page from Firefox with some annotations.
Created attachment 288074 [details] No network connection view
Created attachment 288081 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view. Brought _checkNetwork to application.js Added property connected
This is the final commit. I am sorry I messed up with git commits and patches in the end and ended up committing the entire thing instead of splitting it into two patches. Now, is there any way where I can split this into two? And also please review. Does this look fine?
You can use 'git reset @^' to unstage your changes, then 'git add -p' to stage only portions of a file at a time.
Review of attachment 288081 [details] [review]: Thanks! It looks nice! There are some whitespace problems with this patch. You should use git diff --check before submitting. Also, please check for tab-in-indent: http://kparal.wordpress.com/2011/07/04/git-tip-of-the-day-check-for-whitespace-errors-in-diff/ We use spaces to indent. ::: src/application.js @@ +83,3 @@ /* Translators: This is the program name. */ GLib.set_application_name(_("Maps")); + monitor = Gio.NetworkMonitor.get_default() This should go in the _initServices function. @@ +88,3 @@ + this._checkNetwork(); + monitor.connect('network-changed', + this._checkNetwork.bind(this)); Setting this._connected to false here is fine. But I think we should move the this._checkNetwork() call to after we have created the main window. In vfunc_activate(). And the monitor.connect could go last in _initServices(). By doing it that way we only need to listen to the notify signal in mainWindow and never check connected explicitly. @@ +92,3 @@ + + _checkNetwork: function() { + let addr = new Gio.NetworkAddress({ hostname:'tile.openstreetmap.org', Whitespace problems with this line. ::: src/main-window.ui @@ +134,3 @@ + </child> + </object> + </child> Please make sure you only use spaces for indent in this file and that the indentation is proper. ::: src/mainWindow.js @@ +27,3 @@ const Champlain = imports.gi.Champlain; const GObject = imports.gi.GObject; +const Gio = imports.gi.Gio; This is not needed anymore, right? @@ +93,3 @@ + this._mainStack.add(this._overlay); + this._noNetworkView = ui.noNetworkView; + this._checkConnected(); There is no need to call this here if we move the check in application.js to after the window has been created. @@ +175,3 @@ this._overlay.grab_focus.bind(this._overlay)); + this.window.application.connect('notify::connected', + this._checkConnected.bind(this)); This could be done as an anonymous function: this.window.application.connect('notify::connected', (function() { ... }).bind(this)); @@ +253,3 @@ + _checkConnected: function() { + if(this.window.application.connected == true) Either use: if (this.window.application.connected) or if (this.window.application.connected === true) I think I prefer the former. Please read: http://www.sitepoint.com/javascript-truthy-falsy/ @@ +257,3 @@ + else if(this.window.application.connected == false) + this._mainStack.visible_child = this._noNetworkView; + Utils.debug(this.window.application.connected); This debug message will only print true or false, hardly informational?
Created attachment 288093 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view. Fixed indent problems and moved function calls to appropriate locations.
Created attachment 288094 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view. Moved checkNetwork function call to application.js and added connected property.
Created attachment 288095 [details] [review] Check for the connected property in mainWindow.js
Review of attachment 288095 [details] [review]: Thanks looks great! But the main-window.ui changes belong here :) Also maybe for short log: mainWindow: Switch to no-network-view on no connection Or something similar? ::: src/mainWindow.js @@ +172,3 @@ this.mapView.view.connect('button-press-event', this._overlay.grab_focus.bind(this._overlay)); + this.window.application.connect('notify::connected', (function(app) { No need for the app argument here.
Review of attachment 288094 [details] [review]: Thanks! The main-window changes should go in the other patch, right? Also there are still tabs used in indent in it. Maybe shortlog: application: Add 'connected' property And move the parts about no network view to the next patch. No need for (part x) since each patch can stand on its own justification. ::: src/application.js @@ +83,2 @@ /* Translators: This is the program name. */ GLib.set_application_name(_("Maps")); You removed an empty line here. @@ +151,3 @@ + monitor = Gio.NetworkMonitor.get_default(); + monitor.connect('network-changed', + this._checkNetwork.bind(this)); Can't this be one line?
Created attachment 288097 [details] This is how I envision it. Note: - the horizontal and vertical alignment (centered). - the text paragraph isn't as wide as before. - the icon is a bit smaller.
Ohh ok :) I will work on the UI and have it ready asap ! Thanks :D
Created attachment 288106 [details] like mattias mockup, but align fix I fully agree with Mattias mockup. One tiny little change though, the header needs to be aligned with the rest of the text. Otherwise the hierarchy breaks a bit.
Created attachment 288190 [details] No network connection view
Created attachment 288191 [details] [review] Added connected property to application.js
Created attachment 288192 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Umm I have updated the view and attached a .png file. Along with that I fixed the errors mentioned in the reviews. Also, now headerbar goes insensitive in absence of a network connection.
Review of attachment 288191 [details] [review]: Thanks! This looks nice! It did not work for me tho :) I needed to remove this line from the bottom of application.js: Utils.addSignalMethods(Application.prototype); And you should do that as well in your patch I think. It is only used if we are to use the Javascript way of signals. But we need to use the GObject signal system to have notify work.
Review of attachment 288192 [details] [review]: Awesome! Some nits below. Also when I checked with git diff --check (I also check for tab in indent) it told me: src/main-window.ui:112: tab in indent. + <child> src/main-window.ui:115: tab in indent. + <property name="visible">True</property> src/main-window.ui:122: tab in indent. + <property name="label" translatable="yes">Maps is offline!</property> src/main-window.ui:130: tab in indent. + <property name="ypad">15</property> src/main-window.ui:132: tab in indent. + <property name="label" translatable="yes">Maps need an active internet connection to function properly, but one can't be found. Could you fix that up as well? With those fixes, and an ok from Mattias/Andreas I think we are getting close to merging this. ::: src/mainWindow.js @@ +179,2 @@ this.mapView.view.connect('button-press-event', this._overlay.grab_focus.bind(this._overlay)); Would like an empty line here between these connects. @@ +191,3 @@ + let placeEntry = this._createPlaceEntry(); + this._headerBar.set_custom_title(placeEntry); + placeEntry.has_focus = true; Maybe an empty line here? @@ +203,3 @@ + this.window.application.bind_property('connected', + placeEntry,'sensitive', + GObject.BindingFlags.DEFAULT); Great that you try to avoid going over 80 chars :) But maybe we could do: let app = this.window.application; At the top and then we can indent these in the same way as othher stuff?
Created attachment 288205 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Created attachment 288208 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Created attachment 288209 [details] [review] Added connected property to application.js
Review of attachment 288208 [details] [review]: Looks good to me, Andreas, Mattias is the ui fine now?
Review of attachment 288209 [details] [review]: This still does not remove the Utils.addSignals... :(
Review of attachment 288208 [details] [review]: Thanks, Please check the indentation in main-window.ui there are still tabs there. ::: src/main-window.ui @@ +121,3 @@ + <property name="max-width-chars">60</property> + <property name="label" translatable="yes">Maps is offline!</property> + <property name="xalign">0.08</property> The property xalign is deprecated and should not be used in new code. Try using margin-start? instead maybe? @@ +128,3 @@ + <property name="wrap">True</property> + <property name="max-width-chars">60</property> + <property name="ypad">15</property> The property ypad is deprecated and should not be used in new code.
Review of attachment 288209 [details] [review]: Don't need to remove Utils.addSignalsMethods sinces that was done in [1], maybe rebasing this patch-set can do the work. [1] https://git.gnome.org/browse/gnome-maps/commit/?id=f30888fdb1d05de9e1295565aa6be77626df152c ::: src/application.js @@ +55,3 @@ let geoclue = null; let geocodeService = null; +let monitor = null; Hmmm... I really don't feel comfortable with "monitor" as variable name, looks too generic, what about networkMonitor? @@ +90,3 @@ + _checkNetwork: function() { + let addr = new Gio.NetworkAddress({ hostname:'tile.openstreetmap.org', + port:80 }); An extra space after colon would be nice here: hostname: 'tile.openstreetmap.org', port: 80 @@ +92,3 @@ + port:80 }); + + monitor.can_reach_async(addr, null, (function(monitor, res) { Should we do this always, even if GNetworkMonitor::network-available is false? maybe yes, since we can have tile.openstreemap.org mapped to localhost, but took my attention anyway :)
Review of attachment 288209 [details] [review]: Also, few notes on the commit log: - Remove the space before colon (:), I mean, "application: ..." instead of "application : ..." (same on the next patch). - Commit description must be written in present form and finished with a period.
Review of attachment 288208 [details] [review]: Same note about the commit log :) , no space before colon. Also, something really weird is happening to me after applying the patches, Maps is using a lot of CPU and I only see a spinner indefinitely, no tiles at all :( ::: src/main-window.ui @@ +20,3 @@ <property name="can-focus">True</property> <property name="valign">center</property> + <property name="sensitive">True</property> I don't think this is needed, widgets are sensitive by default. @@ +40,3 @@ <child> <object class="GtkMenuButton" id="layers-button"> + <property name="sensitive">True</property> Same here @@ +59,3 @@ <child> <object class="GtkToggleButton" id="toggle-sidebar-button"> + <property name="sensitive">True</property> Same here ::: src/mainWindow.js @@ +84,2 @@ ui.headerBar.set_custom_title(placeEntry); placeEntry.has_focus = true; There are headerBar initialization code here, must be removed since we now have the _initHeaderBar method @@ +191,3 @@ + _initHeaderbar: function() { + let placeEntry = this._createPlaceEntry(); + this._headerBar.set_custom_title(placeEntry); This is a good opportunity to use the property setter notation here: this._headerBar.custom_title = placeEntry; @@ +206,3 @@ + app.bind_property('connected', + placeEntry,'sensitive', + GObject.BindingFlags.DEFAULT); Space after comma (,) in these bind_property calls
(In reply to comment #51) > Review of attachment 288208 [details] [review]: > > Looks good to me, Andreas, Mattias is the ui fine now? Does anyone have a screenshot? I'm sorry I don't have jhbuild running yet, I'll take care of this tomorrow.
Created attachment 288294 [details] Screenshot of latest version Here's a screenshot for you Andreas!
(In reply to comment #51) > Review of attachment 288208 [details] [review]: > > Looks good to me, Andreas, Mattias is the ui fine now? There's some alignment issues still. Compare the current version (https://bugzilla.gnome.org/attachment.cgi?id=288294) with Andreas' mockup (https://bugzilla.gnome.org/attachment.cgi?id=288106). I really like how the headerbar gets insensitive when the internet connection is down btw, great work Shipra!
Created attachment 288302 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Created attachment 288303 [details] [review] Added connected property to application.js
Ok I have made the required changes and fixed the view. @Mattias Thanks so much! I fixed the alignment issues. Hope it looks better now :) @Damian The error was occuring because of the Utils.addSignalsMethod in application.js.I removed that in my patch. It should work now. Also I fixed all the bugs that you suggested. @Jonas I removed the deprecated tags and also the addSignalsMethods in application.js :) Thanks everyone for reviewing my patch.
(In reply to comment #62) > @Damian The error was occuring because of the Utils.addSignalsMethod in > application.js.I removed that in my patch. It should work now. Ahh... I tried it again and works like a charm, awesome work! But I still think you need to rebase your patches in the current master, since your work from this issue [1] was pushed in master a couple of days ago, and you already removed the addSignalMethods call there. [1] https://bugzilla.gnome.org/show_bug.cgi?id=733414 > Also I fixed all the bugs that you suggested. Great!
Review of attachment 288303 [details] [review]: Nice! Despite the following comments, I like this one. ::: src/application.js @@ +152,3 @@ + networkMonitor = Gio.NetworkMonitor.get_default(); + networkMonitor.connect('network-changed', + this._checkNetwork.bind(this)); Alignment error here. @@ -147,3 @@ } }); -Utils.addSignalMethods(Application.prototype); See comment#63 from this bug, better if fetch the latest changes from Git and rebase your patches, so this removal is not needed. Ping me on IRC if you have any question about Git usage here, I'm not an expert but I can help :) (username: eagleoneraptor)
Review of attachment 288303 [details] [review]: ::: src/application.js @@ -147,3 @@ } }); -Utils.addSignalMethods(Application.prototype); Yeah. This still applies on master and seem to do the right thing, but it's good practice for you to rebase it. Before submitting anything to bugzilla make sure your patches apply cleanly on the current git master.
(In reply to comment #51) > Review of attachment 288208 [details] [review]: > > Looks good to me, Andreas, Mattias is the ui fine now? No, not yet. Contrast: Screenshot: https://bug737221.bugzilla-attachments.gnome.org/attachment.cgi?id=288294 Mockup: https://bug737221.bugzilla-attachments.gnome.org/attachment.cgi?id=288106
Review of attachment 288302 [details] [review]: We are almost there! :) You still have tab in the indents on main-window.ui. You could do something like this: alias tabcheck='git -c core.whitespace=tab-in-indent diff --check' To have an alias for it, or add it to your git config to always have it on. ::: src/main-window.ui @@ +116,3 @@ + +Maps need an active internet connection to function properly, but one can't be found. +Check your connection and proxy settings.</property> I am not conviced by this. I think you should have a GtkGrid here, with vertical alignment, that sets the margin-start property to get the proper alignment to the image for the entire grid. And in the grid have the two labels.
(In reply to comment #67) > Review of attachment 288302 [details] [review]: > > We are almost there! :) > > You still have tab in the indents on main-window.ui. > > You could do something like this: > alias tabcheck='git -c core.whitespace=tab-in-indent diff --check' > To have an alias for it, or add it to your git config to always have it on. > > ::: src/main-window.ui > @@ +116,3 @@ > + > +Maps need an active internet connection to function properly, but one can't be > found. > +Check your connection and proxy settings.</property> > > I am not conviced by this. I think you should have a GtkGrid here, with > vertical alignment, that sets the margin-start property to get the proper > alignment to the image for the entire grid. And in the grid have the two > labels. And with the grid you can set the spacing or similar property to get a spacing between the two labels to be as close to the mockup as possible.
Review of attachment 288302 [details] [review]: ::: src/main-window.ui @@ +118,3 @@ +Check your connection and proxy settings.</property> + <property name="use_markup">True</property> + <child> Agree with Jonas. Also, for the first label, the larger one, it's better if you don't include markup as translatable content since it's not the best practice [1]. According to Andreas mockup, you also need to make the larger label bold. Removing the "<property name="use_markup">True</property>" line to use the default value and adding this to the label object should works: <attributes> <attribute name="weight" value="bold"/> <attribute name="scale" value="1.2"/> </attributes> Finally, in the second label you use line breaks between sentences, this also isn't the good l10n practice [2]. I think it'd be better to put everything in one line and play with the word-wrap related GtkLabel properties. [1] https://wiki.gnome.org/TranslationProject/DevGuidelines/Avoid%20markup%20wherever%20possible [2] https://wiki.gnome.org/TranslationProject/DevGuidelines/Avoid%20hard-coded%20line%20breaks ::: src/mainWindow.js @@ +180,3 @@ + this._mainStack.visible_child = this._overlay; + else + this._mainStack.visible_child = this._noNetworkView; Would be better to use the visible-child-name instead? not sure though. @@ +192,3 @@ + let app = this.window.application; + app.bind_property('connected', + this._gotoUserLocationButton,'sensitive', I still see no space after comma here, what I meant is: this._gotoUserLocationButton, 'sensitive', Instead of this._gotoUserLocationButton,'sensitive', Same below.
Review of attachment 288302 [details] [review]: Also, Shipra: Would it be a problem for you to make sure that the author is your full name on the patches? It would look nicer in the git commit log.
Created attachment 288368 [details] [review] Added connected property to application.js
Created attachment 288369 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Created attachment 288370 [details] [review] No network connection view
Created attachment 288371 [details] No network connection view
Review of attachment 288369 [details] [review]: Thanks, looks fine otherwise! ::: src/main-window.ui @@ +113,3 @@ + <property name="margin-start">30</property> + <child> + <object class="GtkLabel" id="message1"> maybe id no-network-conn-header? @@ +121,3 @@ + <property name="label" translatable="yes">Maps is offline!</property> + <property name="halign">GTK_ALIGN_START</property> + <property name="margin-left">18</property> This should not be here? 1) margin-left is deprecated (should be margin-start as above). 2) It should not be needed since you set it on the grid above. I still see different alignments on the two labels. Did you forget to update the patch? @@ +125,3 @@ + </child> + <child> + <object class="GtkLabel" id="message2"> Maybe id: no-network-conn-text @@ +127,3 @@ + <object class="GtkLabel" id="message2"> + <property name="wrap">True</property> + <property name="max-width-chars">47</property> 47? :) Is it totally arbitrary or does 47 look the best? A round number like 50 might be prettier otherwise? @@ +129,3 @@ + <property name="max-width-chars">47</property> + <property name="margin_top">15</property> + <property name="xalign">0.7</property> xalign is deprecated and should not be used.
Review of attachment 288368 [details] [review]: Thanks, looks great!
Review of attachment 288369 [details] [review]: My UI nits below. Great work again Shipra! ::: src/main-window.ui @@ +116,3 @@ + <attributes> + <attribute name="weight" value="bold" /> + <attribute name="scale" value="1.2" /> 2.0 should be a good scale to get that big header from the mockups. @@ +118,3 @@ + <attribute name="scale" value="1.2" /> + </attributes> + <property name="max-width-chars">15</property> This shouldn't be needed. @@ +120,3 @@ + <property name="max-width-chars">15</property> + <property name="label" translatable="yes">Maps is offline!</property> + <property name="halign">GTK_ALIGN_START</property> GTK_ALIGN_START → start @@ +121,3 @@ + <property name="label" translatable="yes">Maps is offline!</property> + <property name="halign">GTK_ALIGN_START</property> + <property name="margin-left">18</property> Agree, just set the margin-start on the grid (and set it to 15 :)). Regarding the bad alignment of the labels, see below. @@ +127,3 @@ + <object class="GtkLabel" id="message2"> + <property name="wrap">True</property> + <property name="max-width-chars">47</property> 42 seems best from a quick test actually (and will make it look like in the mockup). It also just happens to be the Answer to The Ultimate Question of Life, the Universe, and Everything, which is a good sign. @@ +129,3 @@ + <property name="max-width-chars">47</property> + <property name="margin_top">15</property> + <property name="xalign">0.7</property> However it is needed here. It should be set to 0 and then we'll get the correct alignment. It's the same stuff as in the instruction list in the sidebar... :( @@ +130,3 @@ + <property name="margin_top">15</property> + <property name="xalign">0.7</property> + <property name="label" translatable="yes">Maps need an active internet connection to function properly, but one can't be found. Check your connection and proxy settings.</property> Make a newline after "[...] can't be found.". You can use the escaped XML newline for this "
" This is also to follow the mockup.
Review of attachment 288369 [details] [review]: Just read https://wiki.gnome.org/TranslationProject/DevGuidelines/Avoid%20hard-coded%20line%20breaks and realized I gave some bad reviews just now. Thanks Damián and sorry Shipra. :) Comments below ::: src/main-window.ui @@ +127,3 @@ + <object class="GtkLabel" id="message2"> + <property name="wrap">True</property> + <property name="max-width-chars">47</property> Since text can / should be translated perfectly looking text can only be achieved for one language. Agree with Jonas here, 50 sounds nice. @@ +130,3 @@ + <property name="margin_top">15</property> + <property name="xalign">0.7</property> + <property name="label" translatable="yes">Maps need an active internet connection to function properly, but one can't be found. Check your connection and proxy settings.</property> Hm, so after reading the translation guidelines I think you should just make two labels: 1: Maps need an active internet connection to function properly, but one can't be found. 2: Check your connection and proxy settings.
Review of attachment 288369 [details] [review]: Tested a bit more... I need to get back to work soon. ::: src/main-window.ui @@ +127,3 @@ + <object class="GtkLabel" id="message2"> + <property name="wrap">True</property> + <property name="max-width-chars">47</property> Ok. I just tested with the longest translation of the string I could find and a width of 45 seems to never spill over on a fourth line (which is the only thing that would worry me). So. 45 it is. That's the perfect number. Feels even and looks good. :)
Btw, we also need to hide the sidebar if it is open when this happens (as interacting with it with no network will result in pretty weird behaviour). You should just need to call this._setRevealSidebar(false) inside mainWindow.js. :)
Review of attachment 288369 [details] [review]: ::: src/main-window.ui @@ +129,3 @@ + <property name="max-width-chars">47</property> + <property name="margin_top">15</property> + </object> Mattias is right, in #gnome-hackers someone told me that it's an error in Gtk+, basically you need to set xalign=0 in addition to halign=start in order tu have multiline labels left aligned.
Review of attachment 288368 [details] [review]: ::: src/application.js @@ +90,3 @@ + _checkNetwork: function() { + let addr = new Gio.NetworkAddress({ hostname:'tile.openstreetmap.org', + port:80 }); Oh God! what happened here? :)
(In reply to comment #80) > Btw, we also need to hide the sidebar if it is open when this happens (as > interacting with it with no network will result in pretty weird behaviour). > > You should just need to call this._setRevealSidebar(false) inside > mainWindow.js. :) Nice catch!
(In reply to comment #80) > Btw, we also need to hide the sidebar if it is open when this happens (as > interacting with it with no network will result in pretty weird behaviour). > > You should just need to call this._setRevealSidebar(false) inside > mainWindow.js. :) Should be show it again when Internet is back if it was hidden when Internet was lost?
(In reply to comment #84) > (In reply to comment #80) > > Btw, we also need to hide the sidebar if it is open when this happens (as > > interacting with it with no network will result in pretty weird behaviour). > > > > You should just need to call this._setRevealSidebar(false) inside > > mainWindow.js. :) > > Should be show it again when Internet is back if it was hidden when Internet > was lost? If it doesn't complicate the code too much I'm for!
Can we just set visible true/false?
Review of attachment 288369 [details] [review]: ::: src/main-window.ui @@ +129,3 @@ + <property name="max-width-chars">47</property> + <property name="margin_top">15</property> + <property name="xalign">0.7</property> yup, had this in Boxes.
(In reply to comment #86) > Can we just set visible true/false? Yeah I think binding the connected property to the visible property on the sidebar should be the best. That way it will not be visible, but the reveal-child property will be untouched and it will become visible in the same state as it was.
(In reply to comment #88) > (In reply to comment #86) > > Can we just set visible true/false? > > Yeah I think binding the connected property to the visible property on the > sidebar should be the best. That way it will not be visible, but the > reveal-child property will be untouched and it will become visible in the same > state as it was. Makes sense!
Created attachment 288561 [details] [review] Sorry for being inactive for the past 1 day. I have worked all the UI nits that you people suggested and fixed the bugs. Also , implemented the sidebar thing.
Created attachment 288562 [details] [review] New Function for sidebar
Review of attachment 288562 [details] [review]: Otherwise looks fine! ::: src/mainWindow.js @@ +70,3 @@ this.mapView.gotoUserLocation(false); + this._createSidebar(); I don' think we should hide the assignment of this._sidebar, since it is used further down in _init. How about this._sidebar = this._createSidebar(); And have this._createSidebar return sidebar? @@ +119,3 @@ + _createSidebar: function() { + this._sidebar = new Sidebar.Sidebar(this.mapView); Then this becomes let sidebar =
Created attachment 288563 [details] [review] Maps look terrible in absence of an internet connection. Hence switch to another no network connection view.
Review of attachment 288563 [details] [review]: Looks good to me! Andreas/Mattias: Is the ui fine?
(In reply to comment #94) > Review of attachment 288563 [details] [review]: > > Looks good to me! > > Andreas/Mattias: Is the ui fine? Based on the screenshot in https://bugzilla.gnome.org/attachment.cgi?id=288371 it is!
Created attachment 288575 [details] Based on the UI nits above, I did slight changes to the UI file. The screen shot now looks like this.
(In reply to comment #96) > Created an attachment (id=288575) [details] > Based on the UI nits above, I did slight changes to the UI file. The screen > shot now looks like this. Looks good!
Review of attachment 288563 [details] [review]: After designers ack.
Review of attachment 288563 [details] [review]: With these really small fixes I say merge away! Great work Shipra, you made Maps one step closer to really feel polished! \o/ ::: src/main-window.ui @@ +126,3 @@ + <property name="wrap">True</property> + <property name="max-width-chars">45</property> + <property name="margin_top">15</property> 15 → 10 seems better. @@ +134,3 @@ + <object class="GtkLabel" id="no-network-conn-text-2"> + <property name="wrap">True</property> + <property name="max-width-chars">45</property> And a margin-top of 2 here ::: src/mainWindow.js @@ +119,3 @@ + _createSidebar: function() { + let sidebar = new Sidebar.Sidebar(this.mapView); All references to this._sidebar need to be to just sidebar here (maps crashes otherwise).