Bug 396790 - ScriptedEffects cannot call setActiveFullScreenEffect
Summary: ScriptedEffects cannot call setActiveFullScreenEffect
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: scripting (other bugs)
Version First Reported In: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-23 09:15 UTC by David Edmundson
Modified: 2018-10-03 21:57 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Edmundson 2018-07-23 09:15:49 UTC
This is needed for eg. the fade desktop effect to not clash with others.

(I'll do the QJSEngine port first, then I'll do it)

I'm not sure if we want to make it explicit or implicit from the API.

i.e


effects['desktopChanged'].connect(function(oldDesktop, newDesktop) {
   if (someoneElseHasActiveFullScreenEffect()) {
     return;
    }
    setUsAsActiveFullScreenEffect();
   //set up some animations
}

effect.activeChanged.connect(function() {
    if (!active) releaseActiveFullScreenEffect();
});

(not exact terms, could be a JS RAII equivalent or whatever)


OR to make it merged into the effect.animate like


effect.animate(w, Effect.whatever, duration, 0, FullScreen);

where the last flag no-ops if someone else is fullscreen otherwise calls setsFullScreenEffect(this) with some internal ref-counting.
Comment 1 Vlad Zahorodnii 2018-07-23 13:25:15 UTC
> This is needed for eg. the fade desktop effect to not clash with others.
It would also fix, in some sense, the Fade effect.

Please notice that desktopChanged(uint, uint) (or int?) is deprecated. The
preferred signal is the one with the 'with' argument. Keep in mind, that we
also need to compare EffectWindows (IIRC, in JavaScript, == doesn't work
for objects).
Comment 2 David Edmundson 2018-07-23 14:05:24 UTC
>(IIRC, in JavaScript, == doesn't work for objects).

for dynamic "Objects" maybe not, but that's for web people.

QtScript/QtJS is a very different world.

Wrapped QObjects work just as one would expect with the conventional direct pointer comparison underneath. (QV4ObjectWrapper::isEqualTo)
Comment 3 David Edmundson 2018-07-31 11:23:08 UTC
Edit. I came to a decision on this. 

Explicit getter, implicit setter.

property: fullScreenEffectState -> enum of {No-one, Someone Else, Us}
effect.animate(....., someFlagForFullScreen) 

best of both worlds.
Comment 4 Vlad Zahorodnii 2018-07-31 11:54:40 UTC
(In reply to David Edmundson from comment #3)
> Edit. I came to a decision on this. 
> 
> Explicit getter, implicit setter.
> 
> property: fullScreenEffectState -> enum of {No-one, Someone Else, Us}
> effect.animate(....., someFlagForFullScreen) 
> 
> best of both worlds.

+1

What about WindowForceBackgroundContrastRole and WindowForceBlurRole?

As far as I understand, effects have to force those roles and "unforce"
them in 

effect.fullScreenEffectStateChanged.connect(function () {
    // Check if we no longer full screen
    var windows = effects.stackingOrder();
    for (var i = 0; i < windows.length; ++i) {
        var w = windows[i];
        w.setData(WindowForceBackgroundContrastRole, null);
        w.setData(WindowForceBlurRole, null);
    }
});

right?
Comment 5 David Edmundson 2018-07-31 12:44:39 UTC
Something like that. 

We can add a shorthand for setting data on all windows if we need it.
Comment 6 David Edmundson 2018-10-03 21:57:58 UTC
Git commit 99b33e7428d14d9b24689e8cfb23092bf18820c8 by David Edmundson.
Committed on 03/10/2018 at 21:57.
Pushed by davidedmundson into branch 'master'.

[libkwineffects] Expose getting/setting activeFullScript to scripted effects

Summary:
Getter is exposed as a property on scripted effect in a way that hides
pointers from the scripting side.

Setter is implicitly handled as a property of newly created animations
and holds the activeFullScreenEffect whilst any of them are active. Like
existing effects it remains up to the effect author to avoid the
problems of multiple full screen effects. The RAII lock pattern is
somewhat overkill currently, but it's the direction I hope we can take
EffectsHandler in next API break.

--

This patch is against the QJSEngine port, though it's not conceptually a
requirement.

Test Plan: Unit test

Reviewers: #kwin, zzag

Reviewed By: #kwin, zzag

Subscribers: zzag, kwin

Tags: #kwin

Differential Revision: https://phabricator.kde.org/D14688

M  +70   -0    autotests/integration/effects/scripted_effects_test.cpp
A  +8    -0    autotests/integration/effects/scripts/fullScreenEffectTest.js
A  +22   -0    autotests/integration/effects/scripts/fullScreenEffectTestMulti.js
M  +3    -0    autotests/mock_effectshandler.h
M  +9    -0    effects.cpp
M  +1    -0    effects.h
M  +3    -1    libkwineffects/anidata.cpp
M  +2    -1    libkwineffects/anidata_p.h
M  +24   -4    libkwineffects/kwinanimationeffect.cpp
M  +21   -6    libkwineffects/kwinanimationeffect.h
M  +16   -0    libkwineffects/kwineffects.h
M  +32   -6    scripting/scriptedeffect.cpp
M  +10   -2    scripting/scriptedeffect.h

https://commits.kde.org/kwin/99b33e7428d14d9b24689e8cfb23092bf18820c8