[effects] replace old slide effect with a new one
ClosedPublic

Authored by zzag on Jan 3 2018, 1:59 PM.

Details

Summary

The new slide effect tries to separate each virtual desktop
as much as possible. This separation makes the new slide
effect more intuitive than the old one.

Test Plan
  • switch between virtual desktops
  • or, move a window to another virtual desktop

Diff Detail

Repository
R108 KWin
Branch
effects/slide
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 4 2018, 10:59 AM
zzag added a comment.Jan 4 2018, 12:04 PM

Memory allocations in SlideDesktopsEffect::paintScreen() creep me out. We should not allocate memory during paint calls! If you have any ideas how to solve this, please let me know. Currently, I'm thinking about reusing a vector(for visibleDesktops) and a boolean matrix(for m_paintCtx.fullscreenWindows; I guess name should be changed).

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 4 2018, 12:04 PM
zzag updated this revision to Diff 24932.Jan 8 2018, 11:48 AM

Updating D9638: [effects] add 'Slide Desktops' effect

  • Add 'Slide docks' option
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 8 2018, 11:48 AM
zzag added a comment.EditedJan 8 2018, 11:54 AM

Added 'Slide docks' option:

This option is useful, for example, when using latte-dock.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 8 2018, 11:54 AM
mart added a subscriber: mart.Jan 12 2018, 3:42 PM

Just putting it as an idea: maybe we should just drop the "old" slide desktop effect and replace it with this one?

+1

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 12 2018, 3:42 PM

as we had quite positive feedback on the replacement idea: could you please rework the patch to replace the old effect?

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 12 2018, 5:44 PM
zzag added a comment.Jan 13 2018, 10:33 AM

as we had quite positive feedback on the replacement idea: could you please rework the patch to replace the old effect?

OK.

Correct me if I'm wrong. This effect should be renamed to Slide.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 13 2018, 10:33 AM
zzag updated this revision to Diff 25273.Jan 13 2018, 4:00 PM

Replace old slide effect with this one.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 13 2018, 4:00 PM
zzag updated this revision to Diff 25274.Jan 13 2018, 4:08 PM
zzag retitled this revision from [effects] add 'Slide Desktops' effect to [effects] replace old slide effect with a new one.
zzag edited the summary of this revision. (Show Details)
zzag edited projects, added Plasma; removed KWin.

Update title and summary.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 13 2018, 4:08 PM
zzag added a comment.EditedJan 13 2018, 4:11 PM

@graesslin: I've made some changes to reconfigure()(file effects/slide/slide.cpp, lines 59-66). Could you please review them?

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 13 2018, 4:11 PM
graesslin added inline comments.Jan 13 2018, 7:42 PM
effects/slide/slide.cpp
62–68

So far the interpretation of the config values was: 0 means default settings, anything else means taken from the config. As we have old configs this change would basically disable the animation for everybody with default settings. Given that I suggest to keep the approach as in the old variant.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 13 2018, 7:42 PM
zzag updated this revision to Diff 25283.Jan 13 2018, 8:35 PM

Use default duration value when Duration is 0.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 13 2018, 8:35 PM
zzag marked an inline comment as done.Jan 13 2018, 8:40 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 13 2018, 8:40 PM
zzag updated this revision to Diff 25915.Jan 24 2018, 10:02 PM
  • Rebase onto master
  • Do not populate a list of fullscreen windows if the 'Slide docks' checkbox is unchecked.
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 24 2018, 10:02 PM
zzag added a comment.Jan 24 2018, 10:27 PM

I think this effect is ready for testing.

Yet, I would like to have some clipping but it seems like there is no way to do this with current implementation of Scene::paintGenericScreen.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 24 2018, 10:27 PM
In D9638#195793, @zzag wrote:

I think this effect is ready for testing.

Yet, I would like to have some clipping but it seems like there is no way to do this with current implementation of Scene::paintGenericScreen.

That's quite likely

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 25 2018, 3:59 PM

What you could try is using the PaintClipper to perform clipping. But I don't know whether it's still fully functional on OpenGL.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 25 2018, 4:01 PM
zzag added a comment.Jan 25 2018, 5:09 PM

What you could try is using the PaintClipper to perform clipping. But I don't know whether it's still fully functional on OpenGL.

So, adding

PaintClipper pc(QRegion(
  QPoint(0, 0),
  effects->virtualScreenSize()
));

in SlideEffect::paintScreen would be enough? Doesn't KWin do something like that somewhere else? I've just realized, maybe, this effect doesn't need to clip...


Also, I've noticed that there is no blur behind docks when docks are painted above all windows(Slide docks checkbox is unchecked).

Effect chain positions:

  • slide effect: 50
  • blur: 75
  • background contrast: 76

Docks are drawn above all windows by the following code:

for (EffectWindow* w : windows) {
    if (! w->isDock()) {
        continue;
    }
    WindowPaintData dockData(w);
    int dockMask = mask
        | (w->hasAlpha() ? PAINT_WINDOW_TRANSLUCENT
                         : PAINT_WINDOW_OPAQUE);
    effects->drawWindow(w, dockMask, infiniteRegion(), dockData);
}

