GNOME Bugzilla – Bug 755847
Remember last location and use it at start up
Last modified: 2015-11-18 18:54:59 UTC
We should remember the lat/lon we where at, and go there on startup. Maybe have a timeout function that writes them to settings periodicly if lat/lon has changed? And then look at that setting at start up. Should be a nice task to get to know the code base for someone.
Sure, I am working on it.
Created attachment 314584 [details] [review] application: Start app with last viewed location When users open Maps they start with map of world. This is less convenient than starting at where they were last time. So through this patch we store their last visited location in gsettings and retrieve it back next time they open. This is done by a timer function which finds value of current view bounding box and save it to gsettings. Timer interval is also taken from Gsettings. Another function is used to retrieve these values at startup and move map view to that location. We have stored bounding box dimensions to string array in Gsettings so as to maintain location precision.
Created attachment 314588 [details] [review] application: Start app with last viewed location When users open Maps they start with map of world. This is less convenient than starting at where they were last time. So through this patch we store their last visited location in gsettings and retrieve it back next time they open. This is done by a timer function which finds value of current view bounding box and save it to gsettings. Timer interval is also taken from Gsettings. Another function is used to retrieve these values at startup and move map view to that location. We have stored bounding box dimensions to string array in Gsettings so as to maintain location precision.
Created attachment 314589 [details] [review] application: Start app with last viewed location When users open Maps they start with map of world. This is less convenient than starting at where they were last time. So through this patch we store their last visited location in gsettings and retrieve it back next time they open. This is done by a timer function which finds value of current view bounding box and save it to gsettings. Timer interval is also taken from Gsettings. Another function is used to retrieve these values at startup and move map view to that location. We have stored bounding box dimensions to string array in Gsettings so as to maintain location precision.
Created attachment 314593 [details] [review] application: Start app with last viewed location When users open Maps they start with map of the world. This is less convenient than starting at where they were last time.So through this patch we store their last visited location in gsettings and retrieve it back next time they open. This is done by a timer function which finds value of current view and save it to gsettings. Timer interval is also taken from Gsettings. Another function is used to retrieve these values at startup and move map view to that location. We have stored bounding box dimensions to string array in Gsettings so as to maintain location precision.
Review of attachment 314593 [details] [review]: Thanks for the patch it looks nice! ::: data/org.gnome.Maps.gschema.xml @@ +5,3 @@ + <summary>last viewed location</summary> + <description>Coordinates of last viewed location.</description> + </key> It should be enough to save just the bounding-box, right? We can use get_center in order to get the lat/lon to goto @@ +10,3 @@ + <summary>last viewed location updat intrval</summary> + <description>Interval(in ms) of last viewed location update.</description> + </key> I do not think we should have the interval as a setting, instead we should choose a sane default one, or use a different heuristic of when to save. ::: src/application.js @@ +198,3 @@ + let millis = settings.get('last-viewed-location-update-interval'); + let id = GLib.timeout_add(GLib.PRIORITY_DEFAULT, millis, this._timer.bind(this)); + }, Is timer the best way? We could listen on notify on latitude for instance? And add a timeout on each latitude change? A timeout we cancel if there is another latitude change before that? sort of like: onLatitudeChange: if (id != 0): source_remove(id) id = timer_add(100, store_new_location) store_new_location: id = 0 store_location_to_settings return false @@ +203,3 @@ + let box = this._mainWindow.mapView.get_view().get_bounding_box(); + let [lat,lon] = box.get_center(); + let lastViewedLocation = [""+box.top, ""+box.bottom, ""+box.left, ""+box.right, ""+lat, ""+lon]; Is this really needed? Why do we need to save the coords as string? @@ +208,3 @@ + }, + + _goToStartingPlace: function() { Use a champlainboundingbox and get_center and we only need to store the bbox it self: https://developer.gnome.org/libchamplain/unstable/ChamplainBoundingBox.html#champlain-bounding-box-get-center @@ +218,3 @@ + right : box[3] }), + }); + new MapWalker.MapWalker(place, this._mainWindow.mapView).goTo(true); I am not sure we want animation on this, I want goTo(false) I think. @@ +271,2 @@ }, I do not think this code belongs in application.js, I want it in mapView.js instead. And we should make sure that the view is realized before doing the goTo(false) see how the openGeoJSON method does it!
yes, you are right we can just save the bounding box and get lat from it later. Mapview emits a 'view-moved' event we can use that. And okay I tried using double in Gsettings array and it worked, so great. I don't get it why we need to have a timeout for latest lat change..? Do you mean to say that if location remains same for that period of time then save it. goTo(animate), when animate is false, it only sets the center, just that and returns. It does not set Zoom level, so should I make changes to it ? I hope it wont break anything. And where are openGeoJSON methods ?
Hi, please use the "review" link when replying so history of the conversation is conserved! (In reply to Alaf from comment #7) > yes, you are right we can just save the bounding box and get lat from it > later. > > Mapview emits a 'view-moved' event we can use that. And okay I tried using > double in Gsettings array and it worked, so great. Great! > > I don't get it why we need to have a timeout for latest lat change..? > Do you mean to say that if location remains same for that period of time > then save it. No I mean that if we are panning / tilting the view will move a lot and we would store to settings a lot. If we instead schedule a timeout or we could even use an Mainloop.idle_add() that might be better. > > goTo(animate), when animate is false, it only sets the center, just that and > returns. It does not set Zoom level, so should I make changes to it ? I hope > it wont break anything. > Ah, ok. Hmm. Yeah we might want to change that. But that change needs some thought so we should wait. So I think just keep animate true for now! Also, please check your changes with git diff --check, there were some whitespace damage in your previous patch. > And where are openGeoJSON methods ? in src/mapView.js Thanks!
Created attachment 314750 [details] [review] application: Start app with last viewed location When users open Maps they start with map of the world. This is less convenient than starting at where they were last time. So through this patch we store their last visited location in gsettings and retrieve it back next time they open. Whenever there is a view-moved event we schedule a timeout to save the location. When a new timeout is sceduled, it cancels the previous one. This ensures that we are not saving locations too many times. Another function is used to retrieve these values at startup and move map view to that location.
Review of attachment 314750 [details] [review]: Thanks! Very nice commit message, I have some notes on the implementation but it looks really nice! ::: src/mapView.js @@ +44,3 @@ const Utils = imports.utils; +let timer_id=null; Have this as a member variable of the MapView class instead. Maybe this._storeLocationId or something like that? @@ +45,3 @@ +let timer_id=null; + Be careful not to add extra new lines @@ +103,3 @@ + + this.connect('view-moved', (function() { + this._init_timer(); I do not think we need any timer at all. And actually I do not think we need to connect to this event at all. We could perform the code in the _onViewMoved method that already exist in this file! So in that method you could do: if (this._storeLocationId) return; and then: this._storeLocationId = Mainloop.idle_add(this._storeLocation.bind(this)); Sorry about making my pseudo code unclear in the last review. There should be no need for a timer, just an idle. @@ +297,3 @@ + bottom : box[1], + left : box[2], + right : box[3] }); Make sure parameters like these are aligned like in other places in the code. @@ +300,3 @@ + if (this.view.realized){ + this._gotoBBox(bounding_box); + }else{ Make sure to have spaces around the curly braces. @@ +302,3 @@ + }else{ + this.view.connect('notify::realized', + this._gotoBBox.bind(this, bounding_box)); And again sorry for changing my mind. Since this will be done from the init method of it should always be done on realized. So I thin kyou want to add the following code to the _initView method: this.view.connect(this._gotoStoredLocation.bind(this)); And implement the going to the stored location in gotoStoredLocation.
Review of attachment 314750 [details] [review]: Actually you did mentioned that we need a MainLoop.idle() instead of timer, but I did some tests how many times 'storing location' function was being called. With using only MainLoop.idle() on 'view-moved' event, it saves about 4 times on just a double click zoom, and about ~200 times in the starting animation and any other similar animation. (less optimum .?!) That is the reason I used a timer with mainloop.idle(). Now using both of these together, no extra saves are done, only the last one. It is what we want right.? And yes I was also thinking of putting it(init_timer) directly in view moved function, but would it look good..? Okay sure, I will connect _goToStoredLocation() directly to initView, on realized.
Review of attachment 314750 [details] [review]: Ok, agreed, use a timer. Make a constant for the timeout, have it at 500? I commited a change to master to only do onViewMoved on latitude change, having it on both is insane :) Some more comments below. ::: src/mapView.js @@ +273,3 @@ }, + _init_timer: function() { + if(!timer_id==0) use spaces after if and around equals, this would be: if (storeId !==) 0) ... read http://www.sitepoint.com/javascript-truthy-falsy/ @@ +276,3 @@ + GLib.source_remove(timer_id); + + timer_id = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 400, (function(){ Please use Mainloop.timeout_add(_LOCATION_STORE_TIMEOUT, (function() { ... } @@ +279,3 @@ + timer_id=0; + Mainloop.idle_add((function() { + this._save_location(); Skip the idle_add @@ +285,3 @@ + }, + + _save_location: function() { Please use camelcase, _storeLocation
Created attachment 314938 [details] [review] application: Start app with last viewed location When users open Maps they start with map of the world. This is less convenient than starting at where they were last time. So through this patch we store their last visited location in gsettings and retrieve it back next time they open. Whenever there is a view-moved event we schedule a timeout to save the location. When a new timeout is sceduled, it cancels the previous one. This ensures that we are not saving locations too many times. Another function is used to retrieve these values at startup and move map view to that location.
Review of attachment 314938 [details] [review]: Thanks! Just some more comments other than that this looks really nice! ::: src/mapView.js @@ +276,3 @@ + }, + + _goToStartingPlace: function() { I think I would prefer _goToStoredLocation for name here @@ +405,3 @@ this.emit('view-moved'); + if (this._storeId !== 0) + Mainloop.source_remove(this._storeId); Ok, so this is not what I was thinking at all :) I mean that if there already is a request out to handle this move we should not do anything, so this could be if (this._storeId !== 0) return;
Created attachment 315006 [details] [review] application: Start app with last viewed location When users open Maps they start with map of the world. This is less convenient than starting at where they were last time. So through this patch we store their last visited location in gsettings and retrieve it back next time they open. Whenever there is a view-moved event we schedule a timeout to save the current location. When a new timeout is sceduled, we check if we have a pending timeout, if we do, we dont add the new one. This ensures that we are not saving locations too many times. Another function is used to retrieve these values at startup and move map view to that location.
Review of attachment 315006 [details] [review]: Is this done or do I need to do anything else too?
Review of attachment 315006 [details] [review]: Thanks! Just some small nits, but I can fix them up when commiting! Sorry for losing track of this! Nice work! ::: src/mapView.js @@ +276,3 @@ + }, + + _goToStoredLocation: function() { We really should decide if we want to say gotoBlaBla or goToBlabla, I am in favour of gotoBlaBla. Right now we mix :) @@ +281,3 @@ + bottom : box[1], + left : box[2], + right : box[3] }); We usually do not do this type of aligment of parameters.