Bug 1584

I don't have a 3.5.13.2 package set to test, but in 3.5.13.1 the Cancel button is disabled. In my 3.5.10 virtual machine, selecting the Cancel button does make the dialog disappear.

Fixed in commit ccb5fca.
I have opened bug 2038 for an improvement/clean up of the desktop lock code.

(In reply to Michele Calgaro from comment #2)
> Fixed in commit ccb5fca.
> I have opened bug 2038 for an improvement/clean up of the desktop lock code.

Does this patch enable the Cancel button when "Delay saver start after lock" is enabled?  If so then the patch introduced a new bug, if not we should be OK.

Ideally the cancel button would be completely hidden when "Delay saver start after lock" is enabled--the entire purpose of that feature is to detect when the machine has been completely idle without user input of any type.

(In reply to Timothy Pearson from comment #3)
Hi Tim, I did several tests while looking at this to make sure I had the fix right. I tested using "Start automatically" set to 1 minute.

Without the patch, the behavior is has follow:
1) with "Delay saver start after lock" disabled, after locking the screensaver starts immediately. The cancel button is enabled *BUT* it doesn't do anything. Esc as well doesn't do anything. So this is wrong in first place, the button should either work or be disabled. 
If the user do something and then stops, after about 10 seconds the screensaver restart automatically.
2) with "Delay saver start after lock" enabled, after locking the dialog is displayed. After the screensaver timeout has expired, the screensaver starts. The Cancel button is disabled and Esc does not do anything at all.
If the user do something and then stops, after the screensaver timeout has expired the screensaver restart automatically.

With the patch appied, the behavior is has follow:
1) with "Delay saver start after lock" disabled, after locking the screensaver starts immediately. The cancel button is enabled and it can be used to immediately start the screensaver (same for the Esc key).
If the user doesn't do anything, after about 10 seconds the screensaver starts on its own. If the user do something and then stops, after about 10 seconds the screensaver restart automatically.
2) with "Delay saver start after lock" enabled, after locking the dialog is displayed. After the screensaver timeout has expired, the screensaver starts. 
The Cancel button and Esc behaves as in 1).
If the user do something and then stops, after the screensaver timeout has expired the screensaver restart automatically.

Using the patch and the "Use legacy lock window" option, the behavior is as in option 1), with the difference that the lock dialog is smaller and has less info.


To me the behavior of the patch is consistent and I can't see a bug in it. 
If you are aware of a specific issue, feel free to reopen this bug and please add some comments on how to reproduce it.

In regards to 2) above with the patch applied, the behaviour should be that the Cancel button is removed and that Esc does nothing, but the screensaver will start after the specified timeout.  At least that's how I originally designed it and I do need that behaviour.  If another configuration option would be more obvious then we can do that instead of changing the current behaviour.

> At least that's how I originally designed it and I do need that behaviour.
> If another configuration option would be more obvious then we can do that 
> instead of changing the current behaviour.

Which one of the two options is the one that you need?
1) Do you need the screensaver to activate on its own if the user does nothing 
2) or do you need to "not have a Cancel button"? 
In the first case, it is already doing that, with or without the Cancel button.
In the second case we could add a config option to specifically hide the Cancel button if a user has such requirement. IMO such solution would be better compared to "simply disable the Cancel button".

(In reply to Michele Calgaro from comment #6)
> > At least that's how I originally designed it and I do need that behaviour.
> > If another configuration option would be more obvious then we can do that 
> > instead of changing the current behaviour.
> 
> Which one of the two options is the one that you need?
> 1) Do you need the screensaver to activate on its own if the user does
> nothing 
> 2) or do you need to "not have a Cancel button"? 
> In the first case, it is already doing that, with or without the Cancel
> button.
> In the second case we could add a config option to specifically hide the
> Cancel button if a user has such requirement. IMO such solution would be
> better compared to "simply disable the Cancel button".

Both, and as you said 1) is already in place. ;-)

Yes, we should add a config option (and corresponding GUI control in the desktop control module as well, something like "Delay saver start after user input").  Should I reopen this report to track this or will a fix be coming soon enough that I don't need to do that?

Thanks!

I have just reopened it, better to do things the right way :-)
I should be able to look at this tomorrow.
I will add a "Hide Cancel button" to the screensaver config page, just after the "Hide active windows from saver" checkbox. 
Such option will be independent from the "Delay saver start after lock" option, so it can be used in both cases. The default will be to show the Cancel button.

Let me know if there is something more to add.

Tim, I added an option to hide the Cancel button from the Desktop Session Lock dialog (commit 59ef38d) and at the same time disable the Esc button functionality.
It works well, but I think I run into the problem you mentioned. This is what I see (testing with autostart after 1 min and the Hide Cancel button option enabled):
1) with "delay saver..." enabled, if the user does nothing for 1 minute, the dialog does not disappear immediately, but instead it disappears some time later (about 40 seconds). In those 40 seconds, the process kdesktop_lock goes crazy and uses 75% of CPU. Once the dialog finally disappear, kdesktop_lock CPU % goes back to almost 0 and the screensaver appears.
If the users logins during that period of 40 seconds, the following happens:
a) the dialog closes
b) the screen saver starts
c) the user needs to login again
d) finally the user has access to his session

2) with "delay saver..." disabled, I see the same pattern but the times are shorter (about 10 seconds for kdesktop_lock to go crazy and 14 seconds for the dialog to finally disapper).

Is this the same problem you had tried to fix in the past?
Anyhow I will continue to look at this issue tomorrow, to find a complete fix. In the meanwhile, could you test the new code and let me know if you see anything else that requires action?

Actually on second thought, this may be caused by the new code, since it doesn't happen when the "Hide Cancel button" is unchecked.
Will spend more time on this later.

