[scripting] Allow effects to grab windows
ClosedPublic

Authored by zzag on May 27 2018, 4:19 PM.

Details

Summary

Some JavaScript based effects need to grab particular windows in order
to avoid conflicts with other effects.

Example usage:

effects.windowAdded.connect(function (window) {
    if (effect.grab(window, Effect.WindowAddedGrabRole)) {
        window.coolWindowTypeAnimation = animate({
            ...
        });
    }
});

Diff Detail

Repository
R108 KWin
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4262
Build 4280: arc lint + arc unit
zzag created this revision.May 27 2018, 4:19 PM
Restricted Application added a project: KWin. · View Herald TranscriptMay 27 2018, 4:19 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.May 27 2018, 4:19 PM
zzag planned changes to this revision.May 27 2018, 4:23 PM
zzag added a reviewer: KWin.May 27 2018, 4:25 PM
davidedmundson added inline comments.
scripting/scriptedeffect.cpp
682

is this line the right way round?

What about moving this directly to Effect or AnimationEffect?

zzag added inline comments.May 27 2018, 4:42 PM
scripting/scriptedeffect.cpp
682

Is it okay to grab windows unconditionally? I was thinking about force argument or something.

zzag added a comment.EditedMay 27 2018, 4:47 PM

What about moving this directly to Effect or AnimationEffect?

Yeah, some effects(like Glide or Fall apart) may "benefit" from having this functionally up in the Effect class.

I'll move it to Effect.

zzag updated this revision to Diff 35023.May 28 2018, 1:17 PM

Move isGrabbed, grab, and ungrab to Effect.

zzag planned changes to this revision.May 28 2018, 1:17 PM
zzag retitled this revision from WIP: [scripting] Allow effects to grab windows to WIP: [libkwineffects] Allow effects to grab windows.
zzag added a comment.EditedMay 29 2018, 5:39 PM

@graesslin Should I add tests? If so, where should I put them? In libkwineffects/(as Unit tests) or integration/(as Integration tests)?

romangg added a subscriber: romangg.Jun 1 2018, 3:52 PM
In D13153#270440, @zzag wrote:

@graesslin Should I add tests? If so, where should I put them? In libkwineffects/(as Unit tests) or integration/(as Integration tests)?

At first look I would have said libkwineffects.

But could you explain why there was already an isGrabbed function in sciprting/scriptedeffect.cpp? Where and how was the property, which gets checked by that function, set in the past?

zzag added a comment.Jun 1 2018, 4:12 PM

But could you explain why there was already an isGrabbed function in sciprting/scriptedeffect.cpp?

Without that method that's not possible for JavaScript effects to check whether a given window has been grabbed by somebody else.

E.g. Glide effect grabs windows with WindowAdded and WindowClosed grab roles so Fade effect won't animate them.

Where and how was the property, which gets checked by that function, set in the past?

Grab roles are currently set only by C++ effects(Glide, Sheet, etc). JavaScript effects can't grab windows right now, which is really big problem.

zzag updated this revision to Diff 35359.Jun 1 2018, 4:38 PM

Simplify isGrabbed

davidedmundson accepted this revision.Jun 17 2018, 5:13 PM
This revision is now accepted and ready to land.Jun 17 2018, 5:13 PM
zzag added a comment.Jun 17 2018, 6:02 PM

Oh, I forgot about this diff. I'm not sure whether that's a good diff to land.

(a) I don't know whether that's a right way to do in JavaScript, e.g.

if (effect.grab(w, Effect.WindowAddedGrabRole)) {
}

shouldn't it be

animate({
    // ...
    grab: [
        Effect.WindowAddedGrabRole
    ],
    // ...
})

(b) isGrabbed is not intuitive. So, I don't want to introduce a mess to C++ effects API and add more confusion to C++ effects.

Yet, I think it would be great to add window grabbing code to Effect. For example, I see quite a lot

auto fooGrab = w->data(WindowFooGrabRole).value<void*>();
if (fooGrab && fooGrab != this) {
    return;
}
w->setData(WindowFooGrabRole, QVariant::fromValue(static_cast<void*>(this)));

in different effects, so we could move this up, e.g.

bool canGrab(EffectWindow *w, int role);
void grab(EffectWindow *w, int role);
bool tryGrab(EffectWindow *w, int role);

bool canUngrab(EffectWindow *w, int role);
void ungrab(EffectWindow *w, int role);
bool tryUngrab(EffectWindow *w, int role);

// That's only my thoughts. That's not final api. I still need to think about it.

Example usage:

void NiceEffect::windowAdded(EffectWindow *w) {
    if (effects->activeFullScreenEffect()) {
        return;
    }

    const bool grabbed = tryGrab(w, WindowAddedGrabRole);
    if (!grabbed) {
        return;
    }

    // Setup timeline, etc.
}

Also, I've just noticed that I added a bug in isGrabbed.

zzag added a comment.Jun 17 2018, 6:21 PM

@davidedmundson If you don't mind, I would like to abandon this diff.

@davidedmundson If you don't mind, I would like to abandon this diff.

Fair enough. It's kinda sad, given we won't ever fix scripts if we just end up going for the C++ version every time.

isGrabbed is not intuitive

I agree, but it's frustratingly the one part that we can't change!

zzag abandoned this revision.Jul 16 2018, 4:15 PM
zzag reclaimed this revision.Oct 26 2018, 8:52 AM

We still need to support window grabbing in scripted effects api.

I think grab/ungrab functions are "the right" way because in most cases we'd like to have exclusive "control" over open/close animations (that's okay to hold grab after the animation has ended).

Can we move grab/ungrab somewhere up (AnimationEffect or Effect)? I don't think so, at least right now (because of isGrabbed).

This revision is now accepted and ready to land.Oct 26 2018, 8:52 AM
zzag planned changes to this revision.Oct 26 2018, 8:52 AM

Add tests + flag to ignore previous grab.

zzag added a comment.Oct 26 2018, 8:58 AM

I don't think so, at least right now (because of isGrabbed).

Or maybe we could extend isGrabbed:

enum class GrabbedBy {
    Me,
    Other
};

bool isGrabbed(EffectWindow *w, DataRole role, GrabbedBy by = GrabbedBy::Other);

just a thought

zzag updated this revision to Diff 44344.Oct 28 2018, 10:52 AM
  • Add tests;
  • Use force flag only in grab(): Allow effects to steal window grabs, but only the owner of window grab can ungrab the window. The reason for that is scripted effects won't notice that they are no longer hold window grab because of how isGrabbed works, so instead one has to "steal" window grab and then call ungrab().
This revision is now accepted and ready to land.Oct 28 2018, 10:52 AM
zzag retitled this revision from WIP: [libkwineffects] Allow effects to grab windows to [scripting] Allow effects to grab windows.Oct 28 2018, 10:54 AM

Or maybe we could extend isGrabbed:

If a user does need fine control, they do have the option of calling EffectWindow::data() and doing comparisons with undefined or effect manually.

This revision was automatically updated to reflect the committed changes.