D26934: KCM/Autostart Add a model to separate logic from UI

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > autostart.cpp:167 > { > -KOpenWithDialog owdlg( this ); > -if (owdlg.exec() != QDialog::Accepted) > -return; > +KOpenWithDialog* owdlg =

D28462: [KCM] Add color state probe to allow system settings to display current default state

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > colorsstateprobe.cpp:29 > +: KCModuleStateProbe(parent, args) > +, m_settings(new ColorsSettings(this)) > +{ I'm not sure I see the point of doing

D28461: In sidebar mode show if a module is in default state or not

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > MenuItem.h:160 > + > +void updateDefault(); > + I don't like the name much, I think it could be confused with updating actual default value or such...

D28460: Add KCModuleStateProbe as base class for plugin

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

D28331: KCM/mouse KCM/touchpad: Add a Scroll speed setting for wayland

ervin accepted this revision. ervin added a comment. This revision is now accepted and ready to land. LGTM REPOSITORY R119 Plasma Desktop BRANCH arcpatch-D28331 REVISION DETAIL https://phabricator.kde.org/D28331 To: meven, #kwin, #plasma, davidedmundson, ervin, bport, crossi, hchain

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

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > broulik wrote in autostart.cpp:182 > Yes. > It then also needs to set a transient parent and window modality manual I > think Yes, otherwise you wouldn't get

D26934: KCM/Autostart Add a model to separate logic from UI

ervin added inline comments. INLINE COMMENTS > meven wrote in autostartmodel.h:52 > I see, I meant my code to have all Roles used explicitly declared here rather > than relying on the developer knowing about Qt::DisplayRole. Weeell... knowing about Qt::DisplayRole is kind of prerequisite to

Re: New Framework Review: KDAV

e 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.

Re: New Framework Review: KDAV

e 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

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

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

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,

D27840: Introduce SettingState* elements to ease KCM writing

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

D27840: Introduce SettingState* elements to ease KCM writing

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

D28331: KCM/mouse KCM/touchpad: Add a Scroll speed setting for wayland

ervin added inline comments. INLINE COMMENTS > main.qml:333 > +15, > +20]; > + nitpick: I'd put the closing square bracket on the next line and you can drop the semicolumn > main.qml:343 > +value = index; > +} > +

D26934: KCM/Autostart Add a model to separate logic from UI

ervin requested changes to this revision. ervin added a comment. This revision now requires changes to proceed. Damn turns out I still missed a few, so what @broulik says (when it still applies). @broulik : are you sure about all your comments? It looks like something odd happened, some

D26934: KCM/Autostart Add a model to separate logic from UI

ervin accepted this revision. ervin added a comment. This revision is now accepted and ready to land. Thanks for your patience :-) REPOSITORY R119 Plasma Desktop BRANCH D26934 REVISION DETAIL https://phabricator.kde.org/D26934 To: meven, mlaurent, ervin, #plasma, broulik, bport,

D26934: KCM/Autostart Add a model to separate logic from UI

ervin requested changes to this revision. ervin added a comment. This revision now requires changes to proceed. One last nitpick INLINE COMMENTS > autostartmodel.cpp:121 > +case PlasmaStart: > +return static_cast (index); > +default: Please remove the spurious space before (

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

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

D26934: KCM/Autostart Add a model to separate logic from UI

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > autostartitem.cpp:60 > +m_comboBoxStartup->addItem( AutostartModel::listPathName()[1], > AutostartEntrySource::PlasmaShutdown); > +

D28128: Add force save behavior to KEntryMap

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

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

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

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

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.

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

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,

D27840: Introduce SettingState* elements to ease KCM writing

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

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

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

D27063: Fix disabeling of autolock timeout

ervin added a comment. Ah didn't know! I assumed you could push... But yeah, you authored a few patches now, time to apply for a developer account. You can put my name indeed. REPOSITORY R133 KScreenLocker BRANCH fix_disabled_state REVISION DETAIL https://phabricator.kde.org/D27063

D27063: Fix disabeling of autolock timeout

ervin added a comment. Well, I already accepted it, I thought you pushed long ago (and I suspect it got overlooked by the other potential reviewers). :-) REPOSITORY R133 KScreenLocker BRANCH fix_disabled_state REVISION DETAIL https://phabricator.kde.org/D27063 To: alex, mart, ervin

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

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