This should now be fixed (commit 710fc43).

Tim, before I mark the bug resolved, please test and let me know if there is anything else that still needs to be fixed.

(In reply to Michele Calgaro from comment #11)
> This should now be fixed (commit 710fc43).
> 
> Tim, before I mark the bug resolved, please test and let me know if there is
> anything else that still needs to be fixed.

In my kcontrol window the new option shows up as "&button", and "Cancel" should be lower case.

(In reply to Timothy Pearson from comment #12)
> (In reply to Michele Calgaro from comment #11)
> > This should now be fixed (commit 710fc43).
> > 
> > Tim, before I mark the bug resolved, please test and let me know if there is
> > anything else that still needs to be fixed.
> 
> In my kcontrol window the new option shows up as "&button", and "Cancel"
> should be lower case.

It also looks like enabling this option and the SAK causes kdesktop_lock to hang after entering the SAK sequence and before the login dialog appears.

(In reply to Timothy Pearson from comment #13)
> (In reply to Timothy Pearson from comment #12)
> > (In reply to Michele Calgaro from comment #11)
> > > This should now be fixed (commit 710fc43).
> > > 
> > > Tim, before I mark the bug resolved, please test and let me know if there is
> > > anything else that still needs to be fixed.
> > 
> > In my kcontrol window the new option shows up as "&button", and "Cancel"
> > should be lower case.
> 
> It also looks like enabling this option and the SAK causes kdesktop_lock to
> hang after entering the SAK sequence and before the login dialog appears.

Sorry for the bug spam; I need to also mention that the Cancel button should remain visible and enabled if the SAK is available for use.

> Sorry for the bug spam; I need to also mention that the Cancel button should 
> remain visible and enabled if the SAK is available for use.
No need for apologies, that's exactly the kind of information I need :-)
On my system I don't use SAK and that explains why I missed that bug with the login process. I will work more on this in the next few days, not sure I can do much before Monday though.

To summarize the required features:
1) with SAK disabled: 
   - current behavior is ok
2) with SAK enabled: 
   - cancel button needs to be visible and enabled
   - Esc should work as well
   - kdesktop_lock should not hang after entering the SAK sequence
Anything else to add to the list?

In the meanwhile, it may be a good idea to temporarily revert the commits I already pushed, so that at least when you build you have a working system.
Let me know, I can do that really quick.

(In reply to Michele Calgaro from comment #15)
> > Sorry for the bug spam; I need to also mention that the Cancel button should 
> > remain visible and enabled if the SAK is available for use.
> No need for apologies, that's exactly the kind of information I need :-)
> On my system I don't use SAK and that explains why I missed that bug with
> the login process. I will work more on this in the next few days, not sure I
> can do much before Monday though.
> 
> To summarize the required features:
> 1) with SAK disabled: 
>    - current behavior is ok

I haven't really checked, but I'll take your word for it.

> 2) with SAK enabled: 
>    - cancel button needs to be visible and enabled
>    - Esc should work as well
>    - kdesktop_lock should not hang after entering the SAK sequence
> Anything else to add to the list?

Nope, that's about it.  When SAK is enabled (and available!) Cancel/esc needs to be available so that the user can return to the SAK entry screen if desired to ensure security.

> In the meanwhile, it may be a good idea to temporarily revert the commits I
> already pushed, so that at least when you build you have a working system.
> Let me know, I can do that really quick.

Probably would be a good idea.  kdesktop_lock freezing is not a good thing.

Thanks!

Commits ccb5fca, 59ef38d, 710fc43 have been temporarily reverted, so that kdesktop code is the same as it was before.
Once I have a tested fix for SAK enabled systems, I will post a patch first, so you can test before I pushed it.

In case we use the legacy lock window, should we give the user the possibility to hide the cancel button? Or just leave that window untouched?

Actually, do we need the legacy lock window at all? The "new" TDE dialog has more info and looks better. I suggest we remove the legacy one and simplify the code. 
Tim, Slavek, what do you think?

(In reply to Michele Calgaro from comment #18)
> In case we use the legacy lock window, should we give the user the
> possibility to hide the cancel button? Or just leave that window untouched?

That window should remain untouched, see below.

> Actually, do we need the legacy lock window at all? The "new" TDE dialog has
> more info and looks better. I suggest we remove the legacy one and simplify
> the code. 
> Tim, Slavek, what do you think?

When I tried to replace the legacy window years ago I met with strong resistance, hence the franken-lock that combines both dialogs into a single program.  People had objected strongly to the Windows-like feel of the new dialog, technical merits notwithstanding.  Not wanting to be like certain other projects by alienating users I offered a choice with the configuration options and all was well. :-)

Tim

Created attachment 2050 [details]
tdebase patch

Tim, attached is a proposed patch that should work on both SAK enabled and disabled/not available systems.
I tested on a VM and looks good.
This are details of some tests I did:

1) Use legacy window ON
Cancel button visible, Esc working, legacy window presented

--> All next tests use legacy window OFF

2) Hide cancel=OFF, SAK=OFF
Cancel button visible, Esc working

3) Hide cancel=ON, SAK=OFF
Cancel button not visible, Esc not working

4) SAK=ON (regardless of hide cancel settings)
Cancel button visible, Esc working

--> All next tests are on a SAK-not-available system
5) Hide cancel=OFF
Cancel button visible, Esc working

6) Hide cancel=ON
Cancel button not visible, Esc not working

--------------------------
I tested with "Auto start ON and time=1 min" and both using "delay saver" and not.

Please test on your system and let me know if there is anything else that needs to be done/fixed. If all is ok, the patch is ready to be pushed to GIT (just need to merge a local branch to master).

Patch looks good in my tests; you may push when ready...

Thanks!

Pushed in commits a6fbc0f and 868c510.