Re: plasma-framework, kactivities and kactivities-stats: please consider proper de-KF-ication now

[Kevin Ottens]

Hello,

On Sunday, 5 November 2023 15:32:21 CET christ...@cullmann.io wrote:
> On 2023-11-05 15:11, Nate Graham wrote:
> > Baloo is linked by some apps, e.g. Elisa, and I wouldn't like to make
> > them haul in Plasma stuff.
> 
> some applications link to kactivities, too.
> I think it just makes it more clear that this will just work with
> Plasma.
>
> But I can live with the status quo, too, just thought it would be
> cleaner.

Yes, I think it'd make sense to have a clearer and cleaner separation between 
KDE Frameworks dependencies which can work outside of a Plasma sessions and 
libraries which require Plasma runtime components.

I was clumsily advocating for this Akademy 2021 or 2022 (can't remember 
which).

This way it's clearer to application authors when they tie themselves to a 
given workspace or not.

Also, isn't Elisa able to work without Baloo? It even seems to do the right 
thing if I trust its CMakeLists.txt. It has Baloo as a recommended but 
optional dependency, and disable it altogether for Win32 builds.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud supporting member of KDE
https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: Splitting KGlobalAccel interface and runtime

[Kevin Ottens]

Hello,

On Monday, 13 February 2023 21:25:54 CET Vlad Zahorodnii wrote:
> On 2/13/23 22:05, Nicolas Fella wrote:
> > Hi,
> > 
> > the kglobalaccel framework currently contains two pieces: kglobalacceld,
> > the runtime component that manages global shortcuts, and an
> > application-side library to interact with it.
> > 
> > The two communicate with each other via DBus. On X11 there is a
> > standalone kglobalacceld5 process providing the interface, on Wayland
> > the runtime is linked into KWin and thus the kwin_wayland process
> > provides the interface.
> > 
> > The current architecture has a number of downsides:
> > 
> > - Any call to the KGlobalAccel library may DBus-activate the
> > kglobalacceld5 process, which may be undesired on Desktop other than
> > Plasma since it competes with their native shortcut system. We tried
> > fixing that by making such calls no-op on !Plasma, but that broke things
> > for people that did want it to run, for example people using KWin with
> > LXQt, because KWin relies on KGlobalAccel for shortcuts
> > 
> > - We want to keep the dependencies of the interface library minimal,
> > which is inconvenient for the development of the runtime part. For
> > example we really want to use KIO::ApplicationLauncherJob in the
> > runtime, but currently can't, because that would introduce a dependency
> > cycle in Frameworks (KIO depends on KXmlGui, which depends on
> > KGlobalAccel, which would depend on KIO)
> > 
> > - Coinstallability of KF5 and KF6. Conceptually there can only be one
> > kglobalacceld. If both KF5 KGlobalAccel and KF6 KGlobalAccel install a
> > kglobalacceld this is going to be problematic. This also means that a
> > KF6-based kglobalacceld must work with a KF5 interface library
> > 
> > - Other shortcut daemon implementations. Since somewhat recently there
> > is an XDG Portal for global shortcuts. Platforms like Windows and macOS
> > also have ways for applications to register global shortcuts. While we
> > are currently not using any of these it's very well possible that we
> > would eventually want to use the KGlobalAccel interface library to
> > interact with those. Having the kglobalacceld runtime in the same
> > frameworks therefore doesn't feel right.
> > 
> > To address these issues I suggest we split out the runtime part of
> > kglobalaccel into its own project, under the Plasma release group,
> > because that's primarily where it's used/supported. The interface
> > library would remain in frameworks. We have a similar situation with
> > activities, where the manager (kactivitymanagerd) is in Plasma and the
> > interface is in Frameworks. When doing this we'd also change the way how
> > kglobalacceld is started away from DBus activation towards explicitly
> > starting it as part of the Plasma startup. This avoids accidentally
> > launching it when it shouldn't be but still allows to explicitly start
> > outside of Plasma if really wanted. It would also allow for greater
> > flexibility in the development of the runtime, especially around
> > dependency constraints.
> > 
> > It wouldn't automatically solve the coinstallability problem of KF5 and
> > KF6, because a kglobalacceld provided by KF5-KGlobalAccel would still
> > conflict with a Plasma-provided kglobalacceld, but it's at least
> > conceptually less messy since it's clear that the Plasma-provided one
> > would be the preferred one to use.
> > 
> > Thoughts about this?
> 
> +1
> 
> There's one caveat though: given that the library and the runtime parts
> will have different release schedules, we will have to be careful about
> protocol changes. Perhaps we could borrow a thing or two from activities?

Or... move both runtime and API on the Plasma side? This way no problem of 
different release schedules and it makes it clear that using it ties you to a 
specific desktop anyway?

With a quick grep it looks like most of the users are already shipped with 
Plasma or desktop specific anyway. Granted that leaves with a couple of tough 
nuts to crack though.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud supporting member of KDE
https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: KF6 meeting notes 2021-06-12

[Kevin Ottens]

Hello,

On Friday, 18 June 2021 22:36:34 CEST Volker Krause wrote:
> On Freitag, 18. Juni 2021 22:27:40 CEST Kevin Ottens wrote:
> > On Friday, 18 June 2021 15:21:07 CEST Volker Krause wrote:
> > > Added now, Monday 09:00 UTC.
> > 
> > OK, I will show up only at 10 though. I can't do 9.
> 
> oh, sorry, I did this from the top of my head ("Monday morning works for
> everyone"), I missed you could only do 10:00 UTC. Should we move it?

Well, moving + keeping the "morning slots" constraint means shortening the BoF 
though. Unsure if that's wise. Obviously I'm mainly interested in what I'll 
present in my talk. So we can also cross our fingers that nothing I'm really 
into gets discussed before 10am. ;-)

Other option is moving to 5pm, unsure that suits many people though...

Cheers.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: KF6 meeting notes 2021-06-12

[Kevin Ottens]

On Friday, 18 June 2021 15:21:07 CEST Volker Krause wrote:
> Added now, Monday 09:00 UTC.

OK, I will show up only at 10 though. I can't do 9.

Cheers.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: KF6 meeting notes 2021-06-12

[Kevin Ottens]

Hello,

On Tuesday, 15 June 2021 18:17:27 CEST Volker Krause wrote:
> On Dienstag, 15. Juni 2021 12:45:21 CEST Luigi Toscano wrote:
> > David Faure ha scritto:
> > > On dimanche 13 juin 2021 20:32:42 CEST Luigi Toscano wrote:
> > >> -> we need need input about dfaure presence and try to avoid conflicts
> > >> with
> > >> Plasma BoFs
> > > 
> > > I'm available Mon-Thu, but since Tue-Wed is also Qt Contributor Summit,
> > > maybe better plan for the KF6 BoF on Monday (outside the AGM time)?
> > 
> > It seems Monday is pretty packed already, but there is space at 09:00 UTC
> > and 10:00 UTC, unless people are fine with overlapping with the "KDE goal"
> > slots at 17:00 UTC and 18:00 UTC (there is Kirigami at 16:00 UTC, so maybe
> > not that one):
> > 
> > https://community.kde.org/Akademy/2021/Monday
> > 
> > What do you (plural you) think?
> 
> I'd vote for the Monday morning slots as well, we can then still schedule
> subsequent session later that week if needed, QtCS and Akadmey BoF slots are
> not overlapping all day AFAIK.

Did we book anything after all? Couldn't find anything in the wiki, wondering 
if I missed it somehow?

Cheers.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: KF6 meeting notes 2021-06-12

[Kevin Ottens]

Hello,