D27840: Introduce SettingState* elements to ease KCM writing

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

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

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,

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

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,

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

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

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

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

D27133: kconfig_compiler : generate kconfig settings with subgroup

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

D27971: Solid-device-automounter/kcm: correctly update automountOn

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > DeviceAutomounterKCM.cpp:66 > > -auto emitChanged = [this] { > - > m_devices->setAutomaticMountOnLogin(kcfg_AutomountOnLogin->isChecked()); > -

D27956: [Various KCMs] Notify about changes in GTK related settings

ervin added a comment. In D27956#625419 , @ngraham wrote: > I'm finding myself wondering why we don't just make everything notify by default. IMHO it's mostly a question of limiting the chatter on the bus. REPOSITORY R119 Plasma

D27724: Syncronise setNeedsSave between KCModule and ConfigModule in both directions

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,

D27944: KCM Colors fix apply button always disabled

ervin added a comment. I marked it accepted, but of course this is assuming David's patch wouldn't make it quickly and we'd need the fix here ASAP. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D27944 To: crossi, #plasma, ervin, bport, meven, broulik Cc:

D27502: Create ConfigView an unmanaged ConfigWidget

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

D26934: KCM/Autostart Add a model to separate logic from UI

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > autostart.h:52 > +void updateDesktopStartItem(DesktopStartItem *item, const QString , > const QString , bool disabled, const QString ); > +void

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

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

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

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

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

D27133: kconfig_compiler : generate kconfig settings with subgroup

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

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

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

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

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

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

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

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,

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

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

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

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

D27155: libnotificationmanager : add app-specific kconfig settings

ervin accepted this revision. ervin added a comment. Although might become "parentGroupName" depending on what you do about my comments on the review introducing sub-group handling. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27155 To: crossi, ervin,

D27133: kconfig_compiler : generate kconfig settings with subgroup

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

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

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. >

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

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

D27840: Introduce SettingState* elements to ease KCM writing

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:

D27841: Port desktoptheme, icons and workspace KCMs to SettingStateBinding

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,

D27840: Introduce SettingState* elements to ease KCM writing

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,

D27839: Properly name the content of the kcmcontrols project

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,

D27841: Port desktoptheme, icons and workspace KCMs to SettingStateBinding

ervin created this revision. ervin added reviewers: crossi, hchain, meven, bport, davidedmundson, mart, ngraham. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. ervin requested review of this revision. TEST PLAN Has been tested with the three modules mentioned in the

D27840: Introduce SettingState* elements to ease KCM writing

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

D27839: Properly name the content of the kcmcontrols project

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

D27717: fix min/max entries with dpointer

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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

ervin added inline comments. INLINE COMMENTS > bport wrote in spellchecking.cpp:88 > Yes, or I can probably also use item like before, and that will be handled > for me That would create other issues I think (not fully sure TBH). Well in any case that's an extra level of indirection we don't

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

ervin added inline comments. INLINE COMMENTS > bport wrote in spellchecking.cpp:88 > no load only do findItem for managed widget on the skeleton Sure but still, the skeleton is expected to be already be in a "loaded" state at that point, that's why it works for the items (otherwise a findItem

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > ervin wrote in spellchecking.cpp:55 > What about calling toSet() on those? Not overly efficient but would compress > a bit the resulting code. Alternatively,

D27502: Create ConfigView an unmanaged ConfigWidget

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

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

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

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

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 >

D26934: KCM/Autostart Add a model to separate logic from UI

ervin added inline comments. INLINE COMMENTS > autostart.cpp:87 > > +setMinimumHeight(300); > +setMinimumWidth(400); Unrelated right? Where do this come from? > autostart.cpp:112 > > -void Autostart::addItem( DesktopStartItem* item, const QString& name, const > QString& run, const

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > spellchecking.cpp:55 > + > +auto referenceValue = m_settings->currentIgnoreList(); > +auto currentValue = m_configWidget->ignoreList(); What about

D27502: Create ConfigView an unmanaged ConfigWidget

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

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

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

D27133: kconfig_compiler : generate kconfig settings with subgroup

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

D27497: Fix code generation for entries with min/max

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

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

D27380: [GTK Config] Construct font style by hand instead of relying on Qt function

ervin added a comment. LGTM, now requires swift testing to not miss the next bugfix release REPOSITORY R99 KDE Gtk Configuration Tool REVISION DETAIL https://phabricator.kde.org/D27380 To: gikari, #plasma, ervin, bport, meven, davidedmundson Cc: chauvin, davidre, davidedmundson, cfeck,

D27540: Status indicator for individual widgets in KCModule

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

D27540: Status indicator for individual widgets in KCModule

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

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

D27540: Status indicator for individual widgets in KCModule

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

[frameworks-kconfig] [Bug 418146] kconfig_compiler with GenerateProperties creates uncompilable/wrong code

https://bugs.kde.org/show_bug.cgi?id=418146 --- Comment #2 from Kevin Ottens --- Could you test https://phabricator.kde.org/D27497 ? I think it will address your issue. -- You are receiving this mail because: You are watching all bug changes.

D27497: Fix code generation for entries with min/max

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

D27497: Fix code generation for entries with min/max

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

D27503: [KCM Spellchecking] port to KPropertySkeletonItem

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > spellchecking.cpp:30 > +#include > +#include > +#include Good opportunity to switch to the camel case includes? > spellchecking.cpp:40 >

D27502: Create ConfigView an unmanaged ConfigWidget

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

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

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

D27482: Update kdeglobals config file for Breeze widgetStyle

ervin added inline comments. INLINE COMMENTS > bport wrote in style_widgetstyle_default_breeze.pl:8 > This will fix only for Breeze theme > This will fix existing case but we also need to fix the root cause (i.e. how > we end up with a lowercase name). Sure but that would be a different patch

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

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,

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

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

D27380: [GTK Config] Construct font style by hand instead of relying on Qt function

ervin added a comment. A minor style nitpick, otherwise LGTM. As mentioned by the author, this likely require extensive testing before going in. INLINE COMMENTS > configvalueprovider.cpp:53 > > +QString ConfigValueProvider::fontStyleHelper(const QFont& font) const > +{ Space should be

D27024: Solid-device-automounter/kcm: show disconnected known device when disconnecting it

ervin accepted this revision. ervin added a comment. This revision is now accepted and ready to land. I guess extra care will need to be taken with your other patch moving away from the singleton (dunno which one is based on which, I might review out of order). REPOSITORY R119 Plasma

D27480: Solid-device-automounter/kcm: Get rid of singleton for AutomounterSettings

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > DeviceAutomounterKCM.cpp:44 > : KCModule(parent, args)//DeviceAutomounterKCMFactory::componentData(), > parent) > - , m_devices(new

D27127: Solid-device-automounter/kcm: Use KConfigXT in ui

ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > DeviceAutomounterKCM.cpp:44 > : KCModule(parent, args)//DeviceAutomounterKCMFactory::componentData(), > parent) > + , m_devices(new

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

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. >

D27540: Status indicator for individual widgets in KCModule

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

D27540: Status indicator for individual widgets in KCModule

ervin added a comment. 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?

D27540: Status indicator for individual widgets in KCModule

ervin added a comment. In D27540#615105 , @davidre wrote: > In D27540#615092 , @ervin wrote: > > > In D27540#615061 , @davidre wrote: > > > > > How

D27540: Status indicator for individual widgets in KCModule

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

<    1   2   3   4   5   6   7   8   9   10   >