(WindowForceBackgroundContrastRole and WindowForceBlurRole are set at the beginning of the slide animation)

Am I missing something?


Quick side note: when the Slide docks checkbox is unchecked, docks are drawn above other windows so they can properly animate themselves if an user enters or leaves a virtual desktop with a window in full screen mode.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 25 2018, 5:09 PM
In D9638#196026, @zzag wrote:

What you could try is using the PaintClipper to perform clipping. But I don't know whether it's still fully functional on OpenGL.

So, adding

PaintClipper pc(QRegion(
  QPoint(0, 0),
  effects->virtualScreenSize()
));

in SlideEffect::paintScreen would be enough? Doesn't KWin do something like that somewhere else? I've just realized, maybe, this effect doesn't need to clip...

I just looked into the source (relevant method Scene::paintScreen). For PAINT_SCREEN_WITH_TRANSFORMED_WINDOWS the region is set to infinite region.

if (*mask & (PAINT_SCREEN_TRANSFORMED | PAINT_SCREEN_WITH_TRANSFORMED_WINDOWS)) {
    // Region painting is not possible with transformations,
    // because screen damage doesn't match transformed positions.
    *mask &= ~PAINT_SCREEN_REGION;
    region = infiniteRegion();

The region in this method is later on used for clipping. Given that you need that paintclipper fragment.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 25 2018, 8:15 PM

Docks are drawn above all windows by the following code:

for (EffectWindow* w : windows) {
    if (! w->isDock()) {
        continue;
    }
    WindowPaintData dockData(w);
    int dockMask = mask
        | (w->hasAlpha() ? PAINT_WINDOW_TRANSLUCENT
                         : PAINT_WINDOW_OPAQUE);
    effects->drawWindow(w, dockMask, infiniteRegion(), dockData);
}

(WindowForceBackgroundContrastRole and WindowForceBlurRole are set at the beginning of the slide animation)

Am I missing something?

I have an idea: the problem might be how you create the WindowPaintData. The ctor you use creates the WindowPaintData with an identity screenProjectionMatrix. This might be wrong in this case. The blur effect seems to use it.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 25 2018, 8:25 PM
zzag added a comment.Jan 25 2018, 9:33 PM

Docks are drawn above all windows by the following code:

for (EffectWindow* w : windows) {
    if (! w->isDock()) {
        continue;
    }
    WindowPaintData dockData(w);
    int dockMask = mask
        | (w->hasAlpha() ? PAINT_WINDOW_TRANSLUCENT
                         : PAINT_WINDOW_OPAQUE);
    effects->drawWindow(w, dockMask, infiniteRegion(), dockData);
}

(WindowForceBackgroundContrastRole and WindowForceBlurRole are set at the beginning of the slide animation)

Am I missing something?

I have an idea: the problem might be how you create the WindowPaintData. The ctor you use creates the WindowPaintData with an identity screenProjectionMatrix. This might be wrong in this case. The blur effect seems to use it.

Maybe just elevate docks? E.g. at the beginning for each dock call effects->setElevatedWindow(w, true) and paint docks with the last visible virtual desktop?

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 25 2018, 9:33 PM
zzag added a comment.Jan 25 2018, 9:40 PM

The region in this method is later on used for clipping. Given that you need that paintclipper fragment.

I think PaintClipper pc(QRegion(effects->virtualScreenGeometry())); should be enough.(given that the slide effect only translates windows)

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 25 2018, 9:40 PM
zzag added a comment.Jan 25 2018, 9:44 PM

@graesslin: do elevated windows have order? (e.g. docks are above fullscreen windows, etc)

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 25 2018, 9:44 PM
zzag added a comment.Jan 26 2018, 3:17 PM

I would like to test this effect in "full KDE Plasma wayland session", but I can't start plasma. After splash screen there is only black screen.

Sometimes, there are messages like this

kscreen.kwayland: Connection to Wayland server at socket: "wayland-0" timed out.

Does anyone know how to fix this?


My workflow

xhost +

sudo docker run --rm \
-v /tmp/.X11-unix:/tmp/.X11-unix \
-e DISPLAY=:0 \
--device /dev/dri \
--device /dev/snd \
--device /dev/video0 \
--cap-add ALL \
-v /home/vlad/KDE/build:/home/neon/build \
-v /home/vlad/KDE/log:/home/neon/log \
-v /home/vlad/KDE/src:/home/neon/src \
-v /home/vlad/KDE/usr:/home/neon/usr \
-v /home/vlad/KDE/.kdesrc-buildrc:/home/neon/.kdesrc-buildrc \
-v /home/vlad/KDE/.kdesrc-build:/home/neon/.kdesrc-build \
-ti kdebuilder:v2 bash

##### inside container

$ kdesrc-build --include-dependencies plasma-desktop
$ source kde-env-master.sh
$ startplasmacompositor

(kdebuilder:v2 is an image based on kdeneon/plasma:dev-unstable with some -dev packages preinstalled)

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 26 2018, 3:17 PM
In D9638#196184, @zzag wrote:

@graesslin: do elevated windows have order? (e.g. docks are above fullscreen windows, etc)

They are above all other windows.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 26 2018, 4:30 PM
zzag updated this revision to Diff 26015.Jan 26 2018, 6:18 PM
  • clip with the PaintClipper
  • elevate docks when Slide docks is unchecked
  • rewritten logic which forces blur and background contrast

(tested only on X11)

PS. I wish I could test Wayland part.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 26 2018, 6:18 PM
zzag added a comment.Jan 26 2018, 6:22 PM

If you know how to start wayland session, please let me know. Till then, I can't do too much (I'm familiar with effects part only).

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 26 2018, 6:22 PM
In D9638#196539, @zzag wrote:

