Differential D16219

[Lock Screen] Do not try to unlock when unvisible
ClosedPublic
Actions

Authored by thsurrel on Oct 15 2018, 10:23 AM.

Details

Reviewers
ngraham
davidedmundson
Group Reviewers
Plasma
VDG
Commits
R120:754efdedd4ee: [Lock Screen] Do not try to unlock when unvisible
Summary

In the lock screen, pressing the enter key while the UI is not yet
visible ends up trying to unlock (and would usually fail because the
password field is empty).
Ensure that the UI is visible before unlocking.

BUG: 395671

Test Plan

Lock the session, press enter. The UI should appear without any
"Unlocking failed" message.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
thsurrel created this revision.Oct 15 2018, 10:23 AM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 15 2018, 10:23 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
thsurrel requested review of this revision.Oct 15 2018, 10:23 AM
filipf added a subscriber: filipf.EditedOct 15 2018, 11:28 AM

The new lock screen runs the risk of adding extra steps for the user. This is one change that can help the issue because IIRC in Windows you also have a double lock screen and pressing Enter will get you to the password prompt; I assume the users who have had experience with Windows would expect Plasma to do the same, hence the bug reports. I would also suggest to add something like a "Type to unlock..." text to the unblurred screen because it's not intuitive that the user can just start typing their password, and they should be saved from having to move their mouse etc.

EDIT: Submitted the latter as a feature request.

thsurrel added a comment.Oct 15 2018, 11:40 AM
In D16219#343399, @filipf wrote:

The new lock screen runs the risk of adding extra steps for the user. This is one change that can help the issue because IIRC in Windows you also have a double lock screen and pressing Enter will get you to the password prompt; I assume the users who have had experience with Windows would expect Plasma to do the same, hence the bug reports. I would also suggest to add something like a "Type to unlock..." text to the unblurred screen because it's not intuitive that the user can just start typing their password, and they should be saved from having to move their mouse etc.

Could you create a feature request in https://bugs.kde.org/ about the "Type to unlock..." label ? That should be done in a separate patch, I believe.

abetts added a subscriber: abetts.Oct 15 2018, 2:24 PM

Could you please add a short video or gif showing this problem?

davidedmundson added a subscriber: davidedmundson.Oct 15 2018, 2:27 PM

There's one in the bug report.

abetts added a comment.Oct 15 2018, 2:29 PM
In D16219#343507, @davidedmundson wrote:

There's one in the bug report.

If this is the bug referenced

https://bugs.kde.org/show_bug.cgi?id=399835

There is no video there

ngraham accepted this revision.Oct 17 2018, 3:34 AM
ngraham added a subscriber: ngraham.

Very clever way of solving the problem! I took a look myself a few days ago but got stuck. I like your solution. It works well and I couldn't find any regressions.

Please get a second opinion from a Plasma person before landing. Also, this could probably be considered a bugfix and landed on the stable branch.

This revision is now accepted and ready to land.Oct 17 2018, 3:34 AM
davidedmundson added a comment.Oct 17 2018, 11:55 AM

It's ok as-is, there's a code change that would make things a bit better.

lookandfeel/contents/lockscreen/MainBlock.qml
66

It's generally bad practice to reference properties in the parent scope, it makes future refactors very difficult.

I would prefer an explicit property on MainBlock and then have LockScreenUi.qml bind that when it instantiates the MainBlock.

thsurrel updated this revision to Diff 43792.Oct 17 2018, 2:07 PM

Do not access parent property directly
Thanks for the comment davidedmundson

abetts added a comment.Oct 17 2018, 2:29 PM

+1 for the idea!

davidedmundson accepted this revision.Oct 17 2018, 2:38 PM
ngraham added a comment.Oct 17 2018, 2:54 PM

Let's land this on the Plasma/5.14 branch and then merge to master. See https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

Closed by commit R120:754efdedd4ee: [Lock Screen] Do not try to unlock when unvisible (authored by thsurrel). · Explain WhyOct 17 2018, 8:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 43824

View Options

lookandfeel/contents/lockscreen/LockScreenUi.qml

Loading...
View Options

lookandfeel/contents/lockscreen/MainBlock.qml

Loading...