Fix crash when invoking Present Windows with the group dialog open.
ClosedPublic

Authored by hein on Feb 9 2017, 1:36 PM.

Details

Summary

This fixes a regression from aeec4ae8 which unsets the taskManagerItem
prop when the group dialog is shown for unclear reasons.

Anthony, can you comment on why you added this? It seems to be
unnecessary in my testing.

CCMAIL:bvbfan@abv.bg
BUG:376205

Diff Detail

Repository
R119 Plasma Desktop
Branch
Plasma/5.9
Lint
No Linters Available
Unit
No Unit Test Coverage
hein updated this revision to Diff 11111.Feb 9 2017, 1:36 PM
hein retitled this revision from to Fix crash when invoking Present Windows with the group dialog open..
hein updated this object.
hein edited the test plan for this revision. (Show Details)
hein added reviewers: Plasma, anthonyfieroni.
hein added a subscriber: plasma-devel.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 9 2017, 1:36 PM
anthonyfieroni edited edge metadata.Feb 9 2017, 2:35 PM

This is needed

  1. Enable highlight windows
  2. Hover group item
  3. Left click on effect is stopped
  4. Move mouse over window list
  5. Ugly highlight window effect

So add a guard

if (m_taskManagerItem) {
    KWindowEffects::presentWindows(m_taskManagerItem->window()->winId(), winIds);
}
hein added a comment.Feb 10 2017, 9:01 AM

No, unsetting taskManagerItem is dumb code IMHO. I'll address your concern a different way. Thanks for the input.

You can try in Task.qml

onHighlightedChanged: {
    if (highlighted) {
        backend.cancelHighlightWindows();
    }
}

If it can help :)

hein added a comment.Feb 10 2017, 10:41 AM

I feel like both of those are papering over the real bug, which that we never figured out how to exempt the GroupDialog successfully from the highlight effect. This just breaks highlight for group members ...

hein updated this revision to Diff 11153.Feb 10 2017, 11:03 AM
hein edited edge metadata.
  • Do highlighting from the group dialog properly instead of intentionally breaking it via a side effect that caused this crash.
  • Some extra crash guards.
hein updated this revision to Diff 11154.Feb 10 2017, 11:06 AM

Drop stray debug.

hein updated this revision to Diff 11160.Feb 10 2017, 11:42 AM

Drop another stray debug.

anthonyfieroni accepted this revision.Feb 10 2017, 5:30 PM
anthonyfieroni edited edge metadata.
This revision is now accepted and ready to land.Feb 10 2017, 5:30 PM
anthonyfieroni requested changes to this revision.Feb 10 2017, 5:31 PM
anthonyfieroni edited edge metadata.

One more change, wait for me :)

This revision now requires changes to proceed.Feb 10 2017, 5:31 PM
anthonyfieroni added a comment.EditedFeb 10 2017, 5:39 PM

Task.qml line 145 add
backend.cancelHighlightWindows();
So when it's pressed button (left, middle, right) on task in window list, it should always stop highlight effect :)

hein added a comment.Feb 13 2017, 10:28 AM

Yeah, makes sense since TooltipInstance does the same.

hein updated this revision to Diff 11276.Feb 13 2017, 10:28 AM
hein edited edge metadata.

Cancel window highlight on any click.

hein updated this revision to Diff 11277.Feb 13 2017, 10:43 AM
hein edited edge metadata.

Add bug 376234 to commit msg.

anthonyfieroni accepted this revision.Feb 13 2017, 11:19 AM
anthonyfieroni edited edge metadata.
This revision is now accepted and ready to land.Feb 13 2017, 11:19 AM
This revision was automatically updated to reflect the committed changes.

For LTS 5.8 some guard or backporting it?

hein added a comment.Feb 13 2017, 5:24 PM

You broke the code in 5.9, so I put it into 5.9.

Originally, i push it for 5.8 :)

hein added a comment.Feb 13 2017, 6:04 PM

Hmm really ... thank you, I backported it now.