If you know how to start wayland session, please let me know. Till then, I can't do too much (I'm familiar with effects part only).

I cannot help you with the docker setup, but I can describe how I do my testing:

export $(dbus-launch)
cd build/kde/workspace/kwin/bin
gdb --args ./kwin_wayland --socket wayland-1

Then from other konsole tab

WAYLAND_DISPLAY=wayland-1 kwrite

or any other application I want to try. For testing something like the effect I would probably launch qdbusviewer together with kwin_wayland and use it to switch desktops.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 26 2018, 9:54 PM
zzag added a comment.Jan 26 2018, 10:00 PM

I'll try to build Plasma on my host. I hope startplasmacompositor will launch full Plasma session.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 26 2018, 10:00 PM
zzag added a comment.Jan 27 2018, 9:28 PM

... and it didn't work. I'll ask for help on #plasma IRC.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 27 2018, 9:28 PM
zzag added a comment.Jan 28 2018, 3:40 PM

I've got full(or some part) KDE Plasma wayland session working. Here's what I did:

  • source ~/.config/kde-env-master.sh (you should have install-environment-driver true in your .kdesrc-buildrc)
  • start kwin_wayland with Konsole
  • change some environment variables, like XDG_DATA_DIRS
  • and then launch /usr/lib/startplasma (I'm using plasma shell provided by my distro)

Hacky, but it's working.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 28 2018, 3:40 PM
zzag updated this revision to Diff 26137.Jan 28 2018, 3:41 PM
zzag edited projects, added Plasma; removed KWin.
  • rebase on master
  • finish work on shouldForceBlur() and shouldForceBackgroundContrast()
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 28 2018, 3:41 PM
zzag added a comment.Jan 28 2018, 3:42 PM

@graesslin: could you please review code?

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 28 2018, 3:42 PM
zzag updated this revision to Diff 26788.Feb 8 2018, 8:40 PM

rebase onto master

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 8 2018, 8:40 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 8 2018, 8:41 PM
zzag updated this revision to Diff 27190.Feb 14 2018, 9:57 PM

rebase on master

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 14 2018, 9:57 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 15 2018, 11:19 AM
graesslin accepted this revision.Feb 15 2018, 4:50 PM
This revision is now accepted and ready to land.Feb 15 2018, 4:50 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 15 2018, 4:50 PM
This revision was automatically updated to reflect the committed changes.
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 16 2018, 4:09 AM

Hello,

I just upgraded my system and got the update with this new change. Is it possible to add an option Slide wallpaper similar to the current option Slide docks?
On my 31" display at home the sliding of the wallpaper is a bit too much, though I do like it when the applications themselves slide like in the old version.

Something minor I noticed: In the Virtual Desktops systems settings module there is also a Switching tab. Next to the Animation: selector the settings button is greyed out however, had to access it via the regular desktop effects.

Thanks!

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJun 25 2018, 7:43 PM
zzag added a comment.Jun 25 2018, 8:02 PM

I just upgraded my system and got the update with this new change. Is it possible to add an option Slide wallpaper similar to the current option Slide docks?
On my 31" display at home the sliding of the wallpaper is a bit too much, though I do like it when the applications themselves slide like in the old version.

In 5.14, there will be "Slide desktop background" option.

Something minor I noticed: In the Virtual Desktops systems settings module there is also a Switching tab. Next to the Animation: selector the settings button is greyed out however, had to access it via the regular desktop effects.

Already fixed.

In D9638#282890, @zzag wrote:

I just upgraded my system and got the update with this new change. Is it possible to add an option Slide wallpaper similar to the current option Slide docks?
On my 31" display at home the sliding of the wallpaper is a bit too much, though I do like it when the applications themselves slide like in the old version.

In 5.14, there will be "Slide desktop background" option.

Something minor I noticed: In the Virtual Desktops systems settings module there is also a Switching tab. Next to the Animation: selector the settings button is greyed out however, had to access it via the regular desktop effects.

Already fixed.

Thanks for the quick reply! I will await 5.14's release then. :)