Differential D15175 Diff 41607 effects/cube/cubeslide.cpp

Changeset View

Standalone View

View Options

effects/cube/cubeslide.cpp

Context not available.
33{ 33​{
34 34​
35CubeSlideEffect::CubeSlideEffect() 35​CubeSlideEffect::CubeSlideEffect()
36 : windowMoving(false) 36​ : stickyPainting(false)
37​ , windowMoving(false)
37 , desktopChangedWhileMoving(false) 38​ , desktopChangedWhileMoving(false)
38 , progressRestriction(0.0f) 39​ , progressRestriction(0.0f)
39{ 40​{
40 initConfig<CubeSlideConfig>(); 41​ initConfig<CubeSlideConfig>();
41 connect(effects, SIGNAL(desktopChanged(int,int)), this, SLOT(slotDesktopChanged(int,int))); 42​ connect(effects, &EffectsHandler::windowAdded,
42 connect(effects, SIGNAL(windowStepUserMovedResized(KWin::EffectWindow*,QRect)), this, SLOT(slotWindowStepUserMovedResized(KWin::EffectWindow*))); 43​ this, &CubeSlideEffect::slotWindowAdded);
zzagUnsubmitted
Done

Please move it up, otherwise compiler won't be happy about reordering.

zzag: Please move it up, otherwise compiler won't be happy about reordering.
43 connect(effects, SIGNAL(windowFinishUserMovedResized(KWin::EffectWindow*)), this, SLOT(slotWindowFinishUserMovedResized(KWin::EffectWindow*))); 44​ connect(effects, &EffectsHandler::windowDeleted,
45​ this, &CubeSlideEffect::slotWindowDeleted);
46​ connect(effects, QOverload<int,int,EffectWindow *>::of(&EffectsHandler::desktopChanged),
47​ this, &CubeSlideEffect::slotDesktopChanged);
48​ connect(effects, &EffectsHandler::windowStepUserMovedResized,
49​ this, &CubeSlideEffect::slotWindowStepUserMovedResized);
50​ connect(effects, &EffectsHandler::windowFinishUserMovedResized,
zzagUnsubmitted
Done

Please take a look at QOverload. http://doc.qt.io/qt-5/qtglobal.html#qOverload

QOverload<int, int, EffectWindow *>::of(&EffectsHandler::desktopChanged)

zzag: Please take a look at QOverload. http://doc.qt.io/qt-5/qtglobal.html#qOverload QOverload<int…
51​ this, &CubeSlideEffect::slotWindowFinishUserMovedResized);
44 reconfigure(ReconfigureAll); 52​ reconfigure(ReconfigureAll);
45} 53​}
46 54​
Context not available.
68 76​
69void CubeSlideEffect::prePaintScreen(ScreenPrePaintData& data, int time) 77​void CubeSlideEffect::prePaintScreen(ScreenPrePaintData& data, int time)
70{ 78​{
71 if (!slideRotations.empty()) { 79​ if (isActive()) {
72 data.mask |= PAINT_SCREEN_TRANSFORMED | Effect::PAINT_SCREEN_WITH_TRANSFORMED_WINDOWS | PAINT_SCREEN_BACKGROUND_FIRST; 80​ data.mask |= PAINT_SCREEN_TRANSFORMED | PAINT_SCREEN_WITH_TRANSFORMED_WINDOWS | PAINT_SCREEN_BACKGROUND_FIRST;
73 timeLine.setCurrentTime(timeLine.currentTime() + time); 81​ timeLine.setCurrentTime(timeLine.currentTime() + time);
74 if (windowMoving && timeLine.currentTime() > progressRestriction * (qreal)timeLine.duration()) 82​ if (windowMoving && timeLine.currentTime() > progressRestriction * (qreal)timeLine.duration())
75 timeLine.setCurrentTime(progressRestriction * (qreal)timeLine.duration()); 83​ timeLine.setCurrentTime(progressRestriction * (qreal)timeLine.duration());
76 if (dontSlidePanels) ​
77 panels.clear(); ​
78 stickyWindows.clear(); ​
79 } 84​ }
80 effects->prePaintScreen(data, time); 85​ effects->prePaintScreen(data, time);
81} 86​}
82 87​
83void CubeSlideEffect::paintScreen(int mask, QRegion region, ScreenPaintData& data) 88​void CubeSlideEffect::paintScreen(int mask, QRegion region, ScreenPaintData& data)
84{ 89​{
85 if (!slideRotations.empty()) { 90​ if (isActive()) {
86 glEnable(GL_CULL_FACE); 91​ glEnable(GL_CULL_FACE);
87 glCullFace(GL_FRONT); 92​ glCullFace(GL_FRONT);
88 paintSlideCube(mask, region, data); 93​ paintSlideCube(mask, region, data);
89 glCullFace(GL_BACK); 94​ glCullFace(GL_BACK);
90 paintSlideCube(mask, region, data); 95​ paintSlideCube(mask, region, data);
91 glDisable(GL_CULL_FACE); 96​ glDisable(GL_CULL_FACE);
92 97​ // Paint an extra screen with 'sticky' windows.
93 if (dontSlidePanels) { 98​ if (!staticWindows.isEmpty()) {
94 foreach (EffectWindow * w, panels) { 99​ stickyPainting = true;
95 WindowPaintData wData(w); 100​ effects->paintScreen(mask, region, data);
96 effects->paintWindow(w, 0, infiniteRegion(), wData); 101​ stickyPainting = false;
97 } ​
98 } ​
99 foreach (EffectWindow * w, stickyWindows) { ​
100 WindowPaintData wData(w); ​
101 effects->paintWindow(w, 0, infiniteRegion(), wData); ​
102 } 102​ }
zzagUnsubmitted
Done

Maybe, check if there any static windows.

zzag: Maybe, check if there any static windows.
zzagUnsubmitted
Done

if (!staticWindows.isEmpty()) {

...

}

zzag: if (!staticWindows.isEmpty()) { ... }
103 } else 103​ } else
104 effects->paintScreen(mask, region, data); 104​ effects->paintScreen(mask, region, data);
Context not available.
181 181​
182void CubeSlideEffect::prePaintWindow(EffectWindow* w, WindowPrePaintData& data, int time) 182​void CubeSlideEffect::prePaintWindow(EffectWindow* w, WindowPrePaintData& data, int time)
183{ 183​{
184 if (!slideRotations.empty() && cube_painting) { 184​ if (stickyPainting) {
185 QRect rect = effects->clientArea(FullArea, effects->activeScreen(), painting_desktop); 185​ if (staticWindows.contains(w)) {
186 if (dontSlidePanels && w->isDock()) { 186​ w->enablePainting(EffectWindow::PAINT_DISABLED_BY_DESKTOP);
187 w->setData(WindowForceBlurRole, QVariant(true)); 187​ } else {
188 panels.insert(w); 188​ w->disablePainting(EffectWindow::PAINT_DISABLED_BY_DESKTOP);
189 } 189​ }
190 if (!w->isManaged()) { 190​ } else if (isActive() && cube_painting) {
191 w->setData(WindowForceBlurRole, QVariant(true)); 191​ if (staticWindows.contains(w)) {
192 stickyWindows.insert(w); 192​ w->disablePainting(EffectWindow::PAINT_DISABLED_BY_DESKTOP);
193 } else if (dontSlideStickyWindows && !w->isDock() && 193​ effects->prePaintWindow(w, data, time);
194 !w->isDesktop() && w->isOnAllDesktops()) { 194​ return;
zzagUnsubmitted
Done

I think we should force roles at the beginning of the animation. And reset them at the end of the animation.

We should keep the number of memory allocations at the minimum.

zzag: I think we should force roles at the beginning of the animation. And reset them at the end of…
poboikoAuthorUnsubmitted
Done

Yeah, I though about it. It's just I wanted to avoid code duplication, since animation starts in several different places (i.e. when desktop changes, but also when user moves window through the border). I guess I'll make another function which does it (and maybe other common stuff)

poboiko: Yeah, I though about it. It's just I wanted to avoid code duplication, since animation starts…
195 w->setData(WindowForceBlurRole, QVariant(true)); ​
196 stickyWindows.insert(w); ​
197 } 195​ }
196​ QRect rect = effects->clientArea(FullArea, effects->activeScreen(), painting_desktop);
198 if (w->isOnDesktop(painting_desktop)) { 197​ if (w->isOnDesktop(painting_desktop)) {
199 if (w->x() < rect.x()) { 198​ if (w->x() < rect.x()) {
200 data.quads = data.quads.splitAtX(-w->x()); 199​ data.quads = data.quads.splitAtX(-w->x());
Context not available.
246 245​
247void CubeSlideEffect::paintWindow(EffectWindow* w, int mask, QRegion region, WindowPaintData& data) 246​void CubeSlideEffect::paintWindow(EffectWindow* w, int mask, QRegion region, WindowPaintData& data)
248{ 247​{
249 if (!slideRotations.empty() && cube_painting) { 248​ if (isActive() && cube_painting && !staticWindows.contains(w)) {
250 if (dontSlidePanels && w->isDock()) ​
251 return; ​
252 if (stickyWindows.contains(w)) ​
253 return; ​
254 ​
255 // filter out quads overlapping the edges 249​ // filter out quads overlapping the edges
256 QRect rect = effects->clientArea(FullArea, effects->activeScreen(), painting_desktop); 250​ QRect rect = effects->clientArea(FullArea, effects->activeScreen(), painting_desktop);
257 if (w->isOnDesktop(painting_desktop)) { 251​ if (w->isOnDesktop(painting_desktop)) {
Context not available.
351void CubeSlideEffect::postPaintScreen() 345​void CubeSlideEffect::postPaintScreen()
352{ 346​{
353 effects->postPaintScreen(); 347​ effects->postPaintScreen();
354 if (!slideRotations.empty()) { 348​ if (isActive()) {
355 if (timeLine.currentValue() == 1.0) { 349​ if (timeLine.currentValue() == 1.0) {
356 RotationDirection direction = slideRotations.dequeue(); 350​ RotationDirection direction = slideRotations.dequeue();
357 switch(direction) { 351​ switch(direction) {
Context not available.
386 else 380​ else
387 timeLine.setCurveShape(QTimeLine::LinearCurve); 381​ timeLine.setCurveShape(QTimeLine::LinearCurve);
388 if (slideRotations.empty()) { 382​ if (slideRotations.empty()) {
389 foreach (EffectWindow * w, panels) 383​ for (EffectWindow* w : staticWindows) {
390 w->setData(WindowForceBlurRole, QVariant(false)); 384​ w->setData(WindowForceBlurRole, QVariant());
391 foreach (EffectWindow * w, stickyWindows) 385​ w->setData(WindowForceBackgroundContrastRole, QVariant());
392 w->setData(WindowForceBlurRole, QVariant(false)); 386​ }
393 stickyWindows.clear(); 387​ staticWindows.clear();
394 panels.clear(); ​
395 effects->setActiveFullScreenEffect(0); 388​ effects->setActiveFullScreenEffect(0);
396 } 389​ }
397 } 390​ }
Context not available.
399 } 392​ }
400} 393​}
401 394​
402void CubeSlideEffect::slotDesktopChanged(int old, int current) 395​void CubeSlideEffect::slotDesktopChanged(int old, int current, EffectWindow* w)
403{ 396​{
397​ Q_UNUSED(w)
404 if (effects->activeFullScreenEffect() && effects->activeFullScreenEffect() != this) 398​ if (effects->activeFullScreenEffect() && effects->activeFullScreenEffect() != this)
405 return; 399​ return;
406 if (old > effects->numberOfDesktops()) { 400​ if (old > effects->numberOfDesktops()) {
zzagUnsubmitted
Done

Q_UNUSED(w)

zzag: Q_UNUSED(w)
Context not available.
499 } 493​ }
500 timeLine.setDuration((float)rotationDuration / (float)slideRotations.count()); 494​ timeLine.setDuration((float)rotationDuration / (float)slideRotations.count());
501 if (activate) { 495​ if (activate) {
502 if (slideRotations.count() == 1) 496​ startAnimation();
503 timeLine.setCurveShape(QTimeLine::EaseInOutCurve); ​
504 else ​
505 timeLine.setCurveShape(QTimeLine::EaseInCurve); ​
506 effects->setActiveFullScreenEffect(this); ​
507 timeLine.setCurrentTime(0); ​
508 front_desktop = old; 497​ front_desktop = old;
509 effects->addRepaintFull(); 498​ effects->addRepaintFull();
510 } 499​ }
511} 500​}
512 501​
502​void CubeSlideEffect::startAnimation() {
503​ const EffectWindowList windows = effects->stackingOrder();
504​ for (EffectWindow* w : windows) {
505​ if (!shouldAnimate(w)) {
zzagUnsubmitted
Done

It will detach. See https://www.dvratil.cz/2015/06/qt-containers-and-c11-range-based-loops/

"Proper" way would be

const EffectWindowList windows = effects->stackingOrder();
for (EffectWindow *w : windows) {
}
zzag: It will detach. See https://www.dvratil.cz/2015/06/qt-containers-and-c11-range-based-loops/…
506​ w->setData(WindowForceBlurRole, QVariant(true));
507​ w->setData(WindowForceBackgroundContrastRole, QVariant(true));
508​ staticWindows.insert(w);
509​ }
510​ }
511​ if (slideRotations.count() == 1) {
512​ timeLine.setCurveShape(QTimeLine::EaseInOutCurve);
513​ } else {
514​ timeLine.setCurveShape(QTimeLine::EaseInCurve);
515​ }
516​ effects->setActiveFullScreenEffect(this);
517​ timeLine.setCurrentTime(0);
518​}
519​
zzagUnsubmitted
Done

Please use curly braces.

https://community.kde.org/Policies/Kdelibs_Coding_Style#Braces

zzag: Please use curly braces. https://community.kde.org/Policies/Kdelibs_Coding_Style#Braces
520​void CubeSlideEffect::slotWindowAdded(EffectWindow* w) {
521​ if (!isActive() || shouldAnimate(w)) {
522​ return;
zzagUnsubmitted
Done

Please use isActive instead.

zzag: Please use `isActive` instead.
523​ }
524​ staticWindows.insert(w);
525​ w->setData(WindowForceBlurRole, QVariant(true));
526​ w->setData(WindowForceBackgroundContrastRole, QVariant(true));
zzagUnsubmitted
Not Done

Readability nitpick: isActive() and shouldAnimate check two different things. It would be better to have something like

if (!isActive()) {
    return;
}
if (!shouldAnimate(w)) {
    ...
}
zzag: Readability nitpick: isActive() and shouldAnimate check two different things. It would be…
527​}
528​
zzagUnsubmitted
Done

Please force blur only for panels and sticky windows.

The blur effect can't work in 3d.

zzag: Please force blur only for panels and sticky windows. The blur effect can't work in 3d.
poboikoAuthorUnsubmitted
Done

There is a check for !shouldAnimate, it's fine.

poboiko: There is a check for !shouldAnimate, it's fine.
529​void CubeSlideEffect::slotWindowDeleted(EffectWindow* w) {
530​ staticWindows.remove(w);
531​}
532​
zzagUnsubmitted
Done

No, that's wrong. During the cube slide animation windows are 3D transformed. Thus, that's wrong to blur background behind them.

We should force blur and background contrast only for sticky windows. Also, I think we don't need to keep track of window with forced roles. So, please delete m_forcedRoles.

zzag: No, that's wrong. During the cube slide animation windows are 3D transformed. Thus, that's…
poboikoAuthorUnsubmitted
Done

I thought about it initially, but I found out that it actually works fine - it blurs background on the moving cube face for non-sticky windows.

Concerning m_forcedRoles - I added it so I can remove those roles afterwards. You're saying that I don't really have to do it?

poboiko: I thought about it initially, but I found out that it actually works fine - it blurs background…
zzagUnsubmitted
Done

Yes, we don't need it. We'll force blur only for panels and sticky windows. Thus, m_forcedRoles will be redundant.

zzag: Yes, we don't need it. We'll force blur only for panels and sticky windows. Thus, m_forcedRoles…
zzagUnsubmitted
Done

You don't really need it.

zzag: You don't really need it.
533​bool CubeSlideEffect::shouldAnimate(const EffectWindow* w) const
534​{
535​ if (w->isDock()) {
536​ return !dontSlidePanels;
537​ }
538​ if (w->isOnAllDesktops()) {
zzagUnsubmitted
Not Done

We always have to animate desktop window.

if (w->isDesktop()) {
    return true;
}
zzag: We always have to animate desktop window. ```lang=cpp if (w->isDesktop()) { return true; }…
539​ if (!w->isManaged()) {
540​ return false;
541​ }
542​ return !dontSlideStickyWindows;
543​ }
zzagUnsubmitted
Done

Hm, what about renaming it to isStickyWindow?

zzag: Hm, what about renaming it to `isStickyWindow`?
poboikoAuthorUnsubmitted
Done

Well, it also includes panels, but I can change it (probably, that semantics is even more obvious due to the context).

poboiko: Well, it also includes panels, but I can change it (probably, that semantics is even more…
zzagUnsubmitted
Done

Ok, let's leave it as it is.

zzag: Ok, let's leave it as it is.
544​ return true;
545​}
546​
513void CubeSlideEffect::slotWindowStepUserMovedResized(EffectWindow* w) 547​void CubeSlideEffect::slotWindowStepUserMovedResized(EffectWindow* w)
514{ 548​{
zzagUnsubmitted
Done

It would animate notifications, desktop switch OSD, etc. What about

if (dontSlideStickyWindows && w->isOnAllDesktops()) {
    if (w->isDesktop()) {
        return true;
    }
    // other checks?
    return false;
}

Keep in mind, that's just a hypothesis.

zzag: It would animate notifications, desktop switch OSD, etc. What about ```lang=cpp if…
poboikoAuthorUnsubmitted
Done

Oh. It actually might work, thanks! Will do the testing.

poboiko: Oh. It actually might work, thanks! Will do the testing.
515 if (!useWindowMoving) 549​ if (!useWindowMoving)
zzagUnsubmitted
Done

Maybe, we should force blur and background contrast only for sticky windows.

zzag: Maybe, we should force blur and background contrast only for sticky windows.
Context not available.
594 front_desktop = effects->currentDesktop(); 628​ front_desktop = effects->currentDesktop();
595 if (slideRotations.isEmpty()) { 629​ if (slideRotations.isEmpty()) {
596 slideRotations.enqueue(direction); 630​ slideRotations.enqueue(direction);
597 timeLine.setCurveShape(QTimeLine::EaseInOutCurve); ​
598 windowMoving = true; 631​ windowMoving = true;
599 effects->setActiveFullScreenEffect(this); 632​ startAnimation();
600 } 633​ }
601 effects->addRepaintFull(); 634​ effects->addRepaintFull();
602} 635​}
Context not available.