diff mbox series

[bug#48220] gnu: xfce4-session: Add xset to propagated-inputs.

Message ID 615974577.179836.1620268738791@office.mailbox.org
State Accepted
Headers show
Series [bug#48220] gnu: xfce4-session: Add xset to propagated-inputs. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Brendan Tildesley May 6, 2021, 2:38 a.m. UTC 
> On 05/05/2021 3:23 PM Ludovic Courtès <ludo@gnu.org> wrote:
> 
>  
> Hi Brendan,
> 
> Brendan Tildesley <mail@brendan.scot> skribis:
> 
> > * gnu/packages/xfce.scm (xfce4-session):[propagated-inputs]: Add xset so
> > that xflock4 can turn off the monitor.
> 
> Can’t we instead patch ‘scripts/xflock4’ so that it refers to ‘xset’ by
> its absolute file name?
> 

I can. It felt weird just patching one command and not others though.

> However, my understanding is that the xset code is already a fallback:

> 
> --8<---------------cut here---------------start------------->8---
> # else run another access locking utility, if installed
> for lock_cmd in \
>   "xlock -mode blank" \
>   "slock"
>   do
>     set -- $lock_cmd
>     if command -v -- $1 >/dev/null 2>&1; then
>         $lock_cmd >/dev/null 2>&1 &
> 	# turn off display backlight:
> 	xset dpms force off
>         exit
>     fi
> done
> --8<---------------cut here---------------end--------------->8---
> 
> Probably we should ensure the first ‘for’ loop works as expected.
> Perhaps we need to replace ‘xfce4-screensaver-command’ by its absolute
> file name there.
> 

Currently xfce4-screensaver is not installed in the xfce package at all by default,
so it isn't used. I could add it, but for me it was glitchy/flickering and I would keep 
clearing the password as I was entering it so I couldn't log back in. It even does it
in a VM. I do have a recent amd graphics card with proprietary linux though, my computer
can't boot otherwise :(.

If you run guix environment --ad-hoc xfce4-screensaver
then
 xfce4-screensaver &; xfce4-screensaver-command --lock
does it work for you?

I'm not sure what is best, should we be maximal and include xfce4-screensaver in the xfce
package, or be minimal and make people have to install it manually?
> WDYT?
> 
> Thanks,
> Ludo’.

Comments

Efraim Flashner May 6, 2021, 12:03 p.m. UTC | #1 
On Thu, May 06, 2021 at 04:38:58AM +0200, Brendan Tildesley wrote:
> 
> > On 05/05/2021 3:23 PM Ludovic Courtès <ludo@gnu.org> wrote:
> > 
> >  
> > Hi Brendan,
> > 
> > Brendan Tildesley <mail@brendan.scot> skribis:
> > 
> > > * gnu/packages/xfce.scm (xfce4-session):[propagated-inputs]: Add xset so
> > > that xflock4 can turn off the monitor.
> > 
> > Can’t we instead patch ‘scripts/xflock4’ so that it refers to ‘xset’ by
> > its absolute file name?
> > 
> 
> I can. It felt weird just patching one command and not others though.
> 
> > However, my understanding is that the xset code is already a fallback:
> 
> > 
> > --8<---------------cut here---------------start------------->8---
> > # else run another access locking utility, if installed
> > for lock_cmd in \
> >   "xlock -mode blank" \
> >   "slock"
> >   do
> >     set -- $lock_cmd
> >     if command -v -- $1 >/dev/null 2>&1; then
> >         $lock_cmd >/dev/null 2>&1 &
> > 	# turn off display backlight:
> > 	xset dpms force off
> >         exit
> >     fi
> > done
> > --8<---------------cut here---------------end--------------->8---
> > 
> > Probably we should ensure the first ‘for’ loop works as expected.
> > Perhaps we need to replace ‘xfce4-screensaver-command’ by its absolute
> > file name there.
> > 
> 
> Currently xfce4-screensaver is not installed in the xfce package at all by default,
> so it isn't used. I could add it, but for me it was glitchy/flickering and I would keep 
> clearing the password as I was entering it so I couldn't log back in. It even does it
> in a VM. I do have a recent amd graphics card with proprietary linux though, my computer
> can't boot otherwise :(.
> 
> If you run guix environment --ad-hoc xfce4-screensaver
> then
>  xfce4-screensaver &; xfce4-screensaver-command --lock
> does it work for you?
> 
> I'm not sure what is best, should we be maximal and include xfce4-screensaver in the xfce
> package, or be minimal and make people have to install it manually?
> > WDYT?
> > 
> > Thanks,
> > Ludo’.

Are you using %desktop-services in your OS config? By default they come
with a screen-locker-service for both slock and xlockmore.
Brendan Tildesley May 6, 2021, 12:13 p.m. UTC | #2 
> On 05/06/2021 2:03 PM Efraim Flashner <efraim@flashner.co.il> wrote:
> 
>  
> On Thu, May 06, 2021 at 04:38:58AM +0200, Brendan Tildesley wrote:
> > 
> > > On 05/05/2021 3:23 PM Ludovic Courtès <ludo@gnu.org> wrote:
> > > 
> > >  
> > > Hi Brendan,
> > > 
> > > Brendan Tildesley <mail@brendan.scot> skribis:
> > > 
> > > > * gnu/packages/xfce.scm (xfce4-session):[propagated-inputs]: Add xset so
> > > > that xflock4 can turn off the monitor.
> > > 
> > > Can’t we instead patch ‘scripts/xflock4’ so that it refers to ‘xset’ by
> > > its absolute file name?
> > > 
> > 
> > I can. It felt weird just patching one command and not others though.
> > 
> > > However, my understanding is that the xset code is already a fallback:
> > 
> > > 
> > > --8<---------------cut here---------------start------------->8---
> > > # else run another access locking utility, if installed
> > > for lock_cmd in \
> > >   "xlock -mode blank" \
> > >   "slock"
> > >   do
> > >     set -- $lock_cmd
> > >     if command -v -- $1 >/dev/null 2>&1; then
> > >         $lock_cmd >/dev/null 2>&1 &
> > > 	# turn off display backlight:
> > > 	xset dpms force off
> > >         exit
> > >     fi
> > > done
> > > --8<---------------cut here---------------end--------------->8---
[...]
> Are you using %desktop-services in your OS config? By default they come
> with a screen-locker-service for both slock and xlockmore.
> 

Yep, those ones work but they merely produce a black screen with the backlight on,
then the xset command in the above script actually turns off the screen to save power.
if any of xfce4-screensaver, gnome-screensaver, or xscreensaver are available, then it
will use those first.
Ludovic Courtès May 8, 2021, 10:23 a.m. UTC | #3 
Hi!

Brendan Tildesley <mail@brendan.scot> skribis:

[...]

>> However, my understanding is that the xset code is already a fallback:
>
>> 
>> --8<---------------cut here---------------start------------->8---
>> # else run another access locking utility, if installed
>> for lock_cmd in \
>>   "xlock -mode blank" \
>>   "slock"
>>   do
>>     set -- $lock_cmd
>>     if command -v -- $1 >/dev/null 2>&1; then
>>         $lock_cmd >/dev/null 2>&1 &
>> 	# turn off display backlight:
>> 	xset dpms force off
>>         exit
>>     fi
>> done
>> --8<---------------cut here---------------end--------------->8---
>> 
>> Probably we should ensure the first ‘for’ loop works as expected.
>> Perhaps we need to replace ‘xfce4-screensaver-command’ by its absolute
>> file name there.
>> 
>
> Currently xfce4-screensaver is not installed in the xfce package at all by default,
> so it isn't used. I could add it, but for me it was glitchy/flickering and I would keep 
> clearing the password as I was entering it so I couldn't log back in. It even does it
> in a VM. I do have a recent amd graphics card with proprietary linux though, my computer
> can't boot otherwise :(.

Hmm OK.  We should address this in a separate issue.

> If you run guix environment --ad-hoc xfce4-screensaver
> then
>  xfce4-screensaver &; xfce4-screensaver-command --lock
> does it work for you?

I get:

--8<---------------cut here---------------start------------->8---
$ xfce4-screensaver & xfce4-screensaver-command --lock
[1] 19108
** Message: 12:20:44.361: Screensaver is not running! Start xfce4-screensaver first
--8<---------------cut here---------------end--------------->8---

… but I’m not running Xfce.  I should prolly try in a VM.

> I'm not sure what is best, should we be maximal and include xfce4-screensaver in the xfce
> package, or be minimal and make people have to install it manually?

I’d lean towards including xfce4-screensaver, since that’s the intended
use of Xfce, but only once it actually works.

> From ed66cf50a3b9294effc8bbae04b0f2564bd55c10 Mon Sep 17 00:00:00 2001
> From: Brendan Tildesley <mail@brendan.scot>
> Date: Thu, 6 May 2021 12:34:55 +1000
> Subject: [PATCH] gnu: xfce4-session: Allow xflock4 to use xset.
>
> * gnu/packages/xfce.scm (xfce4-session):
> [inputs]: Add xset.
> [arguments]: Add a phase to use exact store path to xset in xflock4.

[...]

> +         (add-after 'unpack 'patch-xflock
> +           (lambda _
> +             (substitute* "scripts/xflock4"
> +               (("xset") (which "xset"))))))))

