[libkwineffects] Expose getting/setting activeFullScript to scripted effects
ClosedPublic

Authored by davidedmundson on Aug 8 2018, 10:10 AM.

Details

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.

BUG: 396790

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

Test Plan

Unit test

Diff Detail

Repository
R108 KWin
Branch
fullscreeneffect
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1619
Build 1637: arc lint + arc unit
davidedmundson created this revision.Aug 8 2018, 10:10 AM
Restricted Application added a project: KWin. · View Herald TranscriptAug 8 2018, 10:10 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Aug 8 2018, 10:10 AM
zzag added a subscriber: zzag.Aug 8 2018, 11:00 AM

I haven't ran tests yet but won't the slide effect and the test effect conflict?

libkwineffects/anidata_p.h
34

I think the line has become too long. Maybe, move FullScreenEffectLockPtr param to a new line?

53

Typo. (three l's)

libkwineffects/kwinanimationeffect.cpp
48–49

Looks like unrelated change.

scripting/scriptedeffect.cpp
99

I think it would be better to capitalize 'screen', so it's consistent with the rest of api:

  • activeFullScreenEffectChanged
  • fullScreenEffectState
319

That's unreachable code. We would hit this code if a compiler has bugs. :-)

Also, I think we don't need else because of proceeding return.

scripting/scriptedeffect.h
48

Please put a whitespace between // and text.

121

Nitpick: please add empty line between fullScreenEffectState() and public Q_SLOTS.

zzag added a comment.Aug 8 2018, 11:04 AM

Edit: OK, I see, effects are unloaded in cleanup. Still, maybe, you need to disable sldie effect because tests don't have to depend on execution order.

zzag added inline comments.Aug 8 2018, 11:19 AM
autotests/integration/effects/scripts/fullScreenEffectTest.js
6

Also, it would be great to pass an object because it's really hard to say what each number is, e.g.

effect.animate({
    window: w,
    type: Effect.Scale,
    ...,
    fullScreen: true
});
davidedmundson marked 6 inline comments as done.Aug 8 2018, 11:40 AM

I haven't ran tests yet but won't the slide effect and the test effect conflict?

We disable all effects in initTestCase

autotests/integration/effects/scripts/fullScreenEffectTest.js
6

Sure for production code, but the goal here is to test the public API.

I've added a second test with the unboxing version and I've added a comment here.

scripting/scriptedeffect.cpp
319

Sure, else or not compiles to the same thing.
(one jump statement that isn't reachable and gets compiled out)

IMHO this makes it easier to tell the next reader what's going on when you have valid either one or the other situations; but in this case it's only one line so I don't really care.

davidedmundson marked an inline comment as done.

Updates

zzag added a comment.Aug 8 2018, 12:10 PM

We disable all effects in initTestCase

Oh, silly me, I forgot about

const auto builtinNames = BuiltInEffects::availableEffectNames() << loader.listOfKnownEffects();
for (QString name : builtinNames) {
    plugins.writeEntry(name + QStringLiteral("Enabled"), false);
}

sorry.


One last nitpick: please rename AnimationSettings::Fullscreen to AnimationSettings::FullScreen and isFullscreen to isFullScreen(or just fullScreen).

zzag added inline comments.Aug 8 2018, 12:51 PM
autotests/integration/effects/scripts/fullScreenEffectTest.js
6

Looks like the second test can't be located:

testdata ./scripts/fullScreenEffectTestMulti.js could not be located!

Git add missing file

Literally the one time when I actually wanted arc to add the untracked files it didn't bother.

davidedmundson added inline comments.Aug 9 2018, 1:31 PM
libkwineffects/kwinanimationeffect.h
182–183

That other thread made me think maybe I should just do this now and bump the ABI. Will simplify the scripted effect code.

Opinions?

zzag added inline comments.Aug 9 2018, 2:07 PM
libkwineffects/kwinanimationeffect.h
182–183

Well, it's already bumped so 3rd-party effects have to be recompiled.

I have nothing against that. :-)

If we're going to break API, may as well go all in and keep everything clean

zzag accepted this revision.Aug 20 2018, 8:06 PM

Mostly, whitespace nitpicks.

autotests/integration/effects/scripted_effects_test.cpp
356

please delte extra space

362

extra space

394

extra space before (

libkwineffects/anidata.cpp
48–49

please delete whitespace before comma

libkwineffects/kwinanimationeffect.cpp
245

Can we use the create method? e.g.

fullScreenLock = FullScreenEffectLockPtr::create(this);
libkwineffects/kwinanimationeffect.h
99

Maybe, we have to delete copy constructor and copy assignment.

IIRC, QSharedPointer doesn't make copies of the pointed object so it should compile.

177

Capitalize "sets"

234

please dlete extra space before comma

scripting/scriptedeffect.cpp
99

Can be const.

This revision is now accepted and ready to land.Aug 20 2018, 8:06 PM

Given I know @zzag wants this fixed in the next release I've re-patched this to work on the QScriptEngine codebase so we can get it in.

However, I still really want to minimise the amount of other development work we do on a code base that we need to port/replace as it's overall very wasteful.

Rebase to older QScriptEngine

Tests are agnostic to the engine and so still pass

zzag added a comment.Sep 4 2018, 6:19 PM

Given I know @zzag wants this fixed in the next release I've re-patched this to work on the QScriptEngine codebase so we can get it in.

Well, I'm okay with having it landed in 5.15. ;-)

The only blocker is the issue with EffectWindowList.

However, I still really want to minimise the amount of other development work we do on a code base that we need to port/replace as it's overall very wasteful.

davidedmundson marked 13 inline comments as done.

whitespace

Swap the enum tristate for whether any effect is full screen or if we're the active full screen effect
with 2 booleans.

In hindsight it's easier for scripts to bind to the two of them as applicable as with the enum they need to
keep track of what changes are relevant. Best to keep scripting as simple as possible and do more complex logic
externally.

zzag added a comment.Sep 13 2018, 5:11 PM

Frankly, I prefer the old approach (with tristate). Now, we have two very similar properties.

Frankly, I prefer the old approach (with tristate). Now, we have two very similar properties.

I found doing the fadedesktop changes (and using the API) I needed quite a bit of boiler plate involving an ugly global var to turn it into anything usable.

a signal fullScreenEffectStateChanged(newState, oldState) would have sufficed, but then it doesn't nicely map to property notifications and it still leaves tedious logic in the JS code.

zzag added a comment.Sep 22 2018, 6:25 PM

I found doing the fadedesktop changes (and using the API) I needed quite a bit of boiler plate involving an ugly global var to turn it into anything usable.
a signal fullScreenEffectStateChanged(newState, oldState) would have sufficed, but then it doesn't nicely map to property notifications and it still leaves tedious logic in the JS code.

Can fullScreenEffectActive belong then to effects?

effects.fullScreenEffectActive : any effect is full screen
effect.fullScreenEffectActive : "local" to the effect, maybe the name is not the best

scripting/scriptedeffect.h
44

Leftover.

zzag added a comment.EditedSep 23 2018, 5:26 PM

FYI, I ported the Minimize Animation effect to JavaScript (zzag/port-minimizeanimation-to-js branch). Yet, there are several "obstacles":

  • checking whether there is a full screen effect;
  • reversing animations (easy to fix, but I think it will go only after ScriptedEffect is ported to QJSEngine).

Put tracking if there's a scripted effect active or not onto effectsHandler

Keeps API easier to read

zzag added a comment.EditedOct 2 2018, 6:42 PM

I like this one more.


As an alternative approach we could have the tristate and current signals, e.g.

effects.activeFullScreenChanged.conect(function () {
    // effect.activeFullScreenState can have the following values:
    //    - Effect.InactiveFullScreenEffect
    //    - Effect.OwnedFullScreenEffect
    //    - Effect.ActiveFullScreenEffect
});

effect.activeFullScreenChanged.connect(function () {
    // effect.activeFullScreenState can have the following values:
    //    - Effect.InactiveFullScreenEffect
    //    - Effect.ActiveFullScreenEffect
});

This way is cleaner to me(we don't have to add a new property to EffectsHandler). (I really like your tristate approach)

And of course, we would need to document the difference between those 2 signals.

Cons: effect.activeFullScreenState is not intuitive, but that's fine.

zzag accepted this revision.Oct 2 2018, 7:02 PM

In general, I'm okay with it.

Please bump API version in kwineffects.h

effects.cpp
626 ↗(On Diff #42743)

const bool

641 ↗(On Diff #42743)

Readability nitpick: fullscreen_effect != nullptr

scripting/scriptedeffect.h
44

Please delete this one.

45
/**
 *
 **/
127

It would be great to add @since but other fields don't have that...

144

Is it emitted anymore?

163

Please put whitespace before *.

This revision was automatically updated to reflect the committed changes.