Improvement of the styles handling in Calligra

[Thorsten Zachmann]

On Tuesday, April 17, 2012 03:27:39 Inge Wallin wrote:
> On Monday, April 16, 2012 05:27:46 Thorsten Zachmann wrote:
> > Hello,
> > 
> > On Sunday, April 15, 2012 22:36:13 Inge Wallin wrote:
> > > I wrote some documentation during the weekend at
> > > http://community.kde.org/Calligra/Code/Styles. It is mostly
> > > documentation of how styles are handled in Calligra today but at the
> > > end there is a suggestion for an improved system.
> > > 
> > > I got the thumbs up from boemann for this proposal but before I start
> > > coding I'd like other people to take a look and give feedback.  So
> > > please take a look and say what you think of the idea. It would be
> > > good if we could clean up this very vital part of the engine.
> > 
> > thanks for taking a look into this. It is true that named graphic styles
> > are not kept. That does not mean we loose anything on loading and
> > viewing.
> 
> Yes, that is true. And I also acknowledge that in the description
> 
> > However I think to use a system like we do for text properties also for
> > graphic properties is not a good idea as we already have hightly
> > specialized classes that handle the graphic properties. Those classes use
> > much less memory then what would be needed if we go the way we do for
> > paragraph styles. The classes we have at the moment are also very
> > effective and good to use during runtime.
> > 
> > After some thinging I got the following idea to use some hybrid of both
> > methods. Let me explain a bit
> > 
> > For graphic proerties we have e.g. background (color/gradiert ...),
> > forground (color/gradient ...), linestyle, shadow ...
> > We should keep the classes that store this data and handles it.
> > 
> > However all those different properties of the graphic style can be added
> > to a map as used in the kotext style classes. something like
> > 
> > QHash<int, QVariant> m_stylesProperities // int is a enum of the type
> > QVariant a pointer to e.g. KoShapeBackground, KoShapeBorder ....
> > 
> > To have inheritance each level of a style would have that. and define a
> > parent.
> 
> These are interesting thoughts.
> 
> What I think you mean with the description above is that we could use the
> same mapping as for the text styles but instead of storing only low-level
> data we should store higher-level data like complete background /
> foreground descriptions.  Right?

Right just use the classes we have at the moment.

> If so: yes, I agree. I actually thought of this myself but I forgot to
> describe it. In my suggestion I wrote that we should store binary data but
> I didn't only mean colors and numbers but also higher-level stuff.
> 
> After loading the style as is, I was going to propose a private method
> buildCompositeData (or please, give me a better name :) ) that build for
> instance a KoShapeBackground from all the lower-level properties and
> exchange the lower-level stuff for the higher-level.

I think it does not make sense to load/parse stuff 2 times. The stuff should be 
loaded directly in the classes we have as we do today.

> What came to my mind when I read your text is what to do with styles that
> inherits another style and where description of the higher level concept is
> split between these two styles meaning some attributes is in one class and
> some in the other. Granted, that case would be very rare, but I think we
> need ot handle it.

You mean that the base style would e.g. define a solid background with white 
and the inherited style would change that to e.g. green that the inherited 
style would also have a KoColorBackground with white as color where the 
> 
> > If a inherited style overwrites one property of a style e.g. the color a
> > new KoShapeBackground would be set in the inherited style.
> 
> That could work, yes. It will be somewhat complex to create the style again
> on saving.  Here is a problem:
> 
> Say that we have two styles, A and B. B inherits A.  We have two attributes
> x and y which could be set in A, B or Both.  x and y together create the
> complex value Z.  Now suppose both x and y are set in A, creating a Z
> value for style A.  And x is changed in style B meaning that in our binary
> representaiton we will have another value for Z in B. Now when we want to
> save the styles back to a file, there is no way for us to know if y was
> set in B to the same value as in A or if it was left unspecified and just
> inherited its value from A. This makes a difference if the value of y in A
> is ever changed.

You also don't know what was set if the user sets the same value as in the 
inherited style (This is also true for the current system used in kotext). And 
from my feeling I would say this is also not very important. 
I would say if B the background color is changed then I would say a change in 
A should not modify B at all. in the inherited object the style should only 
change if there was no modification to B at all when A changes.

> > To make handling with styles easier I think each shape should have its
> > one inheritance level even if if does not overwrite any properties in
> > it. This will make modifiying the properties of the shape much easier.
> 
> I don't understand this. Could you elaborate?

If we have a style class with inheritance I think it would make sense to use a 
different object for every shape so that modification of the style of the shape 
is easy e.g. when changing the color.

> 
> > I think separating the storage form handling the data makes no sense as
> > that would mean the runtime would be much worse and also the memory
> > footprint would increase quite a lot which will make it harder to use on
> > embedded devices. This is done in kotext as we use qtextdocument and not
> > our own classes to render the stuff and we had to integrate with this
> > system. However that is not a good reason to do that also for the other
> > styles.
> 
> As far as I understand the reality is exactly the opposite. As it is now
> every shape parses the style stack and creates its own copy of all the
> data that is defined in the style: stroke, fill, shadow, layer, etc.  It's
> a lot of data in the graphic styles so this takes a lot of space.

True but that exactly happens in kotext. I agree for inherited graphic styles 
it makes sense to reuse the same styles and that is possible with what I have 
described. What I meant with using more memory is the sytem you described to 
save the properties. It uses more memory then the current solution and it is 
harder to work with also also slower to work with. The current solution is 
much more freindly on runtime behaviour then what I understand from the 
solution you described with saving the individual values.

> If you look at a document file, it is often the case that many shapes look
> exactly the same: for example they can be black lines with just a few
> different line endings. Or they can be rectangles with the same stroke and
> fill, only differing in size and maybe text on the shape. My guess is that
> if you look at a large set of files, over 50% of the shapes would share
> the same graphic style.

As said it makes sense to share names styles. Not so sure about automatic 
styles. Sharing them between the shapes will require a very big refactor of 
what is currently available and will make the code mode difficult. I'm not 
saying it is something we should not do but to fix the problem you where 
descibing of loosing the named styles it is definitely not needed.
>
> This makes it very wasteful to store the same data over and over again,
> which is what happens now that each style in the stack is reparsed for
> every shap and all the style objects are recreated for every shape. If we
> instead made the styles (or rather the graphic-properties) use implicit
> sharing then it would just store *one* copy of the style data for all the
> shapes that share the same style. And this is true even if the style in
> quesiton is not named but inherits a named style and changes some aspects.

True but as said above it adds a level of compexity. Therefore I think we 
should first try to fix inheritance and then see if the additional step would be 
needed and makes sense.

Thorsten