[libkwineffects] Add TimeLine helper
ClosedPublic

Authored by zzag on Jun 26 2018, 2:24 PM.

Details

Summary

Most effects use QTimeLine in the following manner

if (...) {
    m_timeline->setCurrentTime(m_timeline->currentTime() + time);
} else {
    m_timeline->setCurrentTime(m_timeline->currentTime() - time);
}

Because effects do not rely on a timer that QTimeLine has, they can't
toggle direction of the QTimeLine, which makes somewhat harder to write
effects. In some cases that's obvious what condition to use to figure
out whether to add or subtract time, but there are cases when it's
not. In addition to that, setCurrentTime allows to have negative
currentTime, which in some cases causes bugs.

And overall, the way effects use QTimeLine is really hack-ish. It makes
more sense just to use an integer accumulator(like the Fall Apart
effect is doing) than to use QTimeLine.

Another problem with QTimeLine is that it's a QObject and some effects
do

class WindowInfo
{
public:
    ~WindowInfo();

    QTimeLine *timeLine;
};

WindowInfo::~WindowInfo()
{
    delete timeLine;
}

// ...

QHash<EffectWindow*, WindowInfo> m_windows;

which is unsafe.

This change adds the TimeLine class. The TimeLine class is a timeline
helper that designed specifically for needs of effects.

Demo

TimeLine timeLine(1000, TimeLine::Forward);
timeLine.setEasingCurve(QEasingCurve::Linear);

timeLine.value(); // 0.0
timeLine.running(); // false
timeLine.done(); // false

timeLine.update(420);
timeLine.value(); // 0.42
timeLine.running(); // true
timeLine.done(); // false

timeLine.toggleDirection();
timeLine.value(); // 0.42
timeLine.running(); // true
timeLine.done(); // false

timeLine.update(100);
timeLine.value(); // 0.32
timeLine.running(); // true
timeLine.done(); // false

timeLine.update(1000);
timeLine.value(); // 0.0
timeLine.running(); // false
timeLine.done(); // true
Test Plan

Ran tests.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zzag created this revision.Jun 26 2018, 2:24 PM
Restricted Application added a project: KWin. · View Herald TranscriptJun 26 2018, 2:24 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jun 26 2018, 2:24 PM

which is unsafe.

What part are you refering to in the code you posted? The WindowInfo object getting copied?

zzag added a comment.Jun 26 2018, 3:12 PM

which is unsafe.

What part are you refering to in the code you posted?

Take a look at Glide or Sheet effect. ;-)

The WindowInfo object getting copied?

Yep. IIRC, value type for QHash have to copyable. If a value is copied somewhere else and then destroyed, nothing good will happen.

Also, that's really easy to forget &.

In general all your arguments make total sense ++

Either we should be using a QTimeLine using the internal timeline and using the signal to trigger repaints, or not using it at all.
No point working round the one thing QTimeLine provides.

I'm not sure I like the way m_done works, there's no neat way to have the timeline object existing but not running (i.e where done=false).
It's fine when it's wrapped in some WindowData struct where the lifespan matches the animation, but would be difficult to port the zoom effect which has a single instance for the duration of the effect.

zzag added a comment.Jun 26 2018, 4:30 PM

Either we should be using a QTimeLine using the internal timeline and using the signal to trigger repaints, or not using it at all.
No point working round the one thing QTimeLine provides.

I personally would prefer to use delta times provided by KWin. That way, effects don't need to deal with refresh rates, etc.

I'm not sure I like the way m_done works, there's no neat way to have the timeline object existing but not running (i.e where done=false).
It's fine when it's wrapped in some WindowData struct where the lifespan matches the animation, but would be difficult to port the zoom effect which has a single instance for the duration of the effect.

Overall, start method doesn't make sense because of the way the timeline is updated. Yet, progress values of 0 and 1 have special meaning. So, we can use them to check current state of the timeline.

I've been thinking about adding running method

bool TimeLine::running() const
{
    return m_elapsed != 0 && m_elapsed != m_duration;
}

