[Differential] [Commented On] D3702: kconfig_compiler: Use nullptr in generated code

dfaure added a comment. Hmm OK, it's green in CI now. I'm getting local failures around QFont but that seems like a change in Qt. FAIL! : KConfigTest::testComplex() Compared values are not the same Loc: [/home/dfaure/d/kde/src/5/frameworks/kconfig/autotests/kconfigguitest.cpp

[Differential] [Commented On] D3702: kconfig_compiler: Use nullptr in generated code

dfaure added a comment. Well, both ;) Martin's commit landed after the change to nullptr even though it was initially written before that change so it wasn't ready for it -> I just fixed it in https://commits.kde.org/kconfig/d3de5a2a79d2786207d0cfbbc9828e00dabd148d However I see mor

[Differential] [Commented On] D3702: kconfig_compiler: Use nullptr in generated code

dfaure added a comment. This change broke unittests, says the CI. https://build.kde.org/view/Frameworks%20kf5-qt5/job/kconfig%20master%20kf5-qt5/ REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D3702 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/e

[Differential] [Accepted] D3987: Use nullptr in all Frameworks (just diff in KIO shown here)

dfaure accepted this revision. dfaure added a reviewer: dfaure. dfaure added a comment. This revision is now accepted and ready to land. Change it all. One note about "what is a public header" -- the rule is that any private header (i.e. not installed) is called _p.h The rule isn't 100%

[Differential] [Accepted] D3690: Generate an instance with KSharedConfig::Ptr for singleton and arg

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig BRANCH kconfig-change-try2 REVISION DETAIL https://phabricator.kde.org/D3690 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: graesslin, #frameworks

[Differential] [Updated] D3548: Add begin/end insert/remove columns to RearrangeColumns

dfaure added a comment. Can you add a unittest too? This class is fully unittested in krearrangecolumnsproxymodeltest.cpp, it's just that changing the source columns configuration "at runtime" wasn't supported. Just add one more test method, with spies for the signals you emit. Thanks!

[Differential] [Commented On] D3796: Fix Plasma-QPA filedialog to show wrong directory with QFileDialog::selectUrl()

dfaure added a comment. git log says this is qtbase commit 1a42124, from before v5.8.0-alpha1. So I'll check for 5.8 and use clone, thanks for the information. REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D3796 EMAIL PREFERENCES

[Differential] [Commented On] D3796: Fix Plasma-QPA filedialog to show wrong directory with QFileDialog::selectUrl()

dfaure added inline comments. INLINE COMMENTS > kdeplatformfiledialoghelper.cpp:368 > +// like file://, so we have to do it ourselves > +QSharedPointer opt(new > QFileDialogOptions(*options())); > +opt->setInitialDirectory(m_dialog->directory()); This does NOT build for me. qplatfo

[Differential] [Accepted] D3796: Fix Plasma-QPA filedialog to show wrong directory with QFileDialog::selectUrl()

dfaure accepted this revision. dfaure added inline comments. INLINE COMMENTS > kdeplatformfiledialoghelper.cpp:365 > + > +// Qt 5 at least until 5.7.0 does not derive the directory from the > passed url > +// and set the initialDirectory option accordingly, also not for known > schemes

[Differential] [Accepted] D3636: [kconfig_compiler] Improve documentation about Inherits

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig BRANCH inherits-readme-improvement REVISION DETAIL https://phabricator.kde.org/D3636 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: graesslin, #fr

[Differential] [Commented On] D3636: [kconfig_compiler] Improve documentation about Inherits

