Re: oxygen style and kwin client, svn external

Hi Aaron,
>> 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.
>>      
> frame limiting may make a nice addition to kinetic, but i don't see how the
> animations would be slower in this case.
>
> if it takes 100ms to generate a frame in the animation, then the maximum rate
> will be 10 frames/s with kinetic. it doesn't enforce some frame rate,
> regardless of how long it takes to achieve that. it is time based, just like
> with QTimeLine. :)
>
> what can happen is that it uses more cpu to achieve an animation since it may
> generates more frames than you strictly want (if the animation frame
> generation is quick), but the timespan itself should be no different. if
> anything, the animation should be smoother but more cpu intensive.
>    
well, that's the surprising part. It seems (to me) that the "tab" 
transitions take more time (and is smoother) with QPropertyAnimation 
than with QTimeLine, although I agree with the arguments you list so 
that it should not be the case, and don't quite understand why it is so. 
See e.g. in the systemsettings dialog.
I wonder if this could be related to how various threads share the work.
Anyway: or the other animations, there is not much difference in fact, 
and I agree that using Kinetic is the right way to go (for the reasons 
you mention).
(besides: the tab animation is disabled by default).

> so, what do you mean by "slower", exactly? because perhaps you have some
> specific impact in mind other than what that means to me on first read :)
>
>    
>>> 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.
>>      
> using data() only matters if you actually check the object first. e.g. this:
>
> QWeakPointer<QWidget>  *wp = foo;
> .. later ...
> wp.data()->doSomething();
>
> is jut as dangerous as:
>
> QWidget * p = foo;
> ... later ...
> p->doSomething();
>
> what you need to do is:
>
> QWeakPointer<QWidget>  *wp = foo;
> .. later ...
> if (wp) { wp.data()->doSomething(); }
>
>    
I got that. There are in fact some places where the weakPointers are 
checked (the way you mention), in e.g. 
kstyles/oxygen/animations/oxygenanimationdata.h, and last time I 
checked, they were necessary.
> that wasn't happening anywhere in the code that i looked at and so the usage
> of shared pointers was actually not giving any benefit, just overhead.
> fortunately the "shared" pointers weren't really shared at all in any of the
> window decoration code, so it was trivial to just move to plain c++ pointers
> there. i haven't looked much into the style code, but if the pointers really
> are shared around there, please be certain to check that the pointer is valid
> before using data()
>
>    
>>> thoughts?
>>>        
>> QPointer, I'm ready to change. No big deal, and that was the original idea.
>>      
> great :)
>
>    
>> 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).
>>      
> we've been around this same path in Plasma, it's really not worth it.
> isRunning and restart are trivial and don't really save much to warrant the
> addition of a whole other class and QAbstractAnimation is about as abstract as
> one is going to get for an animation base (even to the point of being too
> generic/simple, as it doesn't provide any actual animations framework, just a
> bunch of convenience classes by which to change values over time; this is
> useful and needed, but also very basic :)
>
>    
>> 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 ?)
>>      
> if OxygenAnimation replicated large parts of the API and had a
> QAbstractAnimation or a QTimeline as an internal member. this makes other
> things really difficult though, such as combining animations. i don't think
> it's worth it.
>
> if we're having issues with Kinetic, let's discuss it with the people working
> on Kinetic (some of them are very much involved with KDE development as well)
> and get improvements there.
>
>    
I agree.
> also note that QTimeLine uses a separate timer per object while Kinetic uses a
> shared timer. in the case of many animations running simultaneously, having a
> shared timer is usually better for coordination, wakeups, etc.
>    
True.
>    
>> I'm confident I can commit both before the branching tomorrow (since
>> this was the original plan).
>>      
> great; i've already got the oxygen style done, so i'll commit that then.
>
>    
It's all committed already. Right now I'm trying to address some of the 
obvious bugs that have been reported.
(notably that Qt apps do not used a different text color for disabled 
widgets, while kde apps do)

Hugo