And example usage

void MinimizeAnimationEffect::windowMiminized(EffectWindow *w)
{
    TimeLine &timeLine = m_animations[w];
    
    if (timeLine.running()) {
        timeLine.toggleDirection();
        return;
    }

    timeLine.setDuration(animationTime(250));
    timeLine.setDirection(TimeLine::Forward);
}

void MinimizeAnimationEffect::windowUnminimized(EffectWindow *w)
{
    TimeLine &timeLine = m_animations[w];

    if (timeLine.running()) {
        timeLine.toggleDirection();
        return;
    }

    timeLine.setDuration(animationTime(250));
    timeLine.setDirection(TimeLine::Backward);
}

Still, that's not perfect but I don't see any other sane way because of the way timeine is updated. FWIW, most effects do similar things (e.g. Desktop Grid, Dim Screen, etc).

You have done() why not use it, looks like running

zzag added a comment.Jun 26 2018, 4:56 PM

You have done() why not use it, looks like running

In some cases (like in the example above), it would be conceptually wrong.

+1 from my side. I don't think we would have used it if the QEasingCurve would have existed at that time.

libkwineffects/kwineffects.h
3315

As it's new API: maybe use std::chrono::msec instead of int.

What's the "rvalue" function good for? Can't it be removed?

libkwineffects/kwineffects.h
3329

Name it differently, could be confused with l- and rvalues.

zzag added a comment.Jun 26 2018, 6:09 PM

What's the "rvalue" function good for? Can't it be removed?

I'll delete it. That method is good for calculating 1.0 - value of easing curve at some point for curves like QEasingCurve::InOutElastic (in general, for all InOut curves).

"r" prefix was inspired by rbegin/rend.

zzag added inline comments.Jun 26 2018, 9:14 PM
libkwineffects/kwineffects.h
3315

Not sure if it is worth it right now. That would be the only place where std::chrono::milliseconds is used. Maybe, it would be better to switch to std::chrono::milliseconds in the next major release(KWin 6)? So animationTime, prePaintScreen, and prePaintWindow use it too.

zzag updated this revision to Diff 36720.Jun 26 2018, 9:15 PM
  • Drop rvalue() method
  • Add running() method
zzag edited the summary of this revision. (Show Details)Jun 26 2018, 9:23 PM
davidedmundson accepted this revision.Jun 26 2018, 11:33 PM

May as well port some usages before merging it.

This revision is now accepted and ready to land.Jun 26 2018, 11:33 PM
graesslin requested changes to this revision.Jun 27 2018, 4:25 AM
graesslin added inline comments.
libkwineffects/kwineffects.h
3473

Please use a d-ptr

This revision now requires changes to proceed.Jun 27 2018, 4:25 AM
graesslin added inline comments.Jun 27 2018, 4:31 AM
libkwineffects/kwineffects.h
3315

We don't need to wait for KWin 6 for this. If we want to use chrono, we can integrate and in worst case have to change the ABI. Personally I think it is worth it as it gives way more semantics to the API. If we start with new code it will be easier to migrate the code in future.

From having done two large ports in KWin: I would start with such a small class.

So for long time perspective I would say it's worth to start now and it's not a problem to mix it. Just like QTimer supports chrono now.

zzag updated this revision to Diff 36744.Jun 27 2018, 8:37 AM
  • Use d ptr
  • Switch to std::chrono::milliseconds
zzag marked 4 inline comments as done.Jun 27 2018, 8:39 AM
graesslin accepted this revision.Jun 27 2018, 7:10 PM
graesslin added inline comments.
libkwineffects/kwineffects.cpp
1951

TimeLine::~TimeLine() = default;

This revision is now accepted and ready to land.Jun 27 2018, 7:10 PM
zzag updated this revision to Diff 36788.EditedJun 27 2018, 8:29 PM

= default

zzag marked an inline comment as done.Jun 27 2018, 8:30 PM
graesslin accepted this revision.Jun 28 2018, 4:04 PM
This revision was automatically updated to reflect the committed changes.