[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: koffice
From:       Jan Hambrecht <jaham () gmx ! net>
Date:       2008-06-12 9:00:05
Message-ID: 4850E595.9020002 () gmx ! net
[Download RAW message or body]

Thomas Zander wrote:
> On Thursday 12. June 2008 09:07:28 Jan Hambrecht wrote:
>> In the end i am confident that everything works like before and that
>> there are no regressions. And as i understand it, it is important for
>> KPresenter which needs this for page backgrounds.
> 
> I read the patch this morning, it looks to me that you split QBrush into 3 

Correct.

> classes which have virtual methods for setting and getting data. I'm not sure

Not correct. The virtual function are paint, hasTranparency and 
loadStyle and fillStyle. The data setting functions are not virtual.

> what new features this gives, I'm probably just missing it but all the same I 
> would appreciate some API docs for the new classes ;)

Sure, there are some already, i will add more detailed API docs.

> 
> I noted some things while reading the diff;
> * for refcounting you should consider using QAtomicInt (new in Qt4.4)

Sure. I just used the same refcounting as done in KoShapeBorderModel 
introduced by yourself iirc.

> * you use QGradient (which is a value based class, and implicitly shared) in 
> combination with 'new' and 'delete'. This will give you confusing API and 
> cause for bugs.

Yes because there are QLinearGradient, QRadialGradient and 
QConicalGradient. How would i save these to a single member variable if 
not by using a pointer to the base class?

> * We left QBrush in KoShape until now since we need a design to make shapes 
> use pigment, your solution uses QGradient and thus is not compatible with 
> pigment AFAIK :(

Thats no regression as QBrush uses QGradient too. We are free to port to 
pigment in the future now, which was not possible with QBrush before.

> * KoGradientBackground::cloneGradient is private, it should not be in the 
> header file but it should be a method on the d-pointer object.

Agreed.

To be clear, the refactoring does provide compatibilty with the QBrush 
solution used before, except using different classes for different types 
of background which is more clear imho.

Ciao Jan

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]