Re: [kde-artists] oxygen style and kwin client, svn external

Hi Aaron, others,


> hi everyone ...
>
> a blocker for the git migration is the usage of svn externals. they are a
> broken concept that we need to get away from. there is one in kdebase between
> runtime/kstyles/oxygen/lib and workspace/kwin/clients/oxygen/lib.
>
> in looking at it today, i noticed something odd / interesting in the code
> itself: OxygenAnimation. this can be completely replaced with
> QPropertyAnimation (see attached patch that does this in the kwin client
> code). with Hugo's blessing i'd like to do the same for the oxygen style as
> well.
>
>    
In fact that was the purpose of oxygenanimation to mimic the API of 
QPropertyAnimation and use a qtimeline internally, in order to
- make the transition to QPropertyAnimation easy
- still be able to test with Qt4.5 (to have more feedback, since this 
way you could compile oxygen-trunk against kde4.3).

Now last time I did the switch to QPropertyAnimation, it resulted in 
significantly slower animations (at least on my intel graphics card. 
Reason it seems is that QAnimation tries to push more updates per second 
than QTimeLine. (which kdepepo confirmed in the code). Which is why I 
left it as a QTimeLine.

I'm ok to do the change if there's a concensus.
> there also appears to be something of  a misunderstanding is the usage of
> QPointer in the code. data() is never checked (so it's not actually protecting
> the code against anything!) and QPointer itself results in a global hash being
> populated and checked *whenever a QObject is deleted*. this is made even worse
> since the hash is global to the app it requires aquiring an app-global lock
> every time it is accessed. oy vey! please use QWeakPointer instead of
> QPointer, which avoids this problem. and in the case of code like this where
> the value of data() is never checked, don't bother using a shared pointer at
> all, it just gives the code a false sense of security at the expense of some
> overhead.
>    
Same thing here. I planed to switch to QWeakPointer instead of QPointer, 
which is why I used data() all over the place.
> anyways, if Hugo will so bless the removal of OxygenAnimation, i'll fix this
> in the oxy style as well and then oxygenanimation.h can be removed from the
> oxygen "lib".
>    

> next is to get rid of the svn external altogether, which contains the Helper
> and TileSet classes. both have non-trivial amounts of code in them and it is
> indeed useful to share them. there are two options i can see here:
>
> * create a shared lib in runtime that both the kwin client and qstyle can link
> against. pros: minimal code dupe; cons: ABI would probably need to committed
> to so that running random combinations of kdebase-runtime and kdebase-
> workspace would work together, means a bit more time spent in runtime linking
> (though probably not significant)
>
> * create a static lib in runtime that both kwin client and qstyle can link
> against. pros: the actual source code is shared, no runtime linkage overhead.
> cons: the code is duplicated in both plugins.
>
> * keep a copy of the files (4 of them; 2 headers, 2 imlementations) in both
> runtime and workspace and hand-synchronize them when needed. pros: it's the
> "least works right now" solution, cons: it's the most "require vigilance in
> the future" solution. TileSet doesn't seem to have changed very rapidly, but
> Helper has gone through it's fits and starts
>
> the static lib is, imho, the "least worst" solution here. since we won't have
> to do a "check for the static lib" Find* cmake module (it will be part of
> kdebase runtime!), it should be relatively straight forward.
>
> for 4.4, i think we really need to straighten out the Oxygen::Animation issue
> ASAP. the QPointer usage is really not great, and as that code just duplicates
> Qt's own classes now (though i did notice it has a very nice Kinetic-
> compatible API, huzzah for that! :) it should be surgically removed.
>
> for the svn extern issue, we could wait for 4.5 to open up in trunk, and
> that's probably sensible to avoid that kind of change when not absolutely
> required at this stage of development.
>
> thoughts?
>
>    

QPointer, I'm ready to change. No big deal, and that was the original idea.

OxygenAnimation, its ok too (though I would leave it in lib/ cause I 
added some utility functions (a la "isRunning", "restart" and stuff like 
that, and might benefit a tiny abstraction layer for future changes). 
However, this might trigger complains about style animations performing 
poorly. (to be confirmed. So far I have been the only one to test this. 
By the way: any smart idea on how to actually allow people to test both ?)

I'm confident I can commit both before the branching tomorrow (since 
this was the original plan).

On the shared lib stuff, I'm really no expert ... In my opinion, the 
helper classes would benefit some clean-up before adopting one solution 
or the other, to make whatever we chose more robust and less prone to 
code changes. Would be a good thing for kde4.5 indeed.


>    


______________________________________________________________________________
[email protected] |  https://mail.kde.org/mailman/listinfo/kde-artists