GNOME Bugzilla – Bug 722263
Set saner goto animation duration
Last modified: 2014-01-22 21:02:37 UTC
Champlain calculates the goto_animation_duration using zoom_level. It also treats ensure_visible as a go_to with its own duration time. In Maps, each goTo is a combination of a call to Champlain's ensure_visible and go_to. I want to set some saner value for the goto animation duration. And I think we should use the distance between locations rather than the zoom-level.
Created attachment 266355 [details] [review] mapLocation: set saner goto animation duration Champlain calculates the goto_animation_duration using zoom_level. It also treats ensure_visible as a go_to with its own duration time. In Maps, each goTo is a combination of a call to Champlain's ensure_visible and go_to. This patch calculates the goto animation duration using the distance between the two locations. It also divides the calculated value with two to get the duration for our whole trip that includes a call to ensure_visible.
Review of attachment 266355 [details] [review]: Apart from that, I agree with this logic and if you have tested that it improves the animations on different zoom-levels and distances between from & to, all is good. ::: src/mapLocation.js @@ +139,3 @@ + }, + + _calculateGoToDuration: function(fromLocation) { I think this function should receive toLocation and be called before new location is set. Since its stateful anyway, I'd rather this function sets the new duration itself: _updateGotoDuration(toLocation)
(In reply to comment #2) > Review of attachment 266355 [details] [review]: > > Apart from that, I agree with this logic and if you have tested that it > improves the animations on different zoom-levels and distances between from & > to, all is good. > > ::: src/mapLocation.js > @@ +139,3 @@ > + }, > + > + _calculateGoToDuration: function(fromLocation) { > > I think this function should receive toLocation and be called before new > location is set. Since its stateful anyway, I'd rather this function sets the > new duration itself: _updateGotoDuration(toLocation) What do you mean by 'called before new location is set' ? I think we want to call it in the goTo method. Since one could potentially create a bunch of mapLocations before going to one of them.
Created attachment 266455 [details] [review] mapLocation: set saner goto animation duration Champlain calculates the goto_animation_duration using zoom_level. It also treats ensure_visible as a go_to with its own duration time. In Maps, each goTo is a combination of a call to Champlain's ensure_visible and go_to. This patch calculates the goto animation duration using the distance between the two locations. It also divides the calculated value with two to get the duration for our whole trip that includes a call to ensure_visible.
(In reply to comment #3) > (In reply to comment #2) > > Review of attachment 266355 [details] [review] [details]: > > > > Apart from that, I agree with this logic and if you have tested that it > > improves the animations on different zoom-levels and distances between from & > > to, all is good. > > > > ::: src/mapLocation.js > > @@ +139,3 @@ > > + }, > > + > > + _calculateGoToDuration: function(fromLocation) { > > > > I think this function should receive toLocation and be called before new > > location is set. Since its stateful anyway, I'd rather this function sets the > > new duration itself: _updateGotoDuration(toLocation) > > What do you mean by 'called before new location is set' ? I think we want to > call it in the goTo method. Since one could potentially create a bunch of > mapLocations before going to one of them. Ah, I misunderstood the calling code. NM
Review of attachment 266455 [details] [review]: ::: src/mapLocation.js @@ +45,3 @@ this._view = mapView.view; + + this._currentLocation = new Geocode.Location({ I'd call it oldLocation to differentiate it with the location params we have (this._latitude etc).
Created attachment 266459 [details] [review] mapLocation: set saner goto animation duration Champlain calculates the goto_animation_duration using zoom_level. It also treats ensure_visible as a go_to with its own duration time. In Maps, each goTo is a combination of a call to Champlain's ensure_visible and go_to. This patch calculates the goto animation duration using the distance between the two locations. It also divides the calculated value with two to get the duration for our whole trip that includes a call to ensure_visible.
Review of attachment 266459 [details] [review]: Setting the old/current location from _init is problematic since the mapView location can change before goTo. Also, I talked to Mattias and he wants to move the goTo from mapLocation to mapView, in later patches. So maybe it can stay this way for now, since the goTo code will move soon.
(In reply to comment #8) > Review of attachment 266459 [details] [review]: > > Setting the old/current location from _init is problematic since the mapView > location can change before goTo. I don't quite get it. Could you provide a bit more explanation? > Also, I talked to Mattias and he wants to move the goTo from mapLocation to > mapView, in later patches. Did he give any reasons for that? The goto belongs here as per OO design IMO. >So maybe it can stay this way for now, I don't think that we should refrain from applying a ready patch because the code *might* change soon?
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 266459 [details] [review] [details]: > > > > Setting the old/current location from _init is problematic since the mapView > > location can change before goTo. > > I don't quite get it. Could you provide a bit more explanation? If we set a variable as old/current in the init that is taken from the ChamplainView (this._view.get_center_latitude ...) as now and then we scroll around that value becomes stale. If we call mapLocation.goTo later then it will using that stale current value as fromLocation. > > > Also, I talked to Mattias and he wants to move the goTo from mapLocation to > > mapView, in later patches. > > Did he give any reasons for that? The goto belongs here as per OO design IMO. > I'll let Mattias answer that. > >So maybe it can stay this way for now, > > I don't think that we should refrain from applying a ready patch because the > code *might* change soon? You are right, of course.
(In reply to comment #9) > > Also, I talked to Mattias and he wants to move the goTo from mapLocation to > > mapView, in later patches. > > Did he give any reasons for that? The goto belongs here as per OO design IMO. Dumb data bearers is also a common OO idiom (think POJO's/POCO's in Java/C# for example). Champlain does the same here btw, the goto function is defined on the mapView and the Location and Marker objects are small data bearers that are acted /upon/ rather than does big stuff themselves. Also having these small location objects have references to the mapView makes it easy to write spaghetti code IMO. I ran in to this when working on the routing stuff. The route service needed a reference to the mapView if I wanted it to return mapLocation's and that's an indication to me that we're doing something wrong. That's my two cents at least. :)
(In reply to comment #11) > (In reply to comment #9) > > > Also, I talked to Mattias and he wants to move the goTo from mapLocation to > > > mapView, in later patches. > > > > Did he give any reasons for that? The goto belongs here as per OO design IMO. > > Dumb data bearers is also a common OO idiom (think POJO's/POCO's in Java/C# for > example). > Champlain does the same here btw, the goto function is defined on the > mapView and the Location and Marker objects are small data bearers that are > acted /upon/ rather than does big stuff themselves. Thats very different. The marker objects are nothing more than markers and they do draw themselves, which is what they are mostly meant to do. Also, Champlain doing something doesn't mean its good practice. :) The less code there is in each object, the better the code in general. > Also having these small location objects have references to the mapView makes > it easy to write spaghetti code IMO. I ran in to this when working on the > routing stuff. The route service needed a reference to the mapView if I wanted > it to return mapLocation's and that's an indication to me that we're doing > something wrong. Perhaps then it was wrong for it to return mapLocation objects?
Zeeshan: are you ok with merging this patch? I think the discussion of where goto should be defined can wait.
It's an ACK from me at least. :)
(In reply to comment #13) > Zeeshan: are you ok with merging this patch? I think the discussion of where > goto should be defined can wait. I already explained in previous comments why its not a good idea so given that you didn't agrue anymore, I think the conclusion should be that we are not doing it. If you want to define another class to move the goto (or maybe other map operatiosn to) into, I'm all ears but main point of these classes was not to have too much code in Mapview.
I just want to move this discussion to the future since it's orthogonal to this issue. :-) we'll discuss that later. How about the patch at hand?
(In reply to comment #16) > I just want to move this discussion to the future since it's orthogonal to this > issue. :-) we'll discuss that later. How about the patch at hand? I never said anything about blocking this patch, Jonas did in comment#8. :)
Created attachment 266992 [details] [review] mapLocation: set saner goto animation duration Champlain calculates the goto_animation_duration using zoom_level. It also treats ensure_visible as a go_to with its own duration time. In Maps, each goTo is a combination of a call to Champlain's ensure_visible and go_to. This patch calculates the goto animation duration using the distance between the two locations. It also divides the calculated value with two to get the duration for our whole trip that includes a call to ensure_visible.
(In reply to comment #17) > (In reply to comment #16) > > I just want to move this discussion to the future since it's orthogonal to this > > issue. :-) we'll discuss that later. How about the patch at hand? > > I never said anything about blocking this patch, Jonas did in comment#8. :) Yes, I am the villain in this story, no doubt. See patch above for commit considerations. It does away with _currentLocation and uses the more, for the goTo function, appropriate name fromLocation instead. Sorry for the mess of a bug this turned into.
Review of attachment 266992 [details] [review]: Good call on removing _getCurrentLocation. ACK from me. ::: src/mapLocation.js @@ +79,3 @@ + }); + this._updateGoToDuration(fromLocation); + Ah nice!
(In reply to comment #17) > (In reply to comment #16) > > I just want to move this discussion to the future since it's orthogonal to this > > issue. :-) we'll discuss that later. How about the patch at hand? > > I never said anything about blocking this patch, Jonas did in comment#8. :) You didn't ACK it either, that's why I asked. But let's just get this pushed now shall we? :)
Review of attachment 266992 [details] [review]: ::: src/mapLocation.js @@ +158,3 @@ + + // We divide with two because Champlain treats both go_to and + // ensure_visible as 'goto' journeys with its own duration. * minor grammatical mistake: divide by two * still doens't say why 'divide by two' (as opposed to number X or Y). * If I'm reading the code correctly, we are actually setting duration for ensure_visible() here rather than goto so the name of the function is misleading, especially given this comment here.
Review of attachment 266992 [details] [review]: ::: src/mapLocation.js @@ +158,3 @@ + + // We divide with two because Champlain treats both go_to and + // ensure_visible as 'goto' journeys with its own duration. Well, it is setting the duration for both goTo and ensure_visible. goTo gets called when the anomation of the ensure_visible is done. Both ensure_visible and go_to ends up in the same private go_to_with_duration in champlain and both use the same duration property. So the divide by two is for ensuring we get the duration, all tho it is split up in half for ensure_visible and half for go_to. That is more clear in the commit message. Maybe it should be more clear in the comment as well. I do not think the name is missleading, since our duration for our goto is what is calculated.
Review of attachment 266992 [details] [review]: ack ::: src/mapLocation.js @@ +158,3 @@ + + // We divide with two because Champlain treats both go_to and + // ensure_visible as 'goto' journeys with its own duration. ah ok, thanks for explaining. I'm a bit slow sometimes. :)
Attachment 266992 [details] pushed as 54ec97b - mapLocation: set saner goto animation duration