Details

Reviewers

Group Reviewers

KWin
Plasma

Summary

This effect animates transition to/from fullscreen mode. For the
most part it is a copy-paste of the Maximize effect with some editing.

Test Plan

open an application
toggle fullscreen mode

Diff Detail

Repository

R108 KWin

Branch

effects/fullscreen

Lint

No Linters Available

Unit

No Unit Test Coverage

zzag created this revision.Dec 18 2017, 9:41 AM

Restricted Application added a project: KWin. · View Herald TranscriptDec 18 2017, 9:41 AM

Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript

zzag requested review of this revision.Dec 18 2017, 9:41 AM

Thanks for your patch! I did a similar patch a while ago but I didn't submit it as it seems to cause visual glitches with various apps, such as when fullscreening Yakuake or Chrome.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 18 2017, 10:43 AM

In D9391#180624, @broulik wrote:

Thanks for your patch! I did a similar patch a while ago but I didn't submit it as it seems to cause visual glitches with various apps, such as when fullscreening Yakuake or Chrome.

I guess you should see glitches when maximizing windows too. For example, when I maximize Firefox with reddit, sidebar flicks, etc. So, I guess, that's a side effect of maximize effect.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 18 2017, 11:10 AM

graesslin added a subscriber: graesslin.Dec 18 2017, 8:18 PM

graesslin added inline comments.