On Tuesday, 15 June 2021 12:45:21 CEST Luigi Toscano wrote:
> David Faure ha scritto:
> > On dimanche 13 juin 2021 20:32:42 CEST Luigi Toscano wrote:
> >> -> we need need input about dfaure presence and try to avoid conflicts
> >> with
> >> Plasma BoFs
> > 
> > I'm available Mon-Thu, but since Tue-Wed is also Qt Contributor Summit,
> > maybe better plan for the KF6 BoF on Monday (outside the AGM time)?
> 
> It seems Monday is pretty packed already, but there is space at 09:00 UTC
> and 10:00 UTC, unless people are fine with overlapping with the "KDE goal"
> slots at 17:00 UTC and 18:00 UTC (there is Kirigami at 16:00 UTC, so maybe
> not that one):
> 
> https://community.kde.org/Akademy/2021/Monday
> 
> What do you (plural you) think?

FWIW, best for me would be Monday at 10:00 UTC. Second best is 17:00 UTC.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: Re KIO workers

[Kevin Ottens]

Hello,

On Saturday, 5 June 2021 22:01:54 CEST David Faure wrote:
> On samedi 5 juin 2021 21:07:39 CEST Kevin Ottens wrote:
> > On Saturday, 5 June 2021 17:51:18 CEST David Faure wrote:
> > > On samedi 5 juin 2021 16:29:10 CEST Volker Krause wrote:
> > > > Do KIO slaves still need the klauncher/kinit loading mechanism?
> > > 
> > > No. My request for developers to test KIO_FORK_SLAVES=1
> > > for daily use is so that apps fork kio worker processes directly,
> > > without going via klauncher/kinit. BTW it seems to work fine. I wonder
> > > if we should toggle that in 5.84, as part of the incremental move to the
> > > KF6 world.
> > 
> > Actually it works so well that I almost forgot I turned KIO_FORK_SLAVES
> > on...
> 
> I hope we're both wrong: the env var is *KDE*_FORK_SLAVES :)

I forgot it so well that I got the variable wrong under the influence of your 
email. Yes, I got KDE_FORK_SLAVES enabled... went and check since you gave me 
a doubt. :-D

Cheers.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: Re KIO workers

[Kevin Ottens]

Hello,

On Saturday, 5 June 2021 17:51:18 CEST David Faure wrote:
> On samedi 5 juin 2021 16:29:10 CEST Volker Krause wrote:
> > Do KIO slaves still need the klauncher/kinit loading mechanism?
> 
> No. My request for developers to test KIO_FORK_SLAVES=1
> for daily use is so that apps fork kio worker processes directly, without
> going via klauncher/kinit. BTW it seems to work fine. I wonder if we should
> toggle that in 5.84, as part of the incremental move to the KF6 world.

Actually it works so well that I almost forgot I turned KIO_FORK_SLAVES on...
 
Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: KF6 online sprint: March 27-28

[Kevin Ottens]

Hello,

On Wednesday, 24 March 2021 14:11:19 CET David Edmundson wrote:
> I wanted to bump this thread, just so people remember that it is this
> weekend! There are many many slots still available.

Well, in our great tradition, my expectation is that the first hour will be 
devoted to filling the other slots. ;-)

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: KCGroups in KDEreview

[Kevin Ottens]

Hello,

On Tuesday, 2 March 2021 12:06:51 CET David Edmundson wrote:
> > > (where 1000 is of course dynamic)
> > 
> > Ditto, what enum names could we give to those?
> 
> / -> All
> /system.slice -> System
> user.slice/user-1000.slice/user@1000.service -> User
> user.slice/user-1000.slice/user@1000.service/app.slice -> UserApps
> user.slice/user-1000.slice/user@1000.service/session.slice -> UserSession
> user.slice/user-1000.slice/user@1000.service/background.slice ->
> UserBackground

Sounds good to me. Let's go for this.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

Re: KDE Frameworks 6 - Virtual Sprint setup

[Kevin Ottens]

Hello,

On Saturday, 30 January 2021 12:14:27 CET Volker Krause wrote:
> Thanks for driving this, I'm in! European hours preferred, any weekend
> starting from w6 should be possible.

Same here, not before week 10 or even better not before week 13.

Cheers.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

Re: KCGroups in KDEreview

[Kevin Ottens]

On Sunday, 13 December 2020 22:41:24 CET David Edmundson wrote:
> On Thu, Dec 3, 2020 at 11:48 AM Kevin Ottens  wrote:
> > Hello,
> > 
> > On Thursday, 3 December 2020 12:15:52 CET David Edmundson wrote:
> > > Ultimately this isn't really dealing with cgroups directly but with
> > > the manager to control them, systemd.
> > > 
> > > That's correct usage, kernel docs of cgroups say to go via a
> > > controller for write operations. However that at point is it worth
> > > naming the library ksystemd?
> > > It might cause some contention due to people who get angsty at a name,
> > > but it's a lot more fitting. It would then give us a place to dump a
> > > lot of other wrappers (especially logind) that we're seeing duplicated
> > > in a bunch of places throughout KDE.
> > 
> > Aren't you concerned this might lead to one of our infamous dumping
> > grounds?
> > 
> > There's always a tension between making it too focused and tiny or
> > unfocused and with blackhole mass... we'd need to find where it stands
> > but "systemd wrappers" looks too loosely defined to me.
> > 
> > 
> > Do we have a list of the wrappers you got in mind and which piece of
> > feature they all provide?
> 
> StartUnit, given this has a StopUnit
> Used by plasma-workspace and plasma-firewall
> 
> Logind I have wrappers in:
>  - kwin
>  - libkworkspace
>  - SDDM
>  - powerdevil
>  - kscreenlocker
> 
> None wrapping all of it, only what they need, but there's still overlap.
> You're right that could be a separate framework if that's preferred.

I'll be honest I'm still unsure what that'd mean in practice... What about 
drafting API for a couple of those? Let's not get overboard implementing stuff 
and so on, it's more exploratory that anything for now.

> > > I think we've artificially limited the usage of the library.
> > > The code is very generic for handling units, but all the names and one
> > > tiny line limit it to only managing a subset of units.
> > > 
> > > If we make the "glob" static used in KApplicationScopeLister's have a
> > > public setter (or a protected virtual) we can rename this class and it
> > > becomes a much more generic library for others to use outside of any
> > > initial use-case.
> > 
> > Good point. Got a similar question though, which other unit types would be
> > useful to control? Reason being that API wise I'd smell an enum for
> > something like this.
> 
> Enum potentially works too.
> 
> Describing things in terms of cgroup hierarchy, I think the key use cases
> are:
> 
> /
> user.slice/user-1000.slice/user@1000.service
> user.slice/user-1000.slice/user@1000.service/app.slice
> user.slice/user-1000.slice/user@1000.service/session.slice
> user.slice/user-1000.slice/user@1000.service/background.slice
> (where 1000 is of course dynamic)

Ditto, what enum names could we give to those?

Yes, I know I roll with questions. :-)

Cheers.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

Re: KCGroups in KDEreview

[Kevin Ottens]

Hello,

On Thursday, 3 December 2020 12:15:52 CET David Edmundson wrote:
> Ultimately this isn't really dealing with cgroups directly but with
> the manager to control them, systemd.
> 
> That's correct usage, kernel docs of cgroups say to go via a
> controller for write operations. However that at point is it worth
> naming the library ksystemd?
> It might cause some contention due to people who get angsty at a name,
> but it's a lot more fitting. It would then give us a place to dump a
> lot of other wrappers (especially logind) that we're seeing duplicated
> in a bunch of places throughout KDE.

Aren't you concerned this might lead to one of our infamous dumping grounds?

