GNU bug report logs - #78188
[PATCH] Add KWallet service

Previous Next

Full log

Message #32 received at 78188 <at> debbugs.gnu.org (full text, mbox):

From: Sergio Pastor Pérez <sergio.pastorperez <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 78188 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>,
 Noé Lopez <noelopez <at> free.fr>,
 Liliana Marie Prikler <liliana.prikler <at> gmail.com>,
 Vivien Kraus <vivien <at> planete-kraus.eu>
Subject: Re: [bug#78188] [PATCH v2] services: kwallet: New service.
Date: Fri, 02 May 2025 09:24:06 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
> Sergio Pastor Pérez <sergio.pastorperez <at> gmail.com> writes:
>> +@defvar kwallet-service-type
>> +This is the type of the service that adds the
>> +@uref{https://invent.kde.org/plasma/kwallet-pam, KWallet keyring}.  Its
>> +value is a @code{kwallet-configuration} object (see below).  Note that,
>> +contrary to @code{gnome-desktop-service-type},
>> +@code{plasma-desktop-service-type} does not include this service.
>
> Does gnome-desktop-service-type include the kwallet-service-type?  I
> wouldn't think so.  You probably meant to say it "doesn't include a
> wallet service in its default configuration." ?  Is this the normal
> expectation for the KDE desktop?  I'd assume it comes with kwallet
> pre-configured, if using Fedora for example.  If it does, we should
> probably do so to avoid breaking users expectations.  I've recently made
> the adjustment in GNOME to have the GNOME keyring unlocked by default
> for the GNOME desktop, as that's what users expect.

I think I made a mistake with the wording. What I meant is that, while
the `gnome-desktop-service-type' includes a keyring field which provides
the functionality of `gnome-keyring-service-type' making it necessary
for a `gnome-desktop-service-type' user to add a
`gnome-keyring-service-type' entry to their service list; the
`plasma-desktop-service-type' does not provide this functionality by
default. Which contrary to what the user may expect, they will need to
add the `kwallet-service-type' to their list of services even if they
are users of the `plasma-desktop-service-type'.

In the v2 of this patch series I've rewrote that bit to make it more
clear. Let me know what you think.

>> +;; TODO: consider integrating service in `<plasma-desktop-configuration>' as
>> +;; done in `<gnome-desktop-configuration>'. This requires rewritting the
>> +;; `<plasma-desktop-service-type>' as done for `<gnome-desktop-service-type>'.
>
> Ah, I see my comment above is acknowledged here as a TODO.  I'd
> encourage you to pursue that next!

This will require a major refactor of the `plasma-desktop-service-type'
as was done for the `gnome-desktop-service-type' to make it
modular. Once we get this merged I will try to join the kde team to help
with the refactor.

> Otherwise it LGTM.  Could you please send a v2?

Sure, aside for the things I've mentioned I've also corrected all the
typos you noticed.

Thanks for the review!


Best regards,
Sergio.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.