effects/fullscreen/package/contents/code/fullscreen.js
4 ↗	(On Diff #24055)	cough utf8 please
effects/fullscreen/package/metadata.desktop
3 ↗	(On Diff #24055)	You don't need to add translations. The tooling will overwrite them automatically.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 18 2017, 8:18 PM

Updating D9391: [effects] Add 'Fullscreen' effect

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 20 2017, 3:29 AM

zzag marked 2 inline comments as done.Dec 20 2017, 3:29 AM

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 20 2017, 3:29 AM

It seems to be working, but there is a problem with GTK apps. They have wrong positions in their geometries:

Output like "{...} -> {...}" comes from geometryChanged:

js
geometryChanged: function (window, oldGeometry) {
    "use strict";
    print(JSON.stringify(oldGeometry), "->", JSON.stringify(window.geometry));
    if (window.fullScreenAnimation) {

Output like "oldGeometry = ..." comes from fullScreenChanged:

js
fullScreenChanged: function (window) {
    "use strict";
    if (!window.oldGeometry) {
        return;
    }

    var oldGeometry = window.oldGeometry,
        newGeometry = window.geometry;
    if (oldGeometry.width == newGeometry.width &&
            oldGeometry.height == newGeometry.height)
        oldGeometry = window.olderGeometry;
    window.olderGeometry = window.oldGeometry;
    window.oldGeometry = newGeometry;

    print("oldGeometry=", JSON.stringify(oldGeometry));
    print("newGeometry=", JSON.stringify(newGeometry));
    print("contentsRect=", JSON.stringify(window.contentsRect));
    print("decorationInnerRect=", JSON.stringify(window.decorationInnerRect));

    window.fullScreenAnimation = animate({

Konsole(Qt): Entering Fullscreen mode

kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":55,\"y\":0,\"width\":1000,\"height\":583} -> {\"x\":59,\"y\":29,\"width\":992,\"height\":550}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "oldGeometry= {\"x\":59,\"y\":29,\"width\":992,\"height\":550}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "newGeometry= {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "contentsRect= {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "decorationInnerRect= {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":0,\"y\":0,\"width\":1366,\"height\":768} -> {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":55,\"y\":0,\"width\":1000,\"height\":583} -> {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"

everything is fine!

Konsole(Qt): Leaving Fullscreen mode

kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":0,\"y\":0,\"width\":1366,\"height\":768} -> {\"x\":-4,\"y\":-29,\"width\":1374,\"height\":801}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "oldGeometry= {\"x\":-4,\"y\":-29,\"width\":1374,\"height\":801}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "newGeometry= {\"x\":55,\"y\":0,\"width\":1000,\"height\":583}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "contentsRect= {\"x\":4,\"y\":29,\"width\":992,\"height\":550}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "decorationInnerRect= {\"x\":4,\"y\":29,\"width\":992,\"height\":550}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":55,\"y\":0,\"width\":1000,\"height\":583} -> {\"x\":55,\"y\":0,\"width\":1000,\"height\":583}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":0,\"y\":0,\"width\":1366,\"height\":768} -> {\"x\":55,\"y\":0,\"width\":1000,\"height\":583}"

everything is fine!

Gimp(gtk): Entering Fullscreen mode

kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":87,\"y\":0,\"width\":1141,\"height\":593} -> {\"x\":87,\"y\":0,\"width\":1133,\"height\":560}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "oldGeometry= {\"x\":87,\"y\":0,\"width\":1133,\"height\":560}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "newGeometry= {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "contentsRect= {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "decorationInnerRect= {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":0,\"y\":0,\"width\":1366,\"height\":768} -> {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":87,\"y\":0,\"width\":1141,\"height\":593} -> {\"x\":0,\"y\":0,\"width\":1366,\"height\":768}"

should be

kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "oldGeometry= {\"x\":87,\"y\":29,\"width\":1133,\"height\":560}"

NOTE: width and height are ok!

Gimp(gtk): Leaving Fullscreen mode

kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":0,\"y\":0,\"width\":1366,\"height\":768} -> {\"x\":0,\"y\":0,\"width\":1374,\"height\":801}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "oldGeometry= {\"x\":0,\"y\":0,\"width\":1374,\"height\":801}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "newGeometry= {\"x\":87,\"y\":0,\"width\":1141,\"height\":593}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "contentsRect= {\"x\":4,\"y\":29,\"width\":1133,\"height\":560}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "decorationInnerRect= {\"x\":4,\"y\":29,\"width\":1133,\"height\":560}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":87,\"y\":0,\"width\":1141,\"height\":593} -> {\"x\":87,\"y\":0,\"width\":1141,\"height\":593}"
kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "{\"x\":0,\"y\":0,\"width\":1366,\"height\":768} -> {\"x\":87,\"y\":0,\"width\":1141,\"height\":593}"

should be:

kwin_scripting: "/home/dev/kde/usr/share/kwin/effects/kwin4_effect_fullscreen/contents/code/fullscreen.js" : "oldGeometry= {\"x\":-4,\"y\":-29,\"width\":1374,\"height\":801}"

NOTE: width and height are ok!

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 20 2017, 3:36 AM

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 20 2017, 3:48 AM

NOTE: Window gravity defines the meaning of coordinates passed to gtk_window_move(). See gtk_window_move() and GdkGravity for more details. The default window gravity is GDK_GRAVITY_NORTH_WEST which will typically “do what you mean.” https://developer.gnome.org/gtk3/stable/GtkWindow.html#gtk-window-set-gravity

I guess the only option is to force StaticGravity when entering full screen mode.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 20 2017, 5:05 AM

[effects] Add 'Fullscreen' effect

This effect animates transition to/from fullscreen mode. For the
most part it is a copy-paste of the Maximize effect with some editing.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 20 2017, 7:25 AM

D6799 for the record (I like your patch better, actually, just if you want to look at my implementation)

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 22 2017, 10:13 AM

A possibility to solve this problem with gtk would be to restrict to Wayland windows. I'm absolutely against any changed to window gravity on X11. And I'm also not happy with a new animation if we already know that it doesn't work correctly. Given that 5.12 will be an LTS this might be unpleasant. I remember that it took several iterations and bugfixes to get the maximize effect work properly.

Due to that I personally would prefer if we don't add this for 5.12 or only for Wayland windows.

This revision now requires changes to proceed.Dec 22 2017, 1:22 PM

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 22 2017, 1:22 PM

As this adds a new effect: all the effectloader autotests need to be adjusted, otherweise the tests will start to fail.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 31 2017, 9:50 AM

Updating D9391: [effects] Add 'Fullscreen' effect

Add fullscreen effect to the effect loader tests.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 31 2017, 3:11 PM

Updating D9391: [effects] Add 'Fullscreen' effect

Squash commits.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 31 2017, 3:15 PM

In D9391#182439, @graesslin wrote:

I'm absolutely against any changed to window gravity on X11. And I'm also not happy with a new animation if we already know that it doesn't work correctly.

I don't like changing window gravity code either.

Given that 5.12 will be an LTS this might be unpleasant.

Yeah, it should be at least 5.13

Due to that I personally would prefer if we don't add this for 5.12 or only for Wayland windows.

Is it okay to support only Wayland?

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 31 2017, 4:20 PM

Could you please post a new video with a before and after? I would like to preview it.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 31 2017, 4:49 PM

In D9391#184440, @abetts wrote:

Could you please post a new video with a before and after? I would like to preview it.

Ok, I'll do it later. But keep in mind code hasn't changed much.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 31 2017, 5:57 PM

In D9391#184475, @zzag wrote:

In D9391#184440, @abetts wrote:

Could you please post a new video with a before and after? I would like to preview it.

Ok, I'll do it later. But keep in mind code hasn't changed much.

Awesome! Thanks

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 31 2017, 8:15 PM

Due to that I personally would prefer if we don't add this for 5.12 or only for Wayland windows.

Is it okay to support only Wayland?

Personally I would prefer to have the code generic. But we also see in the maximize effect that there are differences between X11 and Wayland which don't allow to have a generic effect (see bug 384462). So from a maintenance and forward compatibility point of view it makes sense to restrict to Wayland if we are aware that X11 is tricky.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 1 2018, 5:11 PM

So, I've tested this effect on Wayland..

Konsole(XWayland, Qt)

Seems, like cross fading doesn't work.

Konsole(natively on Wayland, Qt)

Maximize effect doesn't work (bug 384462?)
Konsole doesn't enter fullscreen mode

Firefox(XWayland, gtk)

Still, there are problems with FF.

GNOME Terminal(XWayland, gtk)

GNOME Terminal(Wayland, gtk)

Also, this effect is really buggy. Just keep pressing F11:

That's a really bad idea to cancel currently running animation and start a new one. It would be better to toggle direction of a timeline, but seems like there is no way to do this with js api.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 1 2018, 6:33 PM

broulik mentioned this in D6799: Apply maximize effect to full screen changes.Jan 2 2018, 12:41 PM

Updating D9391: [effects] Add 'Fullscreen' effect

I think this should be rewritten in C++ for several reasons:

we need to control timeline
when entering or leaving fullscreen mode, we should handle docks in a special way and I don't really think it is possible with js api

Also, please notice that signature of windowFullScreenChanged has changed.
Now, it also includes previous geometry of a window. This plus several changes
in geometry.cpp and shell_client.cpp should result in a generic
fullscreen effect, I hope.

PLEASE NOTICE THAT'S JUST A STUB, IT DOES NOTHING! (I'm intrested in feedback)

Also, seems like Qt have a bug. If you enter/leave fullscreen mode in any
Qt app, ShellClient::setFullScreen is not called.

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

Updating D9391: [effects] Add 'Fullscreen' effect

Re-implemented this effect. The only missing part is emitters of windowFullScreenChanged
signal.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 6 2018, 6:34 PM

X11

Xwayland

Problems

Seems like KWin has a bug. When a window enters/leaves fullscreen mode, there is a moment when the window is smaller than original(or some sort of it):

With 'Fullscreen' effect turned on

With 'Fullscreen' effect turned off

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 6 2018, 6:47 PM

Updating D9391: [effects] Add 'Fullscreen' effect

Emit windowFullScreenChanged signal
Unreference previous pixmap when a window is deleted

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 7 2018, 10:52 AM

The code is really bloated. Also, I lost interest in developing this effect.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 15 2018, 9:46 AM

zzag mentioned this in D12623: [effects/maximize] Enable blur behind windows.Apr 30 2018, 9:04 PM

		Path
M		abstract_client.h (2 lines)
M		autotests/test_builtin_effectloader.cpp (5 lines)
M		autotests/test_plugin_effectloader.cpp (1 line)
M		autotests/test_scripted_effectloader.cpp (1 line)
M		client.h (8 lines)
M		effects.cpp (9 lines)
M		effects/CMakeLists.txt (1 line)
M		effects/effect_builtins.h (1 line)
M		effects/effect_builtins.cpp (16 lines)
A	M	effects/fullscreen/CMakeLists.txt (7 lines)
A	M	effects/fullscreen/fullscreen.h (81 lines)
A	M	effects/fullscreen/fullscreen.cpp (178 lines)
A	M	effects/fullscreen/timeline.h (188 lines)
M		libkwineffects/kwineffects.h (8 lines)
M		shell_client.h (8 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	24055	38d3a37		Dec 18 2017, 9:41 AM	★	★
Diff 2	24139	7b11e14		Dec 20 2017, 3:29 AM	★	★
Diff 3	24141	6e10f99		Dec 20 2017, 7:25 AM	★	★
Diff 4	24522	a742c53		Dec 31 2017, 3:11 PM	★	★
Diff 5	24524	6e10f99		Dec 31 2017, 3:15 PM	★	★
Diff 6	24718	a252ff9		Jan 4 2018, 3:03 PM	★	★
Diff 7	24849	a252ff9		Jan 6 2018, 6:34 PM	★	★
Diff 8	24871	a252ff9		Jan 7 2018, 10:52 AM	★	★

[effects] Add 'Fullscreen' effect
AbandonedPublic
Actions

Details

Diff Detail

Konsole(Qt): Entering Fullscreen mode

Konsole(Qt): Leaving Fullscreen mode

Gimp(gtk): Entering Fullscreen mode

Gimp(gtk): Leaving Fullscreen mode

Konsole(XWayland, Qt)

Konsole(natively on Wayland, Qt)

Firefox(XWayland, gtk)

GNOME Terminal(XWayland, gtk)

GNOME Terminal(Wayland, gtk)

X11

Xwayland

Problems

Revision Contents
Changeset List

Diff 24871

abstract_client.h

autotests/test_builtin_effectloader.cpp

autotests/test_plugin_effectloader.cpp

autotests/test_scripted_effectloader.cpp

client.h

effects.cpp

effects/CMakeLists.txt

effects/effect_builtins.h

effects/effect_builtins.cpp

effects/fullscreen/CMakeLists.txt

effects/fullscreen/fullscreen.h

effects/fullscreen/fullscreen.cpp

effects/fullscreen/timeline.h

libkwineffects/kwineffects.h

shell_client.h

[effects] Add 'Fullscreen' effectAbandonedPublicActions

Details

Diff Detail

Konsole(Qt): Entering Fullscreen mode

Konsole(Qt): Leaving Fullscreen mode

Gimp(gtk): Entering Fullscreen mode

Gimp(gtk): Leaving Fullscreen mode

Konsole(XWayland, Qt)

Konsole(natively on Wayland, Qt)

Firefox(XWayland, gtk)

GNOME Terminal(XWayland, gtk)

GNOME Terminal(Wayland, gtk)

X11

Xwayland

Problems

Revision ContentsChangeset List

Diff 24871

abstract_client.h

autotests/test_builtin_effectloader.cpp

autotests/test_plugin_effectloader.cpp

autotests/test_scripted_effectloader.cpp

client.h

effects.cpp

effects/CMakeLists.txt

effects/effect_builtins.h

effects/effect_builtins.cpp

effects/fullscreen/CMakeLists.txt

effects/fullscreen/fullscreen.h

effects/fullscreen/fullscreen.cpp

effects/fullscreen/timeline.h

libkwineffects/kwineffects.h

shell_client.h

[effects] Add 'Fullscreen' effect
AbandonedPublic
Actions

Revision Contents
Changeset List