There's always a tension between making it too focused and tiny or unfocused 
and with blackhole mass... we'd need to find where it stands but "systemd 
wrappers" looks too loosely defined to me.

Do we have a list of the wrappers you got in mind and which piece of feature 
they all provide?
 
> I think we've artificially limited the usage of the library.
> The code is very generic for handling units, but all the names and one
> tiny line limit it to only managing a subset of units.
> 
> If we make the "glob" static used in KApplicationScopeLister's have a
> public setter (or a protected virtual) we can rename this class and it
> becomes a much more generic library for others to use outside of any
> initial use-case.

Good point. Got a similar question though, which other unit types would be 
useful to control? Reason being that API wise I'd smell an enum for something 
like this.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

Re: KCGroups in KDEreview

[Kevin Ottens]

Hello,

On Friday, 20 November 2020 14:55:16 CET Henri Chain wrote:
> KCgroups has been moved to KDEReview !
> What is that, you ask ? It's a library that wraps the systemd dbus API to
> expose a higher-level concept of desktop application and allow control of
> its system resource usage (CPU, RAM, IO, etc).
> 
> It relies on the recent ability of plasma to launch applications in their
> own systemd scopes, with correspond to cgroups and provides a more robust
> definition for an application (more details at
> https://lwn.net/Articles/834329/ ) .
> 
> The main use of the library is to expose related resource control settings
> for those applications, at a user space level that other KDE applications
> and frameworks can use, including consumption straight from QML as
> demonstrated in the test application.
> 
> KCgroups is intended to become a (Tier 1) framework. A first user of this
> library might be the foreground window CPU booster daemon that is available
> here:
> https://invent.kde.org/libraries/kcgroups/-/tree/work/foreground-booster
> 
> Packages are already available for both Neon and Arch Linux.
> 
> Looking forward to your feedback and ideas for using this,

Glad to see this come to fruition.

One concern regarding use with other libraries:
 * the OPTIONAL_GADGET use introduce names that I could see potentially 
clashing with application code or code in third party libraries, I wonder if 
they should be moved in a namespace as well (as in "like optional") or have a 
K name at least;

Minor things I found:
 * tests/main.cpp needs to have some of its includes cleaned up (at a glance 
QDebug and QTimer aren't needed there);
 * tests/main.qml should have better ids for its elements IMHO, often those 
manual tests end up used as examples by people and having better readability 
helps (also avoids having people copying and pasting shameful things). ;-)

That's not much but I'm obviously biased by the fact that we discussed this 
API together already. :-)

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

Re: Add loop device interface to Solid framework

[Kevin Ottens]

Hello,

Sorry for the slow reply time. :-)

On Monday, 15 June 2020 20:19:54 CEST Kwon-Young Choi wrote:
> Well, I don't know if it possible to make an optional action such as a
> delete method which would work only if the device is a loop device
> backed by a file.

That's totally doable, and probably a good idea to put it on a separate 
DeviceInterface.
 
> In my opinion, it would be better to check if the device is a loop
> device, if it is you can convert it to the Loop type and can call
> methods like `delete` and access properties like `backingFile`.

Exactly.

> However, I still have no idea where to put the create loop device method
> since there is no concept of a device manager in solid which can modify
> the global state of the system.

That's something I'd be a bit more wary of having into solid. We used to have 
a device manager concept in the API and well... let's say it didn't end up 
well.

The problem of having one is that it somehow starts behaving like an attractor 
for all kinds of weird features and we'd end up doing too much in solid.

Since it's something very specific to loop devices... if we eally want it 
in solid (I suspect it's more something you'd want in the file manager and 
solid reacting to it) then maybe just this time we could have a static method 
in the future "LoopDevice" (or whatever name ends up picked) interface for 
creating those.

Just my 0.02€.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net


signature.asc
Description: This is a digitally signed message part.

D28590: Add a QString Solid::Device::displayName, used in Fstab Device for network mounts

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in device.h:99
> Well I have to make this virtual it seems so this call is dynamically 
> dispatched by `return_SOLID_CALL(Ifaces::Device *, d->backendObject(), 
> QString(), displayName());`
> I assumed this would work based on my review comments rather than on tests :/

Woops... my bad, indeed didn't notice it wasn't marked virtual anymore. It has 
to be indeed.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28590

To: meven, #frameworks, bruns, sitter, dfaure, ngraham
Cc: ngraham, dfaure, broulik, ervin, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, bruns

D28590: Add a QString Solid::Device::displayName, used in Fstab Device for network mounts

[Kevin Ottens]

ervin added a comment.


  LGTM now, I'll let the other reviewers weight in though.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28590

To: meven, #frameworks, bruns, sitter, dfaure
Cc: dfaure, broulik, ervin, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28590: Add a QString Solid::Device::displayName, used in Fstab Device for network mounts

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> device.h:99
> +  */
> +virtual QString displayName() const = 0;
> +

Why not have a default implementation which returns descriptions()? This would 
make for a less intrusive patch (I think it's in part what @bruns suggested 
earlier).

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28590

To: meven, #frameworks, bruns, sitter, dfaure
Cc: dfaure, broulik, ervin, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28460: Add KCModuleDada as base class for plugin

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.
This revision is now accepted and ready to land.


  Please fix the typo in the commit title before pushing, otherwise looks fine 
to me.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28460

To: bport, #plasma, ervin
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  You know it started as a proper painted indicator within the widget area, 
right? As such it couldn't have any of the issues you're pointing out now... 
Who pushed me to have them at distance I wonder? Right, was people from the 
VDG. So I find grand that then it goes all to revert because after weeks of 
pushing me to add more weird constraints then disappearing letting me alone 
trying to figure out where to go (and it was a large struggle at every step), 
the conclusion is "let's revert because VDG wasn't involved". There were 
technical reasons for the very first iteration and they had to be disregarded 
for design license.
  
  Honestly I'm disgusted by the way it's been handled.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: bam, GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham, bruns

D29014: Fix the exclusive group box case for default indicators

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:14ecec53296a: Fix the exclusive group box case for 
default indicators (authored by ervin).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29014?vs=80664=80682

REVISION DETAIL
  https://phabricator.kde.org/D29014

AFFECTED FILES
  src/kconfigdialogmanager.cpp

To: ervin, ngraham, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29014: Fix the exclusive group box case for default indicators

[Kevin Ottens]

ervin updated this revision to Diff 80664.
ervin added a comment.


  Fix misplaced *

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29014?vs=80661=80664

REVISION DETAIL
  https://phabricator.kde.org/D29014

AFFECTED FILES
  src/kconfigdialogmanager.cpp

To: ervin, ngraham, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#652669 , @ngraham wrote:
  
  > Finally clicking on the revert button in Spectacle's settings page causes a 
segfault for me: P590 Spectacle crash backtrace 

  
  
  Couldn't quite reproduce it the way you described, but indeed managed to get 
it to crash. It's fixed with D29014 

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29014: Fix the exclusive group box case for default indicators

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: ngraham, davidedmundson, bport, crossi.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  Turns out there was an oversight here, in case of an exclusive group
  box, the emitter widget is a button inside the group box which can have
  any name (this was thus caught by the assert).
  
  So now we also check if the sender is a button and if that's the case we
  verify if its in one of the known exclusive group boxes. If yes we use
  the group box as reference for the indicator work.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D29014

AFFECTED FILES
  src/kconfigdialogmanager.cpp

To: ervin, ngraham, davidedmundson, bport, crossi
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#652664 , @ngraham wrote:
  
  > David asked for VDG to approve before this landed, which wasn't done.
  
  
  Dude, I jumped through all the hoops for the past weeks. Also it got no 
further reply after I updated the screenshot almost two weeks ago so yes I 
assumed you guys had nothing else.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:24211925f835: Introduce SettingState* elements to ease 
KCM writing (authored by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=80305=80654

REVISION DETAIL
  https://phabricator.kde.org/D27840

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27841: Port desktoptheme, icons and workspace KCMs to SettingStateBinding

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:dfc144bf3f45: Port desktoptheme, icons and workspace KCMs 
to SettingStateBinding (authored by ervin).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27841?vs=76953=80655

REVISION DETAIL
  https://phabricator.kde.org/D27841

AFFECTED FILES
  kcms/desktoptheme/package/contents/ui/main.qml
  kcms/icons/package/contents/ui/IconSizePopup.qml
  kcms/icons/package/contents/ui/main.qml
  kcms/workspaceoptions/package/contents/ui/main.qml

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma, #vdg
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27839: Properly name the content of the kcmcontrols project

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:3501bcbe8da6: Properly name the content of the 
kcmcontrols project (authored by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27839?vs=76951=80653

REVISION DETAIL
  https://phabricator.kde.org/D27839

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:11186c056198: KCModule: Indicate when a setting has been 
changed from the default or previous… (authored by ervin).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=79656=80652

REVISION DETAIL
  https://phabricator.kde.org/D27540

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

Re: Problems in KWayland causes by API and ABI compatibility promises

[Kevin Ottens]

Hello,

On Thursday, 16 April 2020 23:38:23 CEST David Edmundson wrote:
> On Wed, Apr 8, 2020 at 5:10 PM Kevin Ottens  wrote:
> > On Wednesday, 1 April 2020 14:04:10 CEST David Edmundson wrote:
> > > Here is a list of active uses of the KWayland::Client API.
> > > 
> > > frameworks
> > > 
> > > plasma-framework (for window positioning)
> > > 
> > > apps:
> > > spectacle (for window positioning)
> > > kdeconnect-kde  (for fake input)
> > > yakuake (for window positioning)
> > > 
> > > extragear
> > > 
> > > latte-dock (for window positioning, custom shadow (which could be
> > > 
> > > ported already) and windowmanagement)
> > 
> > The part of the list above makes me wonder, shouldn't the window
> > positioning and windowmanager features be on KWindowSystem?
> 
> Yeah, doing that will resolve 90% of the client cases.
> In general for things like blur and everything the slightly higher
> level abstraction is working out better as we can abstract X and
> wayland in one go, which really helps the client code.

Sounds like a plan then.
 
> Also, it's worth really clarifying that in absolutely any proposal
> KWayland as-is can't go away, so clients using that will still
> function.

Of course, it's just that the one in KF would become frozen and deprecated 
until KF6. But then the development would move on with the other copy which 
would be less of a burden for everyone.

> The slight twist on that which we need to be wary of is that client
> code will return shared objects if you request a
> KWaylandClient::PlasmaShellSurface::get(window())
> for the same window from two places you'll get the same PlasmaShell
> instance returned - and therefore the same wl_resource.
> If we hypothetically had a kwayland2::client also have a
> plasmashellsurface::get() method we would have two plasma_shellsurface
> wl_resources's for the same wl_surface which is a protocol error and
> our client will get violently killed.

Honestly you lost me here. :-)

> > > workspace:
> > > kwin unit tests
> > > libkscreen
> > > breeze (till Qt5.15)
> > > oxyen (till Qt5.15)
> > > xdg-desktop-portal
> > > kinfocenter
> > > plasma-workspace
> > > plasma-nano
> > > kscreenlocker
> > > powerdevil
> > > kwayland-integration (the backend for kwindowsystem)
> > > plasma-phone-components
> > >     plasma-integration
> > 
> > I think the above is less of an issue, right? Because workspace would have
> > a copy of KWayland which could be shared with whatever stability
> > constraints needed.
> 
> It means it has to stay as an exported workspace lib, but yeah it
> wouldnt' be a problem.

Yes, exactly my position as well.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin updated this revision to Diff 80305.
ervin added a comment.


  Fix issues found with Cyril's patches on various KCMs.

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=78698=80305

REVISION DETAIL
  https://phabricator.kde.org/D27840

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

[Kevin Ottens]

ervin added a comment.


  Agreed, nullptr is going to be the boolean flag of our time, before it was 0 
though, so still an improvement. ;-)
  
  More seriously, here I'm not sure how to avoid it, at least it's a case of 
"if you feel like passing nullptr here you might be doing something wrong".

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28742

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28742

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

[Kevin Ottens]

ervin added a comment.


  In D28742#646050 , @dfaure wrote:
  
  > In D28742#646009 , @kossebau 
wrote:
  >
  > > And perhaps could be defaulted to nullptr, for use-cases which do not 
have a window at hand and are fine with any default?
  >
  >
  > I've been wondering. But people tend to forget to do so, and in most cases, 
if we choose the dialog delegate, then there's a QWidget based window somewhere.
  >  Plasma uses KNotificationJobUiDelegate so it's not a problem here.
  >  My thinking is that I'd rather force people to think about it, and 
possibly pass a nullptr in case there's really no window around.
  
  
  +1, I don't think defaulting to nullptr is a good idea.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28742

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.


  Indeed, good point.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28742

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28753: Add KNotificationJobUiDelegate(KJobUiDelegate::Flags) constructor

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28753

To: dfaure, broulik, davidedmundson, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28742

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28743: Port kruntest to ApplicationLauncherJob

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28743

To: dfaure, broulik, davidedmundson, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28742

To: dfaure, broulik, davidedmundson, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28741: [KJobUiDelegate] Add AutoHandlingEnabled flag

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28741

To: dfaure, broulik, davidedmundson, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin updated this revision to Diff 79656.
ervin marked 6 inline comments as done.
ervin added a comment.


  Addresses David's comments

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=78703=79656

REVISION DETAIL
  https://phabricator.kde.org/D27540

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin marked 6 inline comments as done.
ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kconfigdialogmanager.cpp:609
> Why not item->readDefault()?

Wouldn't do the same thing at all. readDefault() takes a KConfig object and 
updates the default value stored in the item with what it found in there.

Yes, I know... the item API is weird...

> davidedmundson wrote in kconfigdialogmanager.cpp:625
> won't it do it itself when the property changes?

Good point, was indeed unnecessary now, I removed the line.

> davidedmundson wrote in settingsstatusindicator.cpp:75
> > This is equivalent to calling showFullScreen(), showMaximized(), or 
> > setVisible(true), depending on the platform's default behavior for the 
> > window flags.
> 
> For X and wayland it's setVisible(true)
> 
> but we shouldn't count on it.

I don't think it matters for widgets which have a parent and not the Qt::Window 
window flag, but OK, switching to setVisible() instead of show/hide.

> davidedmundson wrote in settingsstatusindicator.cpp:175
> unused?

I'm not sure how you end up to this conclusion, it's written two below if we 
are at the window edge, and it's used in the move call at the end.

> davidedmundson wrote in settingsstatusindicator.cpp:184
> Can we be sure the tracked widget always has a parent widget?
> 
> If someone doesn't use layouts a widget might not have a parent.

Well that'd mean the tracked widget is a window... it's pretty much an 
impossibility IMO.

> davidedmundson wrote in settingsstatusindicator.cpp:192
> that's not true for the RTL case where the widget is expected to resize.
> 
> It would be   w->pos().x() + w->width() - widgetExpectedWidth(w)

Either I misunderstood what you meant or you got the math wrong on that one.

What you're proposing (or what I understood you're proposing) breaks the "RTL + 
widget at edge" case in my tests. The line I wrote is working perfectly fine 
for my tests with desktoppath and qtquicksettings both in LTR and RTL modes.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#638985 , @ndavis wrote:
  
  > Somehow I missed the notification that this was updated.
  >
  > Thanks for the horizontal alignment. Could you also add a left margin to 
the column of reset buttons? It should be the same as the spacing between the 
labels and the controls, which is equivalent to `Kirigami.Units.smallSpacing`, 
IIRC.
  
  
  Actually was already the case, forgot to update the screenshot.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

Re: Problems in KWayland causes by API and ABI compatibility promises

[Kevin Ottens]

Hello,

On Wednesday, 1 April 2020 14:04:10 CEST David Edmundson wrote:
> Here is a list of active uses of the KWayland::Client API.
> 
> frameworks
> plasma-framework (for window positioning)
> 
> apps:
> spectacle (for window positioning)
> kdeconnect-kde  (for fake input)
> yakuake (for window positioning)
> 
> extragear
> latte-dock (for window positioning, custom shadow (which could be
> ported already) and windowmanagement)

The part of the list above makes me wonder, shouldn't the window positioning 
and windowmanager features be on KWindowSystem?

I got no idea regarding kdeconnect-kde and the fake input...
 
> workspace:
> kwin unit tests
> libkscreen
> breeze (till Qt5.15)
> oxyen (till Qt5.15)
> xdg-desktop-portal
> kinfocenter
> plasma-workspace
> plasma-nano
> kscreenlocker
> powerdevil
> kwayland-integration (the backend for kwindowsystem)
> plasma-phone-components
> plasma-integration

I think the above is less of an issue, right? Because workspace would have a 
copy of KWayland which could be shared with whatever stability constraints 
needed.

Hope that helps. :-)

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

D28460: Add KCModuleStateProbe as base class for plugin

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcmoduleloader.cpp:161
> +if (!mod.service() || mod.service()->noDisplay() || 
> mod.library().isEmpty())
> +{
> +return true;

Curly brace should be on the previous line

> kcmoduleloader.cpp:167
> +args2.reserve(args.count());
> +for (const QString  : args) {
> +args2 << arg;

Wouldn't it be better to initialize args2 with arg iterators?
i.e. `QVariantList args2(arg.cbegin(), arg.cend());`

> kcmodulestateprobe.cpp:47
> +
> +KCModuleStateProbe::~KCModuleStateProbe() {
> +delete d;

Curly brace on the next line

> kcmodulestateprobe.cpp:55
> +
> +void KCModuleStateProbe::registerSkeleton(KCoreConfigSkeleton *skeleton) {
> +if (!skeleton || d->_skeletons.contains(skeleton)) {

Curly brace on the next line

> kcmodulestateprobe.h:44
> +
> +virtual void virtual_hook(int id, void *data);
> +

Should be protected not public

> broulik wrote in kcmodulestateprobe.h:39
> Please add a `virtual_hook` so we can extend this class in the future without 
> breaking ABI should we have the need to extract more data out of a settings 
> module:
> 
>   virtual void virtual_hook(int id, void *data)

I'd slightly disagree here though, if that inherits from QObject anyway I'd 
just rely on meta call dispatching. But OK, let's go virtual_hook.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28460

To: bport, #plasma, ervin
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Kevin Ottens]

ervin added a comment.


  LGTM but @dfaure concerns need to be addressed.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28128: Add force save behavior to KEntryMap

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28128

To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

Re: New Framework Review: KDAV

[Kevin Ottens]

Hello,

On Saturday, 9 November 2019 12:33:54 CEST Volker Krause wrote:
> during Akademy there was a request to promote KDAV from KDE PIM to
> Frameworks for use by Plasma Mobile. KDAV is a framework that implements
> the CalDav/ CardDav/GroupDav protocol on top of KIO's WebDav support. It
> would be classified as a functional tier 3 framework.
> 
> So far we have fixed a number of obvious ABI-compatibility issues, removed
> QtXml[Patterns] usage from the public interface and relicensed GPL parts
> (apart from a bit of test code) to LGPL. The next step would be a more
> thorough review to identify changes necessary before becoming a Framework.
> 
> To avoid the last minute invasive changes we ended up doing for
> KCalendarCore, I'd propose the following timeline:
> 
> - identify and implement all necessary changes to the API and ABI until the
> 20.04 Application release (that includes the still necessary move to the KF5
> library namespace).

I'm likely late to the party, but here is what I found by looking at KDAV 
master today (first day of the KDE PIM sprint):
 * There's a few private methods or Q_SLOTS that I'd hide completely by moving 
them to the d-pointer, for the slots we're using type safe connects so they 
don't even need to be marked as slots at all;
 * Is it worth making DavCollection moveable? It's only copyable right now;
 * We might want to do something about "ctag" in DavCollection it's a bit 
obscure as a name (and the API doc doesn't help), also it seems to not be an 
official standard (while being widely supported) and there's the sync-token 
mechanism which has a RFC (RFC6578);
 * Why isn't DavCollectionModifyJob using DavCollection somehow? (might just 
be my ignorance but I find it surprising that it is solely based on a property 
mechanism);
 * DavCollections(Multi)FetchJob has a mysterious "protocol" parameter on its 
collectionDiscovered signal, is it really necessary? if it has to stay, 
shouldn't be at least documented? or at least a safer type than int?
 * DavCollectionsMultiFetchJob is inconsistent as it's not using 
Q_DECLARE_PRIVATE;
 * KDAV::Error would benefit from more apidox;
 * Is it worth making DavItem moveable? It's only copyable right now;
 * Same comment about etag for DavItem than the ctag one for DavCollection
 * I'd be tempted to move all the protected methods of DavJobBase on its d-
pointer, the job subclasses would have access to them anyway, it'd make sense 
to put them protected in the header only if we expect subclasses outside of 
the lib (and I doubt this is actually supported);
 * It needs to decide between Qt smart pointers or STL ones I think, found a 
bit of both so far (I'd lean toward STL ones but maybe that's just me);
 * Make DavUrl moveable?
 * EtagCache probably shouldn't have anything protected, also, why is it a 
QObject at all?
 * Are we sure we want to return a QLatin1String in ProtocolInfo? this strike 
me as an odd choice.

Overall apidox would likely need a big pass of cleanups as well.

I think that's it from me.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin updated this revision to Diff 78703.
ervin added a comment.


  As advised by Kai and David on D27840 , 
switch to using tool buttons and fix RTL handling.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=78468=78703

REVISION DETAIL
  https://phabricator.kde.org/D27540

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a reviewer: broulik.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin updated this revision to Diff 78698.
ervin marked 10 inline comments as done.
ervin added a comment.


  Addresses Kai and David comments.

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=78469=78698

REVISION DETAIL
  https://phabricator.kde.org/D27840

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin marked 15 inline comments as done.
ervin added inline comments.

INLINE COMMENTS

> broulik wrote in SettingStateBinding.qml:58
> This description didn't really help me in understanding what it does until I 
> read the code further down.

Went for something much more verbose, but at least it says what it does.

> broulik wrote in SettingStateBinding.qml:75
> Can you explain this.
> Can't you
> 
>   indicatorComponent.createObject(target, {
>   settingsState: settingsState
>   });

Good point.

> broulik wrote in SettingStateBinding.qml:91
> We could also bind `parent: target` instead?

Tried it and QQuickItem was very angry at me, I didn't push further. :-)

> broulik wrote in SettingStateBinding.qml:97
> Please check if this works with right-to-left languages (run with `-reverse` 
> argument to test)

Again that's one of those comments I'd have appreciated on the widgets version 
a little while ago. ;-)

> davidedmundson wrote in SettingStateIndicator.qml:30
> This is a faux-button.
> 
> Which means it needs all the relevant:
> Accessible.blah
> 
> roles to be manually added
> 
> Should it be in the tab focus chain?
> 
> (or we could just inherit ToolButton)

Note this is the case for the QtWidgets implementation as well. I switched to 
ToolButton here but I likely need to do something similar in my other patch... 
would have been nice to have this comment earlier there (it's been ready a bit 
longer), you guys have a bias against widgets based patches. ;-)

I think it shouldn't temper with focus at all though, this is almost impossible 
to get correct in that context I think, so it'll simply reject focus (not too 
much of an issue in that context IMHO).

> broulik wrote in SettingStateIndicator.qml:40
> Perhaps bind to `icon.valid`?

Done completely differently with ToolButton anyway.

> broulik wrote in SettingStateIndicator.qml:48
> You could make the `root` `Item` a `MouseArea` instead.
> Also, I think this should get some kind of hover and/or pressed indication?
> Also, what about `Accessible` and keyboard focus? Should this be a proper 
> `ToolButton` control instead?

See my reply to @davidedmundson above, I switched to ToolButton.

> broulik wrote in settingstatebindingprivate.cpp:30
> Odd. Wouldn't we just get `QQuickRowLayout` et al? Or is that if you do a 
> custom item with a `Layout` as a base?

Or if you make something like Kirigami.FormLayout which has Item as base... 
obviously it's mostly an heuristic so can't be 100% perfect.

> broulik wrote in settingstatebindingprivate.cpp:82
> Too bad `QQuickItemChangeListener` is private.
> 
> Also, does any of this need event compression?

That really didn't feel like it required event compression to be honest at 
least I couldn't perceive any visible impact.

> broulik wrote in settingstatebindingprivate.cpp:90
> Check if it actually changed before emitting

Well it will have changed in 90% of the cases and checking means doing the 
computation anyway, which will happen when the getter is called. We'd pay the 
price twice.

An option is of course to cache the result of the getter to the price of higher 
complexity. Again I didn't perceive any visible impact on use. Should I go for 
it anyway?

Note I'm not at all denying this implementation is in some way inefficient, I'm 
just pointing out the trade off in term of readability and maintainability of 
the system.

> broulik wrote in settingstatebindingprivate.cpp:107
> Check if it actually changed before emitting

Same answer than the previous such case.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D27840

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.

INLINE COMMENTS

> kconfigskeletontest.cpp:25
>  
> +static inline QString kdeGlobalsPath() {
> +return 
> QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
> "/kdeglobals";

{ should be on its own line

> kconfigskeletontest.cpp:29
> +
> +static inline QString kdeSystemGlobalsPath() {
> +return 
> QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
> "/system.kdeglobals";

ditto

> kconfigskeletontest.cpp:190
> +{
> +// This test ensure we don't override default value when this one came 
> from a file
> +// system.kdeglobals is a global file and can provide default

s/ensure/ensures/

> kconfigskeletontest.cpp:218
> +QCOMPARE(item->value(), 30);
> +}

More comments welcome in that test as well.

> kcoreconfigskeleton.cpp:13
>  #include 
> +#include 
>  

Looks like this include is unused

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28128: Add force save behavior to KEntryMap

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> kconfigtest.cpp:1953
> +
> +void KConfigTest::testKdeglobalsVSDefault()
> +{

Seeing how this test confuses everyone (including me and I knew the problem 
before hand...), I think it'd benefit greatly from getting comments at the most 
important points in its execution (similarly to other tests in that file).

> kconfigtest.h:76
>  
> +void testKdeglobalsVSDefault();
> +

nitpick: should be "Vs"

> kconfigdata.cpp:105
> +if (e.bGlobal && !(options & EntryGlobal) && !k.bDefault)
> +{
> +e.bOverridesGlobal = true;

Should be at the end of the previous line

> kconfigdata.h:63
> +/**
> + * Entry will need to be written on a not global file even if it matches 
> default value
> + */

nitpick: I think I'd write "non global" rather than "not global"

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28128

To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

Re: Problems in KWayland causes by API and ABI compatibility promises

[Kevin Ottens]

nt and copy it to kwin (likely 
with a rename to avoid symbol clashes);
 6) mark all of kwaylandserver API deprecated and freeze it;
 7) when KF6 gets branched drop all the deprecated APIs.

Again, I'm no Wayland expert so I might be missing some things. In any case: 
yes, this is quite some work and might incur some pain and trade-offs... it's 
unfortunately a price to pay to get out of what seems like a difficult 
situation which will lead to increasing pains over time.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#629380 , @ervin wrote:
  
  > In D27540#629362 , @ndavis wrote:
  >
  > > Is it possible to align all of the reset buttons like a column?
  >
  >
  > Why I'm not surprised. ;-)
  >
  > Honestly with enough code it might be possible, but that'd be expensive in 
term of effort... I'll need to keep track of all widgets in the page. I'll give 
it a shot but don't hold your breath.
  
  
  Alright, turns out I managed to do it both for QtWidgets and QtQuick cases, 
so you can breath again. ;-)
  
  Reviews on all my patches very welcome.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin updated this revision to Diff 78469.
ervin added a comment.


  Have the indicators line up vertically automatically when applicable

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=77848=78469

REVISION DETAIL
  https://phabricator.kde.org/D27840

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
  src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin updated this revision to Diff 78468.
ervin added a comment.


  Have the indicators vertically line up automatically

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=77847=78468

REVISION DETAIL
  https://phabricator.kde.org/D27540

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#629362 , @ndavis wrote:
  
  > Is it possible to align all of the reset buttons like a column?
  
  
  Why I'm not surprised. ;-)
  
  Honestly with enough code it might be possible, but that'd be expensive in 
term of effort... I'll need to keep track of all widgets in the page. I'll give 
it a shot but don't hold your breath.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin updated this revision to Diff 77848.
ervin added a comment.


  Take feedback about the GUI into account

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27840?vs=76952=77848

REVISION DETAIL
  https://phabricator.kde.org/D27840

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin edited the summary of this revision.
ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin updated this revision to Diff 77847.
ervin added a comment.


  Take feedback about the GUI into account

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=76362=77847

REVISION DETAIL
  https://phabricator.kde.org/D27540

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#627618 , @ndavis wrote:
  
  > Some extra rules I thought of:
  >
  > - With the checkable label example in the mockup above, it should reset 
both the label and the checkbox.
  
  
  Just for the record, this will unlikely to be enforceable on the framework 
side of the code, likely to be taken care of on a case by case basis when we 
encounter those in the modules themselves. Just managing expectations on what 
the code can easily do at that level of abstraction. So don't look for it in 
the patches I submit around the topic. ;-)
  
  The rest should be doable I'll update my patches shortly.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27133

To: crossi, ervin, dfaure, #frameworks
Cc: apol, meven, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27724: Syncronise setNeedsSave between KCModule and ConfigModule in both directions

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  What @meven said about the typo, otherwise LGTM indeed.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D27724

To: davidedmundson, ervin
Cc: ervin, meven, iasensio, crossi, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D27502

To: bport, #plasma, ervin, crossi, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#617485 , @ervin wrote:
  
  > And now you got a screenshot as well. Waiting for further feedback now.
  
  
  Ping? This patch was first created three weeks ago now.
  
  Note that look in D27840  and D27841 
 is pretty much the same than in here for 
which I provided screenshot

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin accepted this revision.

REPOSITORY
  R237 KConfig

BRANCH
  arcpatch-D27463_2

REVISION DETAIL
  https://phabricator.kde.org/D27463

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.
This revision is now accepted and ready to land.


  LGTM, please fix the typo in the docs before pushing though

INLINE COMMENTS

> README.dox:372
>  
> +Since 5.68, if present the value attribute will be used used as the 
> choice value written to the backend 
> +instead of the name, allowing to write text incompatible with enum 
> naming.

typo: it says "used used"

REPOSITORY
  R237 KConfig

BRANCH
  arcpatch-D27463_2

REVISION DETAIL
  https://phabricator.kde.org/D27463

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27133

To: crossi, ervin, dfaure, #frameworks
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin added a comment.


  In D27840#622937 , @ngraham wrote:
  
  > This patch doesn't apply on top of KDeclarative for me:
  >
  >   This diff is against commit 3d8757d5dfea2360304e2c8e7d0d575d04b00735, 
but
  >   the commit is nowhere in the working copy. Try to apply it against the
  >   current working copy state? (a1282da765c1b909d03d6c94eb77fd99e4374d74)
  >   [Y/n] y
  >  
  >   Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.h...
  >   Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp...
  >   Checking patch src/qmlcontrols/kcmcontrols/qml/qmldir...
  >   Checking patch 
src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml...
  >   Checking patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml...
  >   Checking patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp...
  >   Checking patch src/qmlcontrols/kcmcontrols/CMakeLists.txt...
  >   error: while searching for:
  >  
  >   set(kcmcontrols_SRCS
  >   kcmcontrolsplugin.cpp
  >   )
  >  
  >   add_library(kcmcontrolsplugin SHARED ${kcmcontrols_SRCS})
  >  
  >   error: patch failed: src/qmlcontrols/kcmcontrols/CMakeLists.txt:2
  >   Hunk #2 succeeded at 12 (offset -1 lines).
  >   Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.h cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/qml/qmldir cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml 
cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml 
cleanly.
  >   Applied patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp cleanly.
  >   Applying patch src/qmlcontrols/kcmcontrols/CMakeLists.txt with 1 reject...
  >   Rejected hunk #1.
  >   Hunk #2 applied cleanly.
  >  
  >Patch Failed! 
  >
  
  
  Ah right, sorry it depends on D27839 .

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D27840

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27811: [KConfigGui] Check font weight when clearing styleName property

[Kevin Ottens]

ervin added a comment.


  In D27811#622916 , @ahmadsamir 
wrote:
  
  > In D27811#622885 , @ervin wrote:
  >
  > > This styleName thing is an endless amount of fun... The patch LGTM, I'll 
let others weight in though since it can have ramifications I might miss.
  >
  >
  > Well this one is the tip of the ice berg, the berg itself was 
a2774ff5b41987c3919a9e 
 :)
  
  
  Yeah I know but we still need to account for this Qt "addition" at other 
places. It wasn't exactly transparent.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27811

To: ahmadsamir, #frameworks, dfaure, davidedmundson, cfeck, ervin, meven, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27833: Add an accessor to get the last loaded value for KConfigSkeletonItem

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  In D27833#622353 , @apol wrote:
  
  > What's the use-case?
  
  
  Having an idea of the patch you want to build on top of this would indeed 
help.

INLINE COMMENTS

> kcoreconfigskeleton.cpp:210
> +Q_D(KConfigSkeletonItem);
> +d->mLoadedValueImpl= impl;
> +}

Missing space before =

> kcoreconfigskeleton.cpp:226
> +Q_D(const KPropertySkeletonItem);
> +return QVariant::fromValue(d->mLoadedValue);
> +});

This is already a QVariant

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27833

To: meven, ervin, bport, crossi, #frameworks
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27811: [KConfigGui] Check font weight when clearing styleName property

[Kevin Ottens]

ervin added a comment.


  This styleName thing is an endless amount of fun... The patch LGTM, I'll let 
others weight in though since it can have ramifications I might miss.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27811

To: ahmadsamir, #frameworks, dfaure, davidedmundson, cfeck, ervin, meven, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcoreconfigskeleton.cpp:581
> +// HACK for BC concerns
> +// TODO KF6: remove KCoreConfigSkeletonPrivate::mValues and add a value 
> field to KCoreConfigSkeleton::ItemEnum::Choice
> +const auto inHash = d_ptr->mValues.value(name);

You mean KConfigSkeletonItemPrivate aren't you? (instead of 
KCoreConfigSkeletonPrivate)

> meven wrote in kcoreconfigskeleton.h:788
> I expect those to be found through grep, and I had comments in the past to 
> put it somewhere where it could not get in documentation, in cpp guarantees 
> this.

Well, regular comments don't go in the docs ;-)
(you need the triple slash or the double start at start of comment for it to be 
picked up by doxygen)

But cpp, why not.

> KConfigCommonStructs.h:60
> +
> +QString value() const {
> +return !val.isEmpty() ? val : name;

New line before opening curly brace please

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27463

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27811: [KConfigGui] Check font weight when clearing styleName property

[Kevin Ottens]

ervin added a reviewer: bport.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27811

To: ahmadsamir, #frameworks, dfaure, davidedmundson, cfeck, ervin, meven, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kconfigcompiler_test.cpp:80
>  "test_signal",
> +"test_notifiers",
>  "test_translation_kde",

This seems unrelated... So we had this test case available but it was in fact 
never run?

> test_subgroups.kcfg:10
> +
> +
> +

Now that I see it, I think I'd go for "parentGroupName" since this is not 
referential and really about the name (like the name parameter)

> test_subgroups.kcfg:20
> +
> +

Probably worth also having cases with:

- the group name being parameterized but not the parentGroup
- both name and parentGroup not be parameterized

Just to have all the possible combinations.

> KConfigSourceGenerator.cpp:354
>  
> +// TODO : Some compiler option won't work or generate bogus settings file.
> +// * Does not manage properly Notifiers=true kcfgc option for parameterized 
> entries :

Unrelated right, this is not due to your patch, or am I confused?

> KConfigXmlParser.cpp:502
>  
> +QString parentGroup = e.attribute(QStringLiteral("parentGroup"));
> +

I'd const auto it, or at least const.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27133

To: crossi, ervin, dfaure, #frameworks
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in kcoreconfigskeleton.cpp:580
> You mean I should prepare this and put it in KF6 waiting for merge queue ?

No I meant the comment needs to be adjusted due to the changes (fields changing 
place and such) sorry if I was unclear.

> meven wrote in kcoreconfigskeleton.h:788
> I have one in `QString KCoreConfigSkeleton::ItemEnum::valueForChoice(QString 
> name) const`

Yeah, noticed only later... I wonder if it's more obvious in the header or the 
cpp...

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27463

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcoreconfigskeleton.cpp:580
> +{
> +// TODO KF6 move value to ItemEnum::Choice and remove 
> KCoreConfigSkeleton::ItemEnum::mValues
> +const auto inHash = mValues.value(name);

Will need an update

> kcoreconfigskeleton.cpp:586
> +/**
> + * Stores a choice value for name
> + */

This comment should go away

> kcoreconfigskeleton.h:788
> + */
> +QString valueForChoice(QString name) const;
> +

const QString 

Probably worth adding a KF6 comment somewhere as well, because your first 
attempt felt more natural, we're going for this weird construct only for BC 
concerns.

> kcoreconfigskeleton.h:793
> + */
> +void setValueForChoice(QString name, QString valueForChoice);
> +

const ref for the parameters

> kcoreconfigskeleton.h:797
>  QList mChoices;
> +QHash mValues;
>  };

Nope, can't be here, this still breaks binary compatibility. As I mentioned in 
my last comment it should be buried all the way into the d-pointer inherited 
from KConfigSkeletonItem... This is inefficient in term of memory load but it's 
the only option to keep BC.

> KConfigCommonStructs.h:108
>  Choices choices;
> +QHash enumValues;
>  QList signalList;

Well, on that side you should have kept the more natural value() method IMO.

> KConfigXmlParser.cpp:193
>  QList chlist;
> +const QRegularExpression choiceNameRegex = 
> QRegularExpression(QStringLiteral("\\w+"));
> +

nitpick: I'd const auto that one, but it's your call

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27463

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin added a dependent revision: D27841: Port desktoptheme, icons and 
workspace KCMs to SettingStateBinding.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D27840

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27841: Port desktoptheme, icons and workspace KCMs to SettingStateBinding

[Kevin Ottens]

ervin edited the summary of this revision.
ervin added reviewers: Frameworks, Plasma, VDG.
ervin added a dependency: D27840: Introduce SettingState* elements to ease KCM 
writing.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27841

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma, #vdg
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin edited the summary of this revision.
ervin added reviewers: Frameworks, Plasma.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D27840

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27839: Properly name the content of the kcmcontrols project

[Kevin Ottens]

ervin added reviewers: Frameworks, Plasma.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D27839

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, 
#frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27840: Introduce SettingState* elements to ease KCM writing

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: crossi, hchain, meven, bport, davidedmundson, mart, 
ngraham.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This is the QML based counterpart of D27540 
. Unlike with the KCModule
  case, this is not automatically propagated to the ConfigModules, they
  will all have to be adapted to make use of it.
  
  I will provide another patch which ports a few ConfigModule to see
  how it looks.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D27840

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt
  src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp
  src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
  src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
  src/qmlcontrols/kcmcontrols/qml/qmldir
  src/qmlcontrols/kcmcontrols/settingstateproxy.cpp
  src/qmlcontrols/kcmcontrols/settingstateproxy.h

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27839: Properly name the content of the kcmcontrols project

[Kevin Ottens]

ervin created this revision.
ervin added reviewers: crossi, hchain, meven, bport, davidedmundson, mart, 
ngraham.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  Looks like it was copied from dragandrop, finish to properly rename
  everything.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D27839

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/CMakeLists.txt

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27717: fix min/max entries with dpointer

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27717

To: hchain, meven, ervin
Cc: kde-frameworks-devel, aacid, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> settings.cpp:62
>  
> -bool Settings::setCheckUppercase(bool check)
> +void Settings::setSkipUppercase(bool check)
>  {

Rename the parameter to skip as well please

> settingsimpl.cpp:260
>  //same defaults are in the default filter (filter.cpp)
> -d->checkUppercase = settings.value(QStringLiteral("checkUppercase"), 
> true).toBool();
> -d->skipRunTogether = settings.value(QStringLiteral("skipRunTogether"), 
> true).toBool();
> +d->checkUppercase = settings.value(QStringLiteral("checkUppercase"), 
> Settings::defaultSkipUppercase()).toBool();
> +d->skipRunTogether = settings.value(QStringLiteral("skipRunTogether"), 
> Settings::defauktSkipRunTogether()).toBool();

I'd expect a negation here... if by default skip is false, check in 
settingsimpl should be true (which was the old default).

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D27502

To: bport, #plasma, ervin, crossi, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ervin wrote in kcoreconfigskeleton.h:764
> Oh right, stupid me, this obviously breaks binary compatibility, we need to 
> find another way to store this.

Most likely place to hide it would be inside KConfigSkeletonItem's d-pointer 
(likely a hash associating vals with names)... it'd be unused by most item 
types of course which sucks but at least it'd be safe BC wise.

> KConfigXmlParser.cpp:202
>  std::cerr << "Tag  requires attribute 'name'." << 
> std::endl;
> +} else if 
> (!(QRegularExpression(QStringLiteral("\\w+"))).match(choice.name).hasMatch()) 
> {
> +std::cerr << "Tag  attribute 'name' must be compatible 
> with Enum naming. name was '" << qPrintable(choice.name) << "'. You can use 
> attribute 'value' to pass any string as the choice value." << std::endl;

We should move the QRegularExpression out of the loop, otherwise it'll get 
compiled over and over for each choice (premature pessimisation imo). We could 
go one step further and have it as a member of the parser to have it compiled 
only once of course, but maybe we'd get in premature optimization territory.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27463

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in kcoreconfigskeleton.h:764
> Something I have noticed while testing this.
> Since it changes the memory of a very common data struct in a very common 
> lib, it creates a lot of crashes if apps are not compiled with the installed 
> version.
> So all local builds should be rebuild or reinstalled, once this has landed, 
> or you end up with a lot of crashes.

Oh right, stupid me, this obviously breaks binary compatibility, we need to 
find another way to store this.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27463

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27502: Create ConfigView an unmanaged ConfigWidget

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> settings.cpp:62
>  
> -bool Settings::setCheckUppercase(bool check)
> +void Settings::setCheckUppercase(bool check)
>  {

Please rename it to "skip uppercase"...

Otherwise it does the opposite of its name, it's just confusing

> settings.cpp:67
>  
>  bool Settings::checkUppercase() const
>  {

Likewise should be renamed to "skip"

> configview.cpp:43
> +
> +void ConfigViewPrivate::slotSelectionChanged() {
> +
> ui.removeButton->setEnabled(!ui.ignoreListWidget->selectedItems().isEmpty());

Break the line before { please

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D27502

To: bport, #plasma, ervin, crossi, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Kevin Ottens]

ervin added a comment.


  In D27540#617589 , @alexde wrote:
  
  > Does this patch also fix indirectly #274629?
  
  
  I doubt it, this bug report seems confused BTW... it was never broken on the 
kdelibs side but *some* dialogs have bugs which break the feature for them. 
Unrelated to this patch we'd been doing a big sweep on the systemsettings 
module to address among other thing the problems they might have in that 
department.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D27133: kconfig_compiler : generate kconfig settings with subgroup

[Kevin Ottens]

ervin added a comment.


  In D27133#617577 , @dfaure wrote:
  
  > I really have zero experience with this stuff, but you're asking for my 
opinion nonetheless, so you'll get it, as crappy as it might be :-)
  >
  > Your comment refers to "group separator" as if it was an existing notion in 
KConfig. Can you point me where? Grepping for '241' or for 'separator' gives 
nothing.
  >  Do KConfig files use this already? Or is this all kconfig_compiler 
specific?
  >  I told you, I'm clueless :-)
  
  
  Right, on the C++ side you'd have to look for \x1d

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27133

To: crossi, ervin, dfaure, #frameworks
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27497: Fix code generation for entries with min/max

[Kevin Ottens]

ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27497

To: hchain, meven, crossi, ervin, bport, tcanabrava
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27497: Fix code generation for entries with min/max

[Kevin Ottens]

ervin edited the summary of this revision.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D27497

To: hchain, meven, crossi, ervin, bport, tcanabrava
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin added a comment.


  In D27540#615160 , @ervin wrote:
  
  > In D27540#615156 , @ngraham 
wrote:
  >
  > > Please add screenshots to the Test Plan section for patches that change 
the UI. :) Also it would be nice to indicate how someone can test this. Which 
apps? System Settings? Where? How?
  >
  >
  > Well, second part I pointed out a few kcms you can test with. ;-)
  
  
  And now you got a screenshot as well. Waiting for further feedback now.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D27540

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

[Kevin Ottens]

ervin updated this revision to Diff 76362.
ervin added a comment.


  Handle visibility of the tracked widget, setters become slots, and add plural 
to "indicatorWidgets".

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=76089=76362

REVISION DETAIL
  https://phabricator.kde.org/D27540

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns