GNOME.org
 

Re: Markers and Popovers

Hello guys!

I will respond you both Mattias and Jonas, below.

2014-05-22 8:09 GMT-03:00 Jonas Danielsson <the sator gmail com>:

On Thu, May 22, 2014 at 01:18:43AM +0200, Mattias Bengtsson wrote:

Great work Damián!



Yeah, good work. I will add some comments below.



Thank you!


Moving this to the maps-list since I think it deserves to be public. If
you're not all subscribed yet, do it now please! :)

(Also please forward your previous replies and keep further discussion
on the list! :))

On ons, 2014-05-21 at 13:29 -0300, Damián Nohales wrote:

Hi guys,

I was racking my brain to get a class design for markers and popovers
in Maps. My idea is that the markers and popovers were versatile so
anyone can have the appearance that anyone wants, and that adding new
kind of markers could be neat and well separated from the other
components.


That sounds good!


I think one thing missed here is where in the code and how render the
marker, there is a note in the top left of the diagram that explain
the problem and some differences in the current markers. Also is
missed some connection between the markers/popovers with the rest of
the UI components.

Please take a look on this, any suggestion will be appreciated.




I do not see it as much of a problem. The renderer will add a marker
to the content of the tile along with some kind of callback on button-press
or notify::selected or whichever signal is best. And in the callback we will
have access to the marker. What would be the real difference between a marker
that is accessed in that way and one that is created on a markerlayer?

And how would it affect the positioning algorithm? We will still have access
to lat/lon x/y.



Regarding the UML:
It was a while ago that I did any class diagram stuff (basically just
for a course some 7-8 years ago).
But according to a reference I quickly read now[1]:
 + means public
 - means private
 # means protected

It would be great if you could state if the properties are part of the
API or not by adding -, + or # in front.
Also some of the methods looks like they might be protected and not
private (like createPopover()). Also please add _ before the protected
and private methods (just so I can understand what you mean exactly :))


Yeah, haha, I'm very messy while crafting UMLs, mainly because I
usually use it as a draft to get an idea. But basically, the private
methods should be protected and _ prefixed, I'll change it in a
moment.



Some questions of the API:
1) why might a MapMarker have many markers? Is this actually when (for
example) userMarker has several actors? I think this could be solved by
just special casing userMarker and let it have an
AccuracyCircleActorSomething in it that is shown when the popover is
shown. No need to make this part of the API by having a markers field I
think (if it is, I didn't understand that really, see above).



I think I agree with this


2) If my thoughts are correct in §1, why not just let MapMarker inherit
from Champlain.Marker? Then you'd get set/get -reactive etc for free.



And also this. Inheriting from Champlain.Marker or Champlain.CustomMarker
feels like the way to go.


Ok, my first thought was to inherit from Champlain.Marker too, until
the accuracy actor messed up my mind. You are right, I don't see any
problems if the UserLocatinMarker class has another actor by
composition, that is showed when the main instance is being showed.
This also deprecates the "reactiveMarker" concept.




3) regarding the current userLocation and mapLocation my guess is that
we would make them userMarker and mapMarker and then make a
searchResultMarker that inherits from mapMarker right?



Yup.


4) What is the positioning algorithm?



Yeah, I have the same question. We should always have access to the lat and lon
right? So positioning should just be a matter of getting the x and y through
the ChamplainView and creating a rect?


The thing is, mostly all markers are positioned using the
ChamplainMarker::set_location method, then I saw the Rishi POIRenderer
class and he use raw positions (using ClutterActor::set_position
method), relative to tile position.

See https://github.com/rishirajsinghjhelumi/GNOME-Maps/blob/poi-testing/src/poiRenderer.js

Maybe this case, POIRenderer should set position by hand or using a
callback to set the marker position (by default this callback calls
set_location), I called this callback positioningAlgorithm, an
optional construction parameter. How to deal with this?




5) I'm not sure entirely how the overlay should come to the popover,
maybe add it back to the mapView and have it there as a property? Really
not sure.



We could also go with making mapView inherit from GtkOverlay.


Nitpicks:
1) I think you can drop the Generic in front of Poi{Marker, Popover}
2) MapMarkerPopover → MapPopover
3) Maybe replace all Popover-instances with Bubble instead, since then
we know that we're talking about the bubbles in the map and not the
popovers in the headerbar etc.


Ok, I'm going to fix that.



4)
What is the point of the createUI function of the MarkerPopover?


_init set the place property and calls to createUi, this avoid to
forgot the parent _init method and you should only implements
createUi. It's just the same thing, we can create the UI components in
_init, after call this.parent(params) and do it without createUi.
Hmmm... now that I think of it, implements _init in the child classes
allows to customize the parent constructor parameters. So, I'm going
to remove createUi.




I think after this I'm going to start some test code based on the
Rishi branch, so POI can be taken into account during coding.


One possibility would be to instead take a stab at the
userLocation{marker, bubble}, it is directly related to the check-in
stuff in your project after all.


Isn't related to Poi markers? a user usually do the check-in to a bar,
restaurant, hotel, etc. Anyway, the "dialogs" part of the check-in
(account selection, place selection, bla bla), can be developed
separated from markers and bubbles stuff.



Regards,
Mattias



/Jonas



1:
http://www.ibm.com/developerworks/rational/library/content/RationalEdge/sep04/bell/



Regards.
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]