Differential D13529

[minimizeAll] Do not try to restore window state on model change
AbandonedPublic
Actions

Authored by anthonyfieroni on Jun 14 2018, 11:26 AM.

Details

Reviewers
davidedmundson
Group Reviewers
Plasma
Summary

onDataChanged: some cases deactivate can not be called or persistent model index is invalidated (this should not happen in this case or it's a bug) which result in effect to stay active

BUG: 395519

Test Plan
  1. Click on applet
  2. Open new windows that rearrange task manager
  3. Try to unminimize window will result in unminimizing other

Diff Detail

Repository
R114 Plasma Addons
Lint
Lint Skipped
Unit
Unit Tests Skipped
Restricted Application added a project: Plasma. · View Herald TranscriptJun 14 2018, 11:26 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
anthonyfieroni requested review of this revision.Jun 14 2018, 11:26 AM
davidedmundson requested changes to this revision.Jun 14 2018, 11:28 AM

I'm happy to admit there's a bug there, but just removing the code isn't fixing it.

Now you're restoring windows as soon as any property, such as a download progressing in any window changes.

This revision now requires changes to proceed.Jun 14 2018, 11:28 AM
anthonyfieroni added a comment.EditedJun 14 2018, 11:41 AM

On previous version stackingOrderChanged will not restore windows state, it just stops the effect. So onActiveTaskChanged to replace onDataChanged?

hein added a subscriber: hein.Jun 15 2018, 8:51 AM

Looks OK to me, but want @davidedmundson to review.

davidedmundson added inline comments.Jun 18 2018, 7:41 AM
applets/minimizeall/package/contents/ui/main.qml
98

Oh!

The old c++ plugin had deactivate(bool) where the parameter indicated whether we restored the minimised windows or not. I had completely misread the old behaviour.

onVirtualDesktopChanged: deactivate()
onActivityChanged: deactivate()

should be doing what you're doing here not calling deactivate().

Can you change those, then ++++ to that specific change.
(BUG: 395519)

as for onDataChanged vs onActiveTaskChanged

The reason I did this was if you click minimise all -> then click on the desktop we deactivate, something I don't think we want.

I don't fully understand what you're saying is wrong with the onDataChanged.

anthonyfieroni edited the summary of this revision. (Show Details)Jun 18 2018, 8:16 AM

onVirtualDesktopChanged and onActivityChanged it looks good to deactivate effect with restoring windows state. Because you can miss or forgive that you activate it, we definitely NOT want to deactivate effect when you click desktop :) I think now it's looking good.

davidedmundson added a comment.Jun 18 2018, 8:28 AM

Because you can miss or forgive that you activate it, we definitely NOT want to deactivate effect when you click desktop :)

in this patch it DOES set active = false when you click on the desktop

That's why I used the onDataChanged

davidedmundson requested changes to this revision.Jun 19 2018, 12:08 AM
davidedmundson added inline comments.
applets/minimizeall/package/contents/ui/main.qml
98

this is deactivating the minimise all whenever the IsWindow role of any window changes? That seems wrong.

Though between the two patches we might have a good idea.

We could use onActiveTaskChanged if we then check that the activeTask isWindow is true.

This revision now requires changes to proceed.Jun 19 2018, 12:08 AM
anthonyfieroni abandoned this revision.Jun 19 2018, 8:09 AM
davidedmundson added a comment.Jun 19 2018, 8:12 AM

Sorry about taking over like that, just wanted to make sure your change got into 5.13.1

Revision Contents
Changeset List

Diff 36300

View Options

applets/minimizeall/package/contents/ui/main.qml

Loading...