Differential D2325

Don't unconditionally emit buttonPressed on profile load
AbandonedPublic
Actions

Authored by broulik on Aug 1 2016, 2:41 PM.

Details

Reviewers
davidedmundson
afiestas
Group Reviewers
Plasma
Commits
R122:48c3dca61d7c: Don't unconditionally emit buttonPressed on profile load
Summary

A button press (technically the "lid closed" event is also a button press) should always be some consicious action triggered by the user. By emitting this signal unconditionally on profile load (ie. whenever changing an activity or (un)plugging AC) the session might always lock or suspend when this happens.

If and only if a profile would really like to take some action or set some state depending on the lid state on profile load - which none do, the handle button press action should only react on explicit button presses anyway - it can still ask the backend for isLidClosed in its onProfileLoad method.

BUG: 366125
FIXED-IN: 5.7.4

Test Plan

Closed the lid, still suspends. I can't find any evidence about the reason this was put in there in the first place.

Diff Detail

Repository
R122 Powerdevil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik updated this revision to Diff 5603.Aug 1 2016, 2:41 PM
broulik retitled this revision from to Don't unconditionally emit buttonPressed on profile load.
broulik updated this object.
broulik edited the test plan for this revision. (Show Details)
broulik added reviewers: Plasma, afiestas.
broulik set the repository for this revision to R122 Powerdevil.
Restricted Application added a project: Plasma. · View Herald TranscriptAug 1 2016, 2:41 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson accepted this revision.Aug 9 2016, 8:45 AM
davidedmundson added a reviewer: davidedmundson.
davidedmundson added a subscriber: davidedmundson.

This is like a tutorial in how to make useless code comments.

This revision is now accepted and ready to land.Aug 9 2016, 8:45 AM
Closed by commit R122:48c3dca61d7c: Don't unconditionally emit buttonPressed on profile load (authored by broulik). · Explain WhyAug 10 2016, 7:36 AM
This revision was automatically updated to reflect the committed changes.
oliverhenshaw reopened this revision.Aug 10 2016, 11:18 AM
oliverhenshaw added a subscriber: oliverhenshaw.

I can't find any evidence about the reason this was put in there in the first place.

'git blame' [1] shows that this was added for https://bugs.kde.org/show_bug.cgi?id=243618 - it should still work as intended as long as the screen detection works properly.
The idea was that switching to a profile and then closing the lid should have the same result as closing the lid and then switching to a profile. Any exceptions are dealt with as inhibitions and will/should work exactly the same in the second case as in the first. I would suggest reverting this and perhaps updating the code comment.

Or perhaps there's a better way to handle bug 243618 now?

[1] with https://community.kde.org/Frameworks/GitOldHistory to graft in the old history.

This revision is now accepted and ready to land.Aug 10 2016, 11:18 AM
broulik added a comment.Aug 10 2016, 11:28 AM

I see. Thanks for your investigation. I must say, though, that I would not expect my laptop to do anything because the lid is closed after I closed the lid.

Since the bug was originally caused by broken monitor detection, which is fixed in 5.7, indeed could be reverted as to not cause regressions in a patch release.

broulik abandoned this revision.Sep 12 2016, 1:05 PM

Revision Contents
Changeset List

PathPackages
Mdaemon/powerdevilcore.cpp (5 lines)

Diff 5782

View Options

daemon/powerdevilcore.cpp

Loading...