Applied after changing ‘which’ to refer to the “xset” input so that it
works correctly when cross-compiling.

Closing, and let’s discuss xfce4-screensaver separately!

Thanks,
Ludo’.
diff mbox series

Patch

From ed66cf50a3b9294effc8bbae04b0f2564bd55c10 Mon Sep 17 00:00:00 2001
From: Brendan Tildesley <mail@brendan.scot>
Date: Thu, 6 May 2021 12:34:55 +1000
Subject: [PATCH] gnu: xfce4-session: Allow xflock4 to use xset.

* gnu/packages/xfce.scm (xfce4-session):
[inputs]: Add xset.
[arguments]: Add a phase to use exact store path to xset in xflock4.
---
 gnu/packages/xfce.scm | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/xfce.scm b/gnu/packages/xfce.scm
index 8aa2770ee8..164631cf4d 100644
--- a/gnu/packages/xfce.scm
+++ b/gnu/packages/xfce.scm
@@ -707,7 +707,13 @@  your system in categories, so you can quickly find and launch them.")
        (list (string-append "--with-xsession-prefix=" %output))
        ;; Disable icon cache update.
        #:make-flags
-       '("gtk_update_icon_cache=true")))
+       '("gtk_update_icon_cache=true")
+       #:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'patch-xflock
+           (lambda _
+             (substitute* "scripts/xflock4"
+               (("xset") (which "xset"))))))))
     (native-inputs
      `(("pkg-config" ,pkg-config)
        ("intltool" ,intltool)))
@@ -717,7 +723,8 @@  your system in categories, so you can quickly find and launch them.")
        ("polkit" ,polkit)
        ("libsm" ,libsm)
        ("libwnck" ,libwnck)
-       ("libxfce4ui" ,libxfce4ui)))
+       ("libxfce4ui" ,libxfce4ui)
+       ("xset" ,xset)))
     (home-page "https://www.xfce.org/")
     (synopsis "Xfce session manager")
     (description
-- 
2.31.1