dfaure added inline comments. INLINE COMMENTS > README.dox:112 > + KConfigSkeleton and must provide a default constructor (kcfgfile not > specified), a constructor > + taking a QString argument kcfgfile with "name" attribute) and a > constructor taking a > + KSharedConfig::Ptr as argument (k

[Differential] [Updated] D3636: [kconfig_compiler] Improve documentation about Inherits

dfaure added a reviewer: mdawson. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D3636 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: graesslin, #frameworks, dfaure, mdawson Cc: nalvarez

[Differential] [Commented On] D3636: [kconfig_compiler] Improve documentation about Inherits

dfaure added inline comments. INLINE COMMENTS > nalvarez wrote in README.dox:111 > an ctor -> a ctor. > > Also, would it make sense to explain here what those arguments //are//, or is > that explained later? Also, "empty ctor" sounds like the body should be empty. I think you meant "a default

[Differential] [Commented On] D3530: Import plasma-workspace kioslaves

dfaure added a comment. In https://phabricator.kde.org/D3530#65873, @mart wrote: > desktop:/ should be probably conditionally built only on Linux(but in there is quite core), while, may make sense to actually kill applications:/ altogether?? I have no doubt it seems "core on linu

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

dfaure added a comment. Actually, I think Thiago's still waiting for a backtrace of all threads. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: albertvaka, mu

[Differential] [Accepted] D2546: Cleanup DBus-related resources before qApp exits

dfaure accepted this revision. This revision is now accepted and ready to land. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2546 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: albertvaka, #frameworks

[Differential] [Updated] D3530: Import plasma-workspace kioslaves

dfaure added a comment. remote:/ sounds very workspace-independent indeed, it sounds useful to have in kio. But desktop:/ and applications:/ make no sense in other workspaces (I'm surprised we even still have applications:/, it's kind of a toy, isn't it?). (ok applications:/ might m

[Differential] [Commented On] D3287: Make sure we don't break compilation with past broken units

dfaure added a comment. I knew everything in your last reply already ;) I'm not sure you understood my suggestion though. If someone writes File=foo, your code will output Couldn't read the \"File\" field while it would be better to output Broken \"File\" field, make s

[Differential] [Accepted] D3287: Make sure we don't break compilation with past broken units

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. The error message is a bit confusing, it sounds like the right file is ${_tmp_FILE}. So I would suggest this instead message(WARNING "${_tmp_FILE}: Broken \"File\" field, make sure

[Differential] [Accepted] D3251: Properly parse function keywords

dfaure accepted this revision. dfaure added a reviewer: dfaure. This revision is now accepted and ready to land. BRANCH master REVISION DETAIL https://phabricator.kde.org/D3251 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: apol, #frameworks, dfaure

[Differential] [Accepted] D3178: Make kconfig_compiler autotests use the KCONFIG_ADD_KCFG_FILES

dfaure accepted this revision. dfaure added a reviewer: dfaure. dfaure added a comment. This revision is now accepted and ready to land. Looks sensible to me. BRANCH master REVISION DETAIL https://phabricator.kde.org/D3178 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/em

[Differential] [Updated] D2546: Cleanup DBus-related resources before qApp exits

dfaure added a comment. Same as in https://phabricator.kde.org/D2545, I think this should be fixed in Qt instead. At least let's see what Thiago says. REVISION DETAIL https://phabricator.kde.org/D2546 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfu

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

dfaure added a comment. We need a backtrace with all threads to understand what's happening. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: arrowdodger, #fram

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

dfaure added a comment. So what is the actual deadlock you're experiencing? I showed this to Thiago and he said he needs more info on what is actually happening, i.e. where exactly it deadlocks in QtDBus. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERE

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

dfaure added a comment. Hmm can you check if https://codereview.qt-project.org/161056 fixes it? BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: #frameworks

[Differential] [Accepted] D2545: Cleanup KSharedUiServerProxy before qApp exists

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. I would prefer if we could fix these kind of issues in Qt itself, but I've had a difficult time doing that (*), this stuff is tricky. (*) for other dbus-thread related problems, not nec

[Differential] [Accepted] D2142: KGlobalAccel: Fix deadlock on exit under Windows

dfaure accepted this revision. dfaure added a comment. Looks good to me. I don't think we have the need to use the singleton from other global destructors. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2142 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emai

[Differential] [Accepted] D1942: [KIconDialog] Do not clear search line when switching category

dfaure accepted this revision. dfaure added a reviewer: dfaure. This revision is now accepted and ready to land. REVISION DETAIL https://phabricator.kde.org/D1942 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: cfeck, #frameworks, htietze, colomar, dfaure _

[Differential] [Accepted] D1862: Various minor cleanups in Kross

dfaure accepted this revision. dfaure added a reviewer: dfaure. dfaure added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > CMakeLists.txt:9 > + if (NOT EXISTS ${_INCDIR}/ui/${_KROSSUIHEADER}) > + file(WRITE ${_INCDIR}/ui/${_KROSSUIHEADER} "#inc

[Differential] [Accepted] D1861: Fix name of QDialogButtonBox's enumerator "StandardButtons"

dfaure accepted this revision. dfaure added a reviewer: dfaure. dfaure added a comment. This revision is now accepted and ready to land. (lack of repo) --> I now understand (from Ben) that this means phabricator isn't ready yet for being used for KF5 reviews. Let's use reviewboard for now (fo

[Differential] [Closed] D1924: KXMLGui: Fix merge indices when removing xmlgui clients with actions in groups.

dfaure closed this revision. dfaure marked an inline comment as done. REVISION DETAIL https://phabricator.kde.org/D1924 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, svuorela, dhaumann Cc: kde-frameworks-devel _

[Differential] [Updated] D1924: KXMLGui: Fix merge indices when removing xmlgui clients with actions in groups.

dfaure marked 2 inline comments as done. dfaure added inline comments. INLINE COMMENTS > dhaumann wrote in kxmlgui_unittest.cpp:305 > Interesting, this looks as if there were invalid reads before? Not exactly. QList's [i] asserts when called out of bounds. It didn't happen before, because when

[Differential] [Changed Subscribers] D1924: KXMLGui: Fix merge indices when removing xmlgui clients with actions in groups.

dfaure added a subscriber: kde-frameworks-devel. REVISION DETAIL https://phabricator.kde.org/D1924 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, svuorela, dhaumann Cc: kde-frameworks-devel ___ Kde-framew

[Differential] [Closed] D1902: KRun: add runApplication method.

dfaure closed this revision. REVISION DETAIL https://phabricator.kde.org/D1902 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, hein, broulik, davidedmundson, svuorela Cc: #plasma, #frameworks ___ Kde-frame

[Differential] [Updated] D1902: KRun: add runApplication method.

dfaure added a reviewer: davidedmundson. REVISION DETAIL https://phabricator.kde.org/D1902 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, hein, broulik, davidedmundson Cc: #plasma, #frameworks ___ Kde-fra

[Differential] [Updated] D1902: KRun: add runApplication method.

dfaure removed a reviewer: Plasma. dfaure added subscribers: Frameworks, Plasma. REVISION DETAIL https://phabricator.kde.org/D1902 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, hein, broulik Cc: #plasma, #frameworks ___

[Differential] [Accepted] D1863: Remove the first attempt to load library because we will try libraryPaths anyway

dfaure accepted this revision. dfaure added a reviewer: dfaure. dfaure added a comment. This revision is now accepted and ready to land. Looks good, assuming that libname is never an absolute path did you check that? BRANCH remove-dupl-load-library REVISION DETAIL https://phabricator

[Differential] [Closed] D1465: KFileItem: remove separate storage of times, use UDSEntry instead.

dfaure closed this revision. REVISION DETAIL https://phabricator.kde.org/D1465 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, Frameworks, aacid Cc: aacid ___ Kde-frameworks-devel mailing list Kde-framewor

[Differential] [Updated] D1465: KFileItem: remove separate storage of times, use UDSEntry instead.

dfaure added a reviewer: Frameworks. REVISION DETAIL https://phabricator.kde.org/D1465 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, Frameworks ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@