Re: KCGroups in KDEreview

2021-03-02 Thread 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

2021-01-30 Thread 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

2020-12-14 Thread 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

2020-12-03 Thread 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

2020-11-29 Thread 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

2020-06-26 Thread 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

2020-05-25 Thread 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

2020-05-23 Thread 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

2020-05-23 Thread 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

2020-04-21 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-20 Thread 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

2020-04-16 Thread 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

2020-04-16 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-11 Thread 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

2020-04-08 Thread 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

2020-04-08 Thread 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

2020-04-08 Thread 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

2020-04-08 Thread 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

2020-04-08 Thread 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

2020-04-07 Thread 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

2020-04-07 Thread 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

2020-04-07 Thread 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

2020-04-04 Thread 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

2020-03-27 Thread 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

2020-03-27 Thread 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

2020-03-27 Thread 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

2020-03-27 Thread 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

2020-03-27 Thread 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

2020-03-27 Thread 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

2020-03-27 Thread 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

2020-03-25 Thread 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

2020-03-25 Thread 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

2020-03-25 Thread 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

2020-03-25 Thread 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

2020-03-17 Thread 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

2020-03-17 Thread 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

2020-03-17 Thread 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

2020-03-17 Thread 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

2020-03-17 Thread 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

2020-03-17 Thread 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

2020-03-16 Thread 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

2020-03-16 Thread 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

2020-03-16 Thread 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

2020-03-13 Thread 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

2020-03-06 Thread 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

2020-03-06 Thread 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

2020-03-06 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-05 Thread 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

2020-03-04 Thread 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

2020-03-04 Thread 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

2020-03-04 Thread 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

2020-03-04 Thread 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

2020-03-04 Thread 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

2020-03-04 Thread 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

2020-02-28 Thread 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

2020-02-27 Thread 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

2020-02-26 Thread 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

2020-02-26 Thread 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

2020-02-25 Thread 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

2020-02-25 Thread 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

2020-02-25 Thread 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

2020-02-25 Thread 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

2020-02-25 Thread 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

2020-02-25 Thread 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

2020-02-25 Thread 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

2020-02-25 Thread 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


D27540: Status indicator for individual widgets in KCModule

2020-02-25 Thread Kevin Ottens
ervin added a comment.


  In D27540#615091 , @davidre wrote:
  
  > The editmarks should probably respect the visibility of the associated 
widget. In this picture I use a invisble lineedit to manage the settings of the 
QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)
  >  F8117874: grafik.png 
  
  
  For the record, next iteration of that patch will honor the visibility of the 
associated widget (got some more changes to do locally before uploading though, 
stay tuned).
  
  That being said, what you did is a very naughty hack you know. Why not fixing 
KConfigDialogManager or go through a QGroupBox (which is handled in some way). 
;-)

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


D27497: Fix code generation for entries with min/max

2020-02-24 Thread Kevin Ottens
ervin added a comment.


  A few smallish issues only, otherwise LGTM.

INLINE COMMENTS

> KConfigSourceGenerator.cpp:316
>  {
> -   stream() << "  " << itemPath(entry, cfg()) << " = "
> +QString innerItemVarStr(innerItemVar(entry, cfg()));
> +if (!entry->signalList.isEmpty()) {

I'd const it and also use the = style of initialization which I find more 
readable.

> KConfigSourceGenerator.cpp:354
> +QString argBracket = QStringLiteral("[%1]").arg(i);
> +QString innerItemVarStr = innerItemVar(entry, cfg()) + argBracket;
>  

const those please

> KConfigSourceGenerator.cpp:365
> +
> +QString itemVarStr(itemPath(entry, cfg()) + argBracket);
> +

ditto

> kconfig_compiler.cpp:397
>  {
> -if (cfg.itemAccessors) {
> -return QString();
> +QString type = cfg.inherits + "::Item" + itemType(e->type);
> +

const

> kconfig_compiler.cpp:401
> +fCap[0] = fCap[0].toUpper();
> +QString argSuffix = (!e->param.isEmpty()) ? 
> (QStringLiteral("[%1]").arg(e->paramMax + 1)) : QString();
> +QString result;

const

> kconfig_compiler.cpp:462
>  
> -QString newItem(const CfgEntry* entry, const QString , const QString& 
> defaultValue,
> +QString newItemInner(const CfgEntry *entry, const QString , const 
> QString ,
>  const KConfigParameters , const QString ) {

Should be named newInnerItem

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

2020-02-24 Thread Kevin Ottens
ervin requested changes to this revision.
This revision now requires changes to proceed.

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


D27502: Create ConfigView an unmanaged ConfigWidget

2020-02-24 Thread Kevin Ottens
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> loader.h:6
>   */
> -#ifndef SONNET_LOADER_P_H
> -#define SONNET_LOADER_P_H
> +#ifndef SONNET_LOADER_H
> +#define SONNET_LOADER_H

What's the reason for loader becoming public? If that's mostly out of 
convenience (it looks like it is), I'd advise not turning it public, Settings 
and ConfigView can use it internally but let's not increase the API surface 
even more with Loader.

> settings.cpp:13
>  #include 
> -#include 
>  #include 

Obviously will reappear here ;-)

> settings.cpp:109
>  {
> -if (d->checkUppercase != check) {
> +if (d->checkUppercase != !check) {
>  d->modified = true;

This is an odd bit of logic, isn't it?

> settings.cpp:119
>  {
> -return d->checkUppercase;
> +return !d->checkUppercase;
>  }

Ditto

> settings.h:9
> +
> +#include 
> +#include 

Please remove that include (it'll become more obvious why you'll be able to do 
it with one of the comments below) ;-)

> settings.h:33
> +
> +public:
> +~Settings() override;

Will need a ctor (see other comments) in the form of `explicit Settings(QObject 
*parent = nullptr);`

> settings.h:36
> +
> +Settings(const Settings &) = delete;
> +Settings =(const Settings &) = delete;

This is likely useless now, since we inherit from QObject and QObject is 
non-copyable

> settings.h:37
> +Settings(const Settings &) = delete;
> +Settings =(const Settings &) = delete;
> +

Ditto

> settings.h:79
> +// A static list of KDE specific words that we want to recognize
> +static QStringList defaultIgnoreList()
> +{

Move all the implementations of those static methods to the cpp file.

> settings.h:136
> +private:
> +void readIgnoreList();
> +bool setQuietIgnoreList(const QStringList );

Time to move those to the SettingsPrivate class.

> settings.h:141
> +friend class Loader;
> +explicit Settings(Loader *loader);
> +private:

Might end up disappearing completely, unsure but to be evaluated

> speller.cpp:216
>  case CheckUppercase:
> -d->settings->setCheckUppercase(b);
> +d->settings->setCheckUppercase(!b);
>  break;

And that negation again? We're negating twice now...

> speller.cpp:232
>  case CheckUppercase:
> -return d->settings->checkUppercase();
> +return !d->settings->checkUppercase();
>  case SkipRunTogether:

And again...

I wonder what's happening there, it's highly confusing.

> configview.cpp:33
> +layout->setObjectName(QStringLiteral("SonnetConfigUILayout"));
> +d->wdg = new QWidget(this);
> +d->ui.setupUi(d->wdg);

I'm kind of surprised at this extra widget, can't we get rid of it?

> configview.h:17
> +}
> +#include "sonnetui_export.h"
> +

Please move this line just after the `#include `

> configview.h:23
> +{
> +Q_OBJECT
> +public:

You might want to declare a bunch of Q_PROPERTY there

> configview.h:25
> +public:
> +explicit ConfigView(QWidget *parent, QStringList clients);
> +~ConfigView();

clients should be a const reference (although I suspect it'll disappear)

parent should be the last parameter and default to nullptr

> configview.h:26
> +explicit ConfigView(QWidget *parent, QStringList clients);
> +~ConfigView();
> +

override

> configview.h:30
> +
> +void setPreferredLanguages(const QStringList );
> +QStringList preferredLanguages() const;

All the setters there would make for good public slots

> configview.h:34
> +QString language() const;
> +void setIgnoreList(const QStringList& ignoreList);
> +QStringList ignoreList() const;

Space before & not after

> configview.h:39
> +void setBackgroundCheckingButtonShown(bool);
> +protected Q_SLOTS:
> +void slotIgnoreWordRemoved();

Are they directly called by a subclass outside the library? If not move them to 
the dpointer and use Q_PRIVATE_SLOT

> configview.h:42
> +void slotIgnoreWordAdded();
> +private Q_SLOTS:
> +void slotUpdateButton(const QString );

Move to the d pointer and use Q_PRIVATE_SLOT for those

> configview.h:55
> +ConfigViewPrivate *const d;
> +QStringList m_ignoreList;
> +};

Should be in the d pointer

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


D27059: KConfigSkeletonItem : allow to set a KconfigGroup to read and write items in nested groups

2020-02-24 Thread Kevin Ottens
ervin accepted this revision.

REPOSITORY
  R237 KConfig

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

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


D27059: KConfigSkeletonItem : allow to set a KconfigGroup to read and write items in nested groups

2020-02-24 Thread Kevin Ottens
ervin accepted this revision.
ervin added a comment.
This revision is now accepted and ready to land.


  LGTM, please don't forget to address dfaure's comment before pushing though.

REPOSITORY
  R237 KConfig

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

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


D27059: KConfigSkeletonItem : allow to set a KconfigGroup to read and write items in nested groups

2020-02-24 Thread Kevin Ottens
ervin added a comment.


  In D27059#612205 , @dfaure wrote:
  
  > Well, I'm not the KConfig maintainer, mdawson is :-)
  
  
  Well, my understanding is that mdawson is MIA, so I rely on the old "dfaure 
as default maintainer" model. ;-)

REPOSITORY
  R237 KConfig

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

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


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

2020-02-24 Thread Kevin Ottens
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcoreconfigskeleton.h:766
> +
> +public:
> +QString value() const {

It's a struct you can drop the public: here.

> kcoreconfigskeleton.h:767
> +public:
> +QString value() const {
> +return val.isEmpty() ? name : val;

There should be a new line before the opening curly brace.

I also wonder if it wouldn't be better with the implementation being moved to 
the cpp file, otherwise it risks being inlined which might not be the most 
convenient for longer term management of the change (if for some reason the 
implementation needs to be changed).

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

New line before opening curly brace please.

> KConfigXmlParser.cpp:203
>  }
> +else if (choice.name.contains(QLatin1Char(' ')) || 
> choice.name.contains(QLatin1Char('/')) || 
> choice.name.contains(QLatin1Char('.')) || 
> choice.name.contains(QLatin1Char(':'))) {
> +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;

else if should be just after the closing curly brace

As for checking the valid characters for an enum name this is "easy" it can 
only be alphabetical, numerical and underscore characters (technically 
shouldn't start with a digit). Any other character will be rejected by the 
compiler, the current filter is thus not discriminating enough at all and I 
think its logic should be reversed (the blacklist being potentially infinite it 
should be white list based).

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


D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens
ervin added a comment.


  In D27540#615132 , @davidre wrote:
  
  > They stick around because as you said the settings differ from the defaults.
  
  
  Right, so "not a bug" (as in that's what's the patch is intended to do so 
far).
  
  > The pen for me conveys the meaning that something was edited that's the 
reason I would only show it for unsaved changes. But let's others weigh in what 
they think
  
  Right, I picked visual indicators which came to mind, I'm totally open to 
suggestions on which would be better suited.

REPOSITORY
  R265 KConfigWidgets

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

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


  1   2   3   4   5   6   7   8   9   10   >