[Marble-devel] Merging of FloatItems and MarbleGraphicsItems

[Torsten Rahn]

Ok, in addition to the important suggestions in my last mail here are some 
less important thoughts:

Another thing that we need to keep in mind and that I haven't mentioned yet is 
this (however that doesn't affect our current classes much yet):

The KML reference knows a single tag that is not at all subject to the chosen 
geographical projection:

http://code.google.com/intl/de-
DE/apis/kml/documentation/Images/hierarchy_withgx.gif

"ScreenOverlay".

So I suggest that in the future we introduce a 

ScreenOverlayGraphicsItem class which derives from ScreenGraphicsItem.

Basically this class would have an additional setter that allows to

setScreenOverlay( const GeoDataScreenOverlay );

As such it would just display a predefined bitmap (called "icon" in KML) in a 
non-movable fashion.

As I said above this isn't too relevant right now but it might be useful to 
keep in mind. :-)

Regards,

Torsten


Am Montag 20 Juli 2009 03:07:50 schrieb Torsten Rahn:
> Hi,
>
> Am Sonntag 19 Juli 2009 23:06:04 schrieb Bastian Holst:
> > I have used ScreenGraphicsItem (which implements all features for
> > MarbleGraphicsItems that have no geo location) to implement
> > AbstractFloatItem. It would be nice if somebody could review this.
>
> Much of the stuff is very good. I feel however that some of the proposed
> changes need some severe adjustments (easy to implement, but changes the
> current design quite a bit).
>
> > Especially my changes to the API of AbstractFloatItem, ScreenGraphicsItem
> > and MarbleGraphicsItem.
> >
> > Now all MarbleGraphicsItems can define a background, margin, padding etc.
> > This would help for example to implement WeatherItems.
>
> Although this would help for the WeatherItem I think that stuff like the
> background, margin and padding shouldn't be part of the lower-most base
> class. (and indeed the QGraphicsItem class doesn't introduce anything
> similar -- comparable stuff like "margins" gets only introduced a bit
> higher in the class hierarchy).
>
> For many classes inheriting from e.g. GeoGraphicsItem (which inherits from
> MarbleGraphicsItem) stuff like "padding" or "margin" doesn't make sense and
> just adds unneeded API and adds largely to the memory consumption:
>
> E.g. what would be the "backgroundShape" or the "padding" of a
> GeoDataLineStringItem or a GeoDataPointItem?
>
> So my opinion is that these properties need to get moved up in the
> inheritance hierarchy:
>
> On the ScreenGraphicsItem side they need to get moved into
> ScreenGraphicsItem.
>
>
> On the GeoGraphicsItem side it's a bit more tricky:
>
> The difference between a ScreenGraphicsItem and a GeoGraphicsItem is that
> the ScreenGraphicsItem doesn't follow the projection at all and that the
> extent is given in pixels.
> The GeoGraphicsItem on the other hand is an item of which the position is
> expressed in geographics coordinates and it follows the projection
> automatically. The extent/shape of a geographics Item can change and vary
> according to the projection. E.g. for a line string the shape will change
> massively in spherical projection depending on how the globe is rotated.
>
> Now like for your WeatherItem a GeoGraphicsItem can have a "Label" which is
> positioned according to the GeoGraphicsItem's position and which might
> contain images and/or Text.
>
> The same is true for the Placemark(Item) which can have an icon-"Label" and
> a text-"Label" assigned.
>
> The shape of the "Label" is not subject to the distortion that might get
> introduced via the projection. So in that regard it's quite similar to a
> ScreenGraphicsItem. According to our definition of Screen- and
> GeoGraphicsItem it's still a GeoGraphicsItem though.
> So I suggest that we create a GeoLabelGraphicsItem class which inherits
> from GeoGraphicsItem. Inside the GeoLabelGraphicsItem we'd have all the
> padding, backgroundShape, margin properties etc. .
>
> The GeoPlacemarkGraphicsItem class could then use GeoLabelGraphicsItem
> internally to display the icon and the text label.
>
> For the WeatherItem you could do likewise: You let the WeatherItem own a
> GeoLabelGraphicsItem.
>
>
> So the following methods should get IMHO lifted from MarbleGraphicsItem to
>
> * ScreenGraphicsItem and
> * GeoLabelGraphicsItem (which needs to be created)
>
> (and yes, you'd need to duplicated the API/implementation there :-/ ):
>
> +    qreal margin() const;
> +    void setMargin( qreal margin );
> +    qreal marginTop() const;
> +    void setMarginTop( qreal marginTop );
> +    qreal marginBottom() const;
> +    void setMarginBottom( qreal marginBottom );
> +    qreal marginLeft() const;
> +    void setMarginLeft( qreal marginLeft );
> +    qreal marginRight() const;
> +    void setMarginRight( qreal marginRight );
> +    qreal borderWidth() const;
> +    void setBorderWidth( qreal width );
> +    qreal padding() const;
> +    void setPadding( qreal width );
> +    qreal border() const;
> +    void setBorder( qreal width );
> +    QBrush borderBrush() const;
> +    void setBorderBrush( const QBrush &brush );
> +    Qt::PenStyle borderStyle () const;
> +    void setBorderStyle( Qt::PenStyle style );
> +    QBrush background() const;
> +    void setBackground( const QBrush &background );
> +    QRectF contentRect( const QPointF& position = QPointF( 0.0, 0.0 ) )
> +    QRectF paintedRect( const QPointF& position = QPointF( 0.0, 0.0 ) )
> const;
>
> > Until now the brush and color for the background as well as some other
> > things were set globally (if you look at the old code these were static
> > properties, but setter and getter functions were not static).
>
> The reason was that for the FloatItems to have a uniform appearance all
> items would have the same look and feel.
> And another concern would be memory. However both of these issues can much
> better get solved with object properties if the implementation is done in a
> smart way:
>
> Imagine we hold the margins and padding in a single object that is
> referenced by a pointer. Then we can reference the default values to a
> single "default value" object that is shared between all (e.g. 20000)
> labels.
> This way we could ensure the same look and feel shared by all labels and we
> could keep the memory consumption low.
>
> > As it is possible that different items would want different background
> > colors, I changed these to object properties.
>
> So yes I agree that your move from static properties to object properties
> in the API is good. But I think we need to improve the way the properties
> get stored if we deal with lots of objects like this.
>
> > Another option would be to have a default item style class (static)
> > holding the standard values which will be used when nothing else has been
> > set.
>
> Ooops, that sounds roughly like what I was just thinking of :-)))
>
> > If
> > someone sets the background color of the compass to yellow, only the
> > compass will appear in yellow and if now somebody sets the standard
> > background color to red all other items get a red background (compass of
> > course has still a yellow background).
> > What do you think?
>
> Looks great!
> The only issue I see is the styling/layout methods which imho needs to get
> moved upwards to ScreenGraphicsItem and GeoLabelGraphicsItem :-)
>
> Regards,
> Torsten
>
> > Regards
> > Bastian
>
> _______________________________________________
> Marble-devel mailing list
> Marble-devel at kde.org
> https://mail.kde.org/mailman/listinfo/marble-devel