Re: Review Request 113969: Do not assume every items have the same height
On April 6, 2014, 7:48 a.m., David Faure wrote: kdeui/itemviews/kcategorizedview.cpp, line 795 https://git.reviewboard.kde.org/r/113969/diff/4/?file=215105#file215105line795 Coding style comment: some code paths use break (which results in return QModelIndex() at the end) and some other code paths (e.g. line 776) do return QModelIndex() directly. Any reason for the inconsistency? The break at this line is not inconsistent with the `return QModelIndex()` at line 776 (line 764 in the new patch) because that line is inside another for loop. It is indeed not consistent with the old code at line 750 (line 738 in the new patch) which I didn't noticed before. I've changed this in the new patch to be consistent with the old style. - Yichao --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113969/#review55062 --- On April 22, 2014, 6:13 a.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113969/ --- (Updated April 22, 2014, 6:13 a.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs - kdeui/itemviews/kcategorizedview.cpp be811aa Diff: https://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113969/ --- (Updated April 22, 2014, 6:13 a.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs (updated) - kdeui/itemviews/kcategorizedview.cpp be811aa Diff: https://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
On March 26, 2014, 11:07 p.m., Thomas Lübking wrote:
you could sighup or sigusr the greeter process and have it
setImmediateLock(true);
desktopResized();
in return
Wolfgang Bauer wrote:
I agree, this would be a bit nicer...
But I tried it and cannot get it to work.
My signal handler is called, and both setImmediateLock(true) and
desktopResized() are called, (I verified this with debug output statements)
but the password dialog still doesn't show.
Wolfgang Bauer wrote:
Oops, sorry. I just noticed that the signal handler is apparently only
called when I run kscreenlocker_greet manually (and do kill -USR1), but not
when the locker runs it.
Will have to investigate this.
Wolfgang Bauer wrote:
Forget my previous comment.
The signal handler was actually called all the time.
But setImmediateLock(true); followed by desktopResized(); DOES NOT
have any (visual) effect.
I have yet to find out what else to call to make that dialog appear. Any
idea maybe?
Calling exit(1) does work, but that's not much different to killing the
greeter I suppose... ;-)
Thomas Lübking wrote:
Ahhh... me was too smart in the multiscreen code ;-)
the for loop in desktopResized() (which is relevant for the
m_immediateLock handling) won't be entered since no screen changed.
Instead you'd run
for (int i = 0; i desktop()-screenCount(); ++i) {
QDeclarativeProperty(m_views.at(i)-rootObject(),
locked).write(true);
}
aside fixing the m_immediateLock value.
Sorry for the false information.
Wolfgang Bauer wrote:
Ok, that works.
Thank you for the help! :-)
Btw, I noticed that UnlockApp::setLockedPropertyOnViews() basically does
the same, so I'm just calling that instead to avoid code duplication.
I have uploaded a new patch.
Christoph Feck wrote:
What is the status of this patch? Do we have someone (besides yourself),
who has enough knowledge of the screen locker to approve it?
Well, this patch fixes the issue on all systems I tried (which is only 2).
It is also part of openSUSE's KDE 4.12/4.13 (actually
kdebase4-workspace-4.11.8) packages since over a week.
I think this is a grave bug, and really would like to see this fixed in 4.11.9.
Added plasma now to the group of reviewers, maybe somebody there is willing to
approve it...
- Wolfgang
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117091/#review54247
---
On April 22, 2014, 6:41 p.m., Wolfgang Bauer wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117091/
---
(Updated April 22, 2014, 6:41 p.m.)
Review request for kde-workspace, Plasma and Aaron J. Seigo.
Bugs: 327947 and 329076
http://bugs.kde.org/show_bug.cgi?id=327947
http://bugs.kde.org/show_bug.cgi?id=329076
Repository: kde-workspace
Description
---
If the screen locker is set to not require a password to unlock, it will not
show the password input field even when the powermanagement settings suspend
the system and are set to require a password after resume (when it was
already running at that point).
This locks people out of their system.
This patch adds a signal handler for SIGUSR1 that switches the running
greeter to immediateLock mode. The locker sends that signal to make sure the
greeter shows the password input field when necessary.
Diffs
-
ksmserver/screenlocker/greeter/greeterapp.h 8b79188
ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85
ksmserver/screenlocker/greeter/main.cpp d898734
ksmserver/screenlocker/ksldapp.cpp 3dfcc9e
Diff: https://git.reviewboard.kde.org/r/117091/diff/
Testing
---
Disable Require password after in the screen locker settings (the default),
set it to start after 1 min. (for easier testing).
Enable Suspend session after and set it to 2 minutes. (set the action to
Suspend, Hibernate, or Lock Screen, doesn't matter)
Make sure Lock screen on resume is enabled in the powermanagements
Advanced Options (it is by default).
After 1 minute the screen locker kicks in, and doesn't require a password.
After 2 minutes the session gets suspended, hibernated or locked, and
requires a password to resume.
Without this patch no password dialog is shown, the user cannot resume the
session by entering the password.
With this patch this works: there is a password input field, the session is
unlocked when the user enters the password.
Thanks,
Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56190 --- see minor nitpicks, but in general the patch seems fine and required to me. shipIt! unless veto ;-) ksmserver/screenlocker/greeter/greeterapp.h https://git.reviewboard.kde.org/r/117091/#comment39254 lockImmediatedly()? ksmserver/screenlocker/greeter/main.cpp https://git.reviewboard.kde.org/r/117091/#comment39257 camelCase - signalHandler ksmserver/screenlocker/greeter/main.cpp https://git.reviewboard.kde.org/r/117091/#comment39255 it's nearly academical, but m_instance should be defaulted to NULL and tested here ksmserver/screenlocker/greeter/main.cpp https://git.reviewboard.kde.org/r/117091/#comment39256 ooc.: what's wrong about just: signal(SIGUSR1, signalHandler); ? - Thomas Lübking On April 22, 2014, 4:41 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 4:41 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
On April 22, 2014, 7:34 p.m., Thomas Lübking wrote: ksmserver/screenlocker/greeter/main.cpp, line 89 https://git.reviewboard.kde.org/r/117091/diff/2/?file=262501#file262501line89 ooc.: what's wrong about just: signal(SIGUSR1, signalHandler); ? signal() is deprecated, according to its manpage. man signal even says: The behavior of signal() varies across UNIX versions, and has also var- ied historically across different versions of Linux. Avoid its use: use sigaction(2) instead. See Portability below. On April 22, 2014, 7:34 p.m., Thomas Lübking wrote: ksmserver/screenlocker/greeter/main.cpp, line 38 https://git.reviewboard.kde.org/r/117091/diff/2/?file=262501#file262501line38 it's nearly academical, but m_instance should be defaulted to NULL and tested here Yeah, it can't really happen that m_instance isn't set, but anyway. ;) Before somebody is asking, I'm going to use the signal handler for another bugfix as well. (bug#224200) That's why I'm doing the check this way. - Wolfgang --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56190 --- On April 22, 2014, 9:28 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 9:28 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
On April 22, 2014, 5:34 p.m., Thomas Lübking wrote: ksmserver/screenlocker/greeter/main.cpp, line 89 https://git.reviewboard.kde.org/r/117091/diff/2/?file=262501#file262501line89 ooc.: what's wrong about just: signal(SIGUSR1, signalHandler); ? Wolfgang Bauer wrote: signal() is deprecated, according to its manpage. man signal even says: The behavior of signal() varies across UNIX versions, and has also var- ied historically across different versions of Linux. Avoid its use: use sigaction(2) instead. See Portability below. the posix manpage does of course not state that ... luckily it wraps around sigaction on BSD defaults on modern glibc systems - pfeew ;-) sigaction(SIGUSR1, sa, NULL); // for later nullptr i doubt that you'll need another update for that, though. - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56190 --- On April 22, 2014, 7:28 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 7:28 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56209 --- ksmserver/screenlocker/greeter/main.cpp https://git.reviewboard.kde.org/r/117091/#comment39263 /me would not insist on newline, but there needs to be a blank after the if if (!m_instance) return; - Thomas Lübking On April 22, 2014, 7:28 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 7:28 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 9:56 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Changes --- Added a space and a newline. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs (updated) - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
On April 22, 2014, 9:48 p.m., Thomas Lübking wrote: ksmserver/screenlocker/greeter/main.cpp, line 38 https://git.reviewboard.kde.org/r/117091/diff/3/?file=267733#file267733line38 /me would not insist on newline, but there needs to be a blank after the if if (!m_instance) return; OK, I added a newline as well, just to be sure... ;) - Wolfgang --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56209 --- On April 22, 2014, 9:56 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 9:56 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
On April 22, 2014, 7:34 p.m., Thomas Lübking wrote: ksmserver/screenlocker/greeter/main.cpp, line 89 https://git.reviewboard.kde.org/r/117091/diff/2/?file=262501#file262501line89 ooc.: what's wrong about just: signal(SIGUSR1, signalHandler); ? Wolfgang Bauer wrote: signal() is deprecated, according to its manpage. man signal even says: The behavior of signal() varies across UNIX versions, and has also var- ied historically across different versions of Linux. Avoid its use: use sigaction(2) instead. See Portability below. Thomas Lübking wrote: the posix manpage does of course not state that ... luckily it wraps around sigaction on BSD defaults on modern glibc systems - pfeew ;-) sigaction(SIGUSR1, sa, NULL); // for later nullptr i doubt that you'll need another update for that, though. Well, should I change it? In kscreensaver/libkscreensaver/main.cpp they used sigaction() as well. - Wolfgang --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56190 --- On April 22, 2014, 9:56 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 9:56 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
On April 22, 2014, 5:34 p.m., Thomas Lübking wrote: ksmserver/screenlocker/greeter/main.cpp, line 89 https://git.reviewboard.kde.org/r/117091/diff/2/?file=262501#file262501line89 ooc.: what's wrong about just: signal(SIGUSR1, signalHandler); ? Wolfgang Bauer wrote: signal() is deprecated, according to its manpage. man signal even says: The behavior of signal() varies across UNIX versions, and has also var- ied historically across different versions of Linux. Avoid its use: use sigaction(2) instead. See Portability below. Thomas Lübking wrote: the posix manpage does of course not state that ... luckily it wraps around sigaction on BSD defaults on modern glibc systems - pfeew ;-) sigaction(SIGUSR1, sa, NULL); // for later nullptr i doubt that you'll need another update for that, though. Wolfgang Bauer wrote: Well, should I change it? In kscreensaver/libkscreensaver/main.cpp they used sigaction() as well. 0 - NULL: yes. sigaction - signal: no. it just doesn't seem too much of a problem that I've constantly been using signal() whenever i needed it ;-) - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56190 --- On April 22, 2014, 7:56 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 7:56 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Review Request 117644: screenlocker: don't leave behind screensaver processes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/ --- Review request for kde-workspace and Plasma. Bugs: 224200 http://bugs.kde.org/show_bug.cgi?id=224200 Repository: kde-workspace Description --- Currently the screen locker just kills the greeter (kscreenlocker_greet) when the screen is unlocked by the user during the grace time. But apparently this can leave behind running screensaver processes launched by the greeter, see the bug report (which has the highest number of votes of all open bugs AFAICS). This patch changes this to only terminate the greeter, and adds a signal handler to the greeter to exit gracefully in this case. The signal handler exits with return code 1, so that it is not possible to circumvent the password input by just sending a SIGTERM. (the screen locker restarts the greeter in case it doesn't quit with exit code 0) Diffs - ksmserver/screenlocker/ksldapp.cpp 3dfcc9e ksmserver/screenlocker/greeter/main.cpp d898734 Diff: https://git.reviewboard.kde.org/r/117644/diff/ Testing --- Configure a legacy screensaver in Systemsettings-Display and Monitor-Screen Locker, be sure to leave Require Password after disabled. Wait for the screen locker to kick in. Unlock the screen by moving the mouse or pressing a key. Check the process list. Without this patch at least kswarm.kss and kblankscreen.kss reliably kept running after unlocking the screen on my system. With this patch they quit themselves. I'm using this patch for over two weeks now, and I haven't seen any left-over screen saver processes any more (and I even set the timeout to 1 minute). I also tried to terminate kscreenlocker_greet manually by running killall kscreenlocker_greet from a text console in case of a password required, and the locker didn't quit, you still have to enter the password. Thanks, Wolfgang Bauer
Re: Review Request 117644: screenlocker: don't leave behind screensaver processes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/ --- (Updated April 22, 2014, 10:41 p.m.) Review request for kde-workspace and Plasma. Changes --- Adapted the patch to a change in https://git.reviewboard.kde.org/r/117091/ Bugs: 224200 http://bugs.kde.org/show_bug.cgi?id=224200 Repository: kde-workspace Description --- Currently the screen locker just kills the greeter (kscreenlocker_greet) when the screen is unlocked by the user during the grace time. But apparently this can leave behind running screensaver processes launched by the greeter, see the bug report (which has the highest number of votes of all open bugs AFAICS). This patch changes this to only terminate the greeter, and adds a signal handler to the greeter to exit gracefully in this case. The signal handler exits with return code 1, so that it is not possible to circumvent the password input by just sending a SIGTERM. (the screen locker restarts the greeter in case it doesn't quit with exit code 0) Diffs (updated) - ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117644/diff/ Testing --- Configure a legacy screensaver in Systemsettings-Display and Monitor-Screen Locker, be sure to leave Require Password after disabled. Wait for the screen locker to kick in. Unlock the screen by moving the mouse or pressing a key. Check the process list. Without this patch at least kswarm.kss and kblankscreen.kss reliably kept running after unlocking the screen on my system. With this patch they quit themselves. I'm using this patch for over two weeks now, and I haven't seen any left-over screen saver processes any more (and I even set the timeout to 1 minute). I also tried to terminate kscreenlocker_greet manually by running killall kscreenlocker_greet from a text console in case of a password required, and the locker didn't quit, you still have to enter the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 10:34 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Changes --- Changed 0 to NULL in the sigaction() call. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs (updated) - ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117644: screenlocker: don't leave behind screensaver processes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/#review56219 --- Ship it! from here - see veto condition in the other RR (i do not maintain the screenlocker), but SIGTERM is oc. far better than SIGKILL - Thomas Lübking On April 22, 2014, 8:41 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/ --- (Updated April 22, 2014, 8:41 p.m.) Review request for kde-workspace and Plasma. Bugs: 224200 http://bugs.kde.org/show_bug.cgi?id=224200 Repository: kde-workspace Description --- Currently the screen locker just kills the greeter (kscreenlocker_greet) when the screen is unlocked by the user during the grace time. But apparently this can leave behind running screensaver processes launched by the greeter, see the bug report (which has the highest number of votes of all open bugs AFAICS). This patch changes this to only terminate the greeter, and adds a signal handler to the greeter to exit gracefully in this case. The signal handler exits with return code 1, so that it is not possible to circumvent the password input by just sending a SIGTERM. (the screen locker restarts the greeter in case it doesn't quit with exit code 0) Diffs - ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117644/diff/ Testing --- Configure a legacy screensaver in Systemsettings-Display and Monitor-Screen Locker, be sure to leave Require Password after disabled. Wait for the screen locker to kick in. Unlock the screen by moving the mouse or pressing a key. Check the process list. Without this patch at least kswarm.kss and kblankscreen.kss reliably kept running after unlocking the screen on my system. With this patch they quit themselves. I'm using this patch for over two weeks now, and I haven't seen any left-over screen saver processes any more (and I even set the timeout to 1 minute). I also tried to terminate kscreenlocker_greet manually by running killall kscreenlocker_greet from a text console in case of a password required, and the locker didn't quit, you still have to enter the password. Thanks, Wolfgang Bauer
Re: Review Request 117644: screenlocker: don't leave behind screensaver processes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/#review56240 --- would you please also adapt that for plasma-workspace repo (new master)? - Martin Gräßlin On April 22, 2014, 10:41 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/ --- (Updated April 22, 2014, 10:41 p.m.) Review request for kde-workspace and Plasma. Bugs: 224200 http://bugs.kde.org/show_bug.cgi?id=224200 Repository: kde-workspace Description --- Currently the screen locker just kills the greeter (kscreenlocker_greet) when the screen is unlocked by the user during the grace time. But apparently this can leave behind running screensaver processes launched by the greeter, see the bug report (which has the highest number of votes of all open bugs AFAICS). This patch changes this to only terminate the greeter, and adds a signal handler to the greeter to exit gracefully in this case. The signal handler exits with return code 1, so that it is not possible to circumvent the password input by just sending a SIGTERM. (the screen locker restarts the greeter in case it doesn't quit with exit code 0) Diffs - ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117644/diff/ Testing --- Configure a legacy screensaver in Systemsettings-Display and Monitor-Screen Locker, be sure to leave Require Password after disabled. Wait for the screen locker to kick in. Unlock the screen by moving the mouse or pressing a key. Check the process list. Without this patch at least kswarm.kss and kblankscreen.kss reliably kept running after unlocking the screen on my system. With this patch they quit themselves. I'm using this patch for over two weeks now, and I haven't seen any left-over screen saver processes any more (and I even set the timeout to 1 minute). I also tried to terminate kscreenlocker_greet manually by running killall kscreenlocker_greet from a text console in case of a password required, and the locker didn't quit, you still have to enter the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56241 --- Is that only relevant for the legacy (XSS) locker or also for the new locker? I'm just wondering whether it needs to be ported to master - Martin Gräßlin On April 22, 2014, 10:34 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 10:34 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
