[effects/trackmouse] Allow to use both modifiers and shortcut
ClosedPublic

Authored by zzag on Sep 4 2018, 4:22 PM.

Details

Summary

The Track Mouse effect can be toggled either by pressing modifier keys
and moving mouse or by pressing a shortcut. It's not possible to use
the latter and then the former without changing config.

But there is one caveat, in order to use shortcut, you have to uncheck
all modifier keys. This seems to be not very intuitive.

In addition to that, the KCM allows to change shortcut even if there is
some checked modifier.

As the title says, this change makes possible to use both modifier keys
and shortcut to activate this effect without changing config.

KCM:

BUG: 398124
FIXED-IN: 5.14.0

Diff Detail

Repository
R108 KWin
Branch
effects-trackmouse-shortcut
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2516
Build 2534: arc lint + arc unit
zzag created this revision.Sep 4 2018, 4:22 PM
Restricted Application added a project: KWin. · View Herald TranscriptSep 4 2018, 4:22 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Sep 4 2018, 4:22 PM
zzag edited the summary of this revision. (Show Details)Sep 4 2018, 4:23 PM
zzag edited the summary of this revision. (Show Details)Sep 4 2018, 6:48 PM
ngraham added a subscriber: ngraham.Sep 5 2018, 3:05 AM

I feel like we need a hearer of some sort to make it clear what this is actually doing. The old one had "Trigger on:"; could we maybe just keep that? Or maybe "Trigger effect with:"

zzag updated this revision to Diff 41024.Sep 5 2018, 7:47 AM

Keep "Trigger on" header

zzag edited the summary of this revision. (Show Details)Sep 5 2018, 7:48 AM

Thanks Vlad! Could we make the title left-aligned and add a colon to the end of it?

zzag added a comment.EditedSep 5 2018, 3:39 PM

and add a colon to the end of it?

Well, that's easy.

make the title left-aligned

... and that's not easy.

It looks like KTitleWidget has some margin on the left by default and there is no way to tweak that.

Would it be okay to use Label instead? e.g.

In D15272#320913, @zzag wrote:

and add a colon to the end of it?

Well, that's easy.

make the title left-aligned

... and that's not easy.

It looks like KTitleWidget has some margin on the left by default and there is no way to tweak that.

That's a shame. Might be worth fixing.

Would it be okay to use Label instead? e.g.

Since in this case, we really are using it more as a section header than a title, I think this is probably okay.

Now that I stare at that label, I wonder if we should use a slightly different string though. Like:

"Trigger effect with:"
"Effect triggers" (no colon)

Thoughts?

abetts added a subscriber: abetts.Sep 5 2018, 4:30 PM
In D15272#320913, @zzag wrote:

and add a colon to the end of it?

Well, that's easy.

make the title left-aligned

... and that's not easy.

It looks like KTitleWidget has some margin on the left by default and there is no way to tweak that.

That's a shame. Might be worth fixing.

Would it be okay to use Label instead? e.g.

Since in this case, we really are using it more as a section header than a title, I think this is probably okay.

Now that I stare at that label, I wonder if we should use a slightly different string though. Like:

"Trigger effect with:"
"Effect triggers" (no colon)

Thoughts?

I like those strings you proposed. They seem more approachable. Trigger effect "with" (using) is my favorite.

zzag updated this revision to Diff 41064.Sep 5 2018, 4:45 PM

"Trigger on" => "Trigger effect with:"

zzag edited the summary of this revision. (Show Details)Sep 5 2018, 4:46 PM
ngraham accepted this revision as: VDG.Sep 5 2018, 4:56 PM

Looks great UI-wise! Thanks for your patience here, Vlad.

zzag edited the summary of this revision. (Show Details)Sep 7 2018, 8:47 AM

It looks like KTitleWidget has some margin on the left by default and there is no way to tweak that.

KTitleWidget normally also has a frame but e.g. Breeze widget style overrides it to not have one

davidedmundson accepted this revision.Sep 12 2018, 12:24 PM
This revision is now accepted and ready to land.Sep 12 2018, 12:24 PM
effects/trackmouse/trackmouse.cpp
220

One comment I forgot to save.

There's an inbalance here:

If you activate by shortcut, then pressing a modifier on and off won't disable it
But if you activate by modifier then toggling the shortcut will disable it

This might become an issue if a user has it bound to both meta modifier and shortcut meta+a and pressed whilst moving the mouse.

They would hit meta, activate it by modifier, press the shortcut which then turns it off, release the keys and then it would stay off even though they hit the shortcut once.

/maybe/ we actually want

case State::ActivatedByModifiers:

m_state = ActivatedByShortcut
break;

but it's an extreme edge case

zzag added inline comments.Sep 12 2018, 12:46 PM
effects/trackmouse/trackmouse.cpp
220

If you activate by shortcut, then pressing a modifier on and off won't disable it. But if you activate by modifier then toggling the shortcut will disable it

That's totally intentional.

They would hit meta, activate it by modifier, press the shortcut which then turns it off, release the keys and then it would stay off even though they hit the shortcut once.

mouseChanged is emitted when, for example, mouse has been moved. If you press a modifier key, mouseChanged is not emitted. So, that's fine.

Also, this leads to a bug in some sense, e.g. if you stop moving mouse and then release modifiers, the effect will be still active even though modifiers are not pressed anymore.

zzag added inline comments.Sep 12 2018, 12:49 PM
effects/trackmouse/trackmouse.cpp
220

OK, that's bs. On Wayland, I suppose things are a little bit different.

zzag updated this revision to Diff 41472.Sep 12 2018, 1:17 PM

Fix on Wayland

(maybe it makes sense to bark at user if the modifiers serve as a prefix of the keyboard shortcut)

zzag updated this revision to Diff 41474.Sep 12 2018, 1:19 PM

Don't fall through

davidedmundson accepted this revision.Sep 12 2018, 1:30 PM
This revision was automatically updated to reflect the committed changes.