[Marble-devel] Merging of FloatItems and MarbleGraphicsItems

[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