[Differential] [Commented On] D3987: Use nullptr in all Frameworks (just diff in KIO shown here)
kfunk added inline comments. INLINE COMMENTS > graesslin wrote in job.h:50 > Question: is this change API and ABI stable? Definitely ABI stable, since default arguments are not part of the function signature. I'm not aware this could break API compat either (well, only in case the compiler consuming this code is not C++11-compliant)... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3987 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks Cc: graesslin
[Differential] [Updated] D3987: Use nullptr in all Frameworks (just diff in KIO shown here)
kfunk updated the summary for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3987 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Updated] D3987: Use nullptr in all Frameworks (just diff in KIO shown here)
kfunk updated the summary for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3987 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Request, 2,027 lines] D3987: Use nullptr in all Frameworks (just diff in KIO shown here)
kfunk created this revision. kfunk added a reviewer: Frameworks. kfunk set the repository for this revision to R241 KIO. Restricted Application added a project: Frameworks. REVISION SUMMARY The full patch (all Frameworks ported to using nullptr instead of null literals) changes around 9000 lines in total: This is what I use locally to keep track of my changes: % kde-frameworks-list.sh | xargs -n1 -I% sh -c "(cd %; git-difflinesonly.sh)" | head -Code39Barcode::Code39Barcode() : AbstractBarcode(), d(0){ +Code39Barcode::Code39Barcode() : AbstractBarcode(), d(nullptr){ -Code93Barcode::Code93Barcode() : AbstractBarcode(), d(0){ +Code93Barcode::Code93Barcode() : AbstractBarcode(), d(nullptr){ -DataMatrixBarcode::DataMatrixBarcode() : d(0) { +DataMatrixBarcode::DataMatrixBarcode() : d(nullptr) { -QRCodeBarcode::QRCodeBarcode() : AbstractBarcode(), d(0){ +QRCodeBarcode::QRCodeBarcode() : AbstractBarcode(), d(nullptr){ -BarcodeExampleWidget(Prison::AbstractBarcode* barcode, QWidget* parent=0); +BarcodeExampleWidget(Prison::AbstractBarcode* barcode, QWidget* parent=nullptr); % kde-frameworks-list.sh | xargs -n1 -I% sh -c "(cd %; git-difflinesonly.sh)" | wc -l 18592 > ~9000 lines changed. == This change affects *all files*. Not just headers. There are more options to limit the number of changes: - Less changes: Just change headers (.h files) -- easy - Even less changes: Just change public headers -- slightly more difficult for me to figure out *what* is public from a scripting POV If you think we should limit our changes, please speak up. I wouldn't recommend it though. Let's move forward instead. My plan was to push this after the next KF5 release. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D3987 AFFECTED FILES autotests/clipboardupdatertest.cpp autotests/deletejobtest.cpp autotests/dropjobtest.cpp autotests/fileundomanagertest.cpp autotests/http/httpauthenticationtest.cpp autotests/http_jobtest.cpp autotests/httpserver_p.h autotests/jobguitest.cpp autotests/jobremotetest.cpp autotests/jobtest.cpp autotests/kcookiejar/kcookiejartest.cpp autotests/kdirlistertest.cpp autotests/kdirmodeltest.cpp autotests/kfilecopytomenutest.cpp autotests/kfilewidgettest.cpp autotests/klocalsockettest.cpp autotests/knewfilemenutest.cpp autotests/krununittest.cpp autotests/ktcpsockettest.cpp autotests/kurifiltersearchprovideractionstest.cpp autotests/kurifiltertest.cpp autotests/kurlcompletiontest.cpp autotests/kurlnavigatortest.cpp autotests/listdirtest.cpp autotests/mkpathjobtest.cpp autotests/pastetest.cpp autotests/threadtest.cpp src/core/authinfo.cpp src/core/chmodjob.cpp src/core/connection.cpp src/core/connection_p.h src/core/connectionbackend.cpp src/core/connectionbackend_p.h src/core/connectionserver.cpp src/core/connectionserver.h src/core/copyjob.cpp src/core/dataprotocol.cpp src/core/deletejob.cpp src/core/filecopyjob.cpp src/core/forwardingslavebase.cpp src/core/hostinfo.cpp src/core/job.cpp src/core/job.h src/core/job_base.h src/core/job_error.cpp src/core/job_p.h src/core/jobtracker.cpp src/core/jobuidelegateextension.cpp src/core/jobuidelegatefactory.cpp src/core/kacl.cpp src/core/kcoredirlister.cpp src/core/kcoredirlister.h src/core/kcoredirlister_p.h src/core/kdirnotify.h src/core/kfileitem.cpp src/core/kfileitem.h src/core/klocalsocket.cpp src/core/klocalsocket.h src/core/klocalsocket_unix.cpp src/core/kmountpoint.cpp src/core/kprotocolmanager.cpp src/core/krecentdocument.cpp src/core/kremoteencoding.cpp src/core/kremoteencoding.h src/core/ksambashare.cpp src/core/ksslcertificatemanager.cpp src/core/kssld_interface.h src/core/ktcpsocket.h src/core/scheduler.cpp src/core/scheduler_p.h src/core/simplejob.cpp src/core/slave.cpp src/core/slave.h src/core/slavebase.cpp src/core/slavebase.h src/core/slaveconfig.cpp src/core/slaveinterface.h src/core/slaveinterface_p.h src/core/storedtransferjob.cpp src/core/tcpslavebase.h src/core/transferjob.cpp src/core/usernotificationhandler.cpp src/core/usernotificationhandler_p.h src/filewidgets/kdiroperator.cpp src/filewidgets/kdiroperator.h src/filewidgets/kdiroperatordetailview_p.h src/filewidgets/kdirsortfilterproxymodel.h src/filewidgets/kencodingfiledialog.h src/filewidgets/kfilecopytomenu.cpp src/filewidgets/kfilefiltercombo.h src/filewidgets/kfilemetapreview.cpp src/filewidgets/kfileplaceeditdialog.cpp src/filewidgets/kfileplaceeditdialog.h src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h src/filewidgets/kfileplacesview_p.h src/filewidgets/
[Differential] [Accepted] D3977: Fix memleak in KDynamicJobTracker, KWidgetJobTracker needs QApplication
kfunk accepted this revision. kfunk added a reviewer: kfunk. kfunk added a comment. This revision is now accepted and ready to land. Rest LGTM, but let's wait for another review INLINE COMMENTS > kdynamicjobtrackernowidgetstest.cpp:34 > +public: > +virtual void start() { QTimer::singleShot(testJobRunningTime, this, > &TestJob::doEmit); } > + `Q_DECL_OVERRIDE` > kdynamicjobtracker.cpp:99 > +} else { > +trackers.widgetTracker = 0; > } Here & below: Use `nullptr`? REPOSITORY R241 KIO BRANCH fixKDynamicJobTracker REVISION DETAIL https://phabricator.kde.org/D3977 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kossebau, #frameworks, kfunk Cc: kfunk
[Differential] [Commented On] D3850: Pass -fno-operator-names when supported
kfunk added a comment. Note: I'll push this after the imminent KF5 release if no-one objects. REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D3850 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, #buildsystem, ivan Cc: rakuco, elvisangelaccio
[Differential] [Commented On] D3850: Pass -fno-operator-names when supported
kfunk added a comment. In https://phabricator.kde.org/D3850#72308, @rakuco wrote: > Isn't it better to use `check_cxx_compiler_flag` to see if the flag is supported and enable it in case it is? -fno-operator-names is an ancient compiler flag, supported by GCC since at least 2000 [1]. I've also tested Clang, it's supported from at *least* Clang 3.5 there. Intel compiler also supports it since at least 2005 [2]. I don't think we need a `check_cxx_compiler_flag` here. [1] https://gcc.gnu.org/ml/gcc-patches/2000-04/msg00333.html [2] https://software.intel.com/en-us/forums/intel-c-compiler/topic/308561 REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3850 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, ivan, #buildsystem Cc: rakuco, elvisangelaccio
[Differential] [Commented On] D3850: Pass -fno-operator-names when supported
kfunk added a comment. In https://phabricator.kde.org/D3850#72077, @elvisangelaccio wrote: > What about adding a way (cmake variable?) to opt-in if one wants to use the alternative operators? Personally I like and use them whenever I start something from scratch... Hmm... You could overwrite the setting by just doing this after `include(KDECompilerSettings)`: set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -foperator-names") But I just learned that clang++ (Clang 3.9) does not support -foperator-names. GCC does support it... I'd like to hear some more opinions (from Ivan maybe) first. Making the use of alternative tokens opt-in is an option of course. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3850 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, ivan Cc: elvisangelaccio
[Differential] [Updated] D3850: Pass -fno-operator-names when supported
kfunk added a reviewer: ivan. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3850 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, ivan
[Differential] [Commented On] D3850: Pass -fno-operator-names when supported
kfunk added a comment. Note: Just refreshing my complete KF5 build to test the change. kactivities fails: /home/kfunk/devel/src/kf5/kactivities/autotests/common/test.h:143:25: error: token is not a valid binary operator in a preprocessor subexpression #if defined(Q_NO_DEBUG) or (not defined(Q_OS_LINUX)) ~~~ ^ 1 error generated. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3850 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Updated] D3850: Pass -fno-operator-names when supported
kfunk added a reviewer: Frameworks. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3850 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Accepted] D3733: Force colored warnings in ninja's output
kfunk accepted this revision. kfunk added a comment. This revision is now accepted and ready to land. LGTM REPOSITORY R240 Extra CMake Modules BRANCH ninja-colors REVISION DETAIL https://phabricator.kde.org/D3733 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: elvisangelaccio, #frameworks, #buildsystem, kfunk Cc: kfunk
[Differential] [Requested Changes To] D3733: Force colored warnings in ninja's output
kfunk requested changes to this revision. kfunk added a reviewer: kfunk. kfunk added a comment. This revision now requires changes to proceed. This flag is only needed for Ninja, correct? Thus please check for Ninja in `CMAKE_GENERATOR ` before adding the compiler flag. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3733 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: elvisangelaccio, #frameworks, #buildsystem, kfunk Cc: kfunk
[Differential] [Updated] D3548: Add begin/end insert/remove columns to RearrangeColumns
kfunk added a reviewer: dfaure. REPOSITORY R275 KItemModels REVISION DETAIL https://phabricator.kde.org/D3548 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: lepagevalleeemmanuel, #frameworks, dfaure Cc: kfunk, ltoscano
[Differential] [Changed Subscribers] D3548: Add begin/end insert/remove columns to RearrangeColumns
kfunk added inline comments. INLINE COMMENTS > krearrangecolumnsproxymodel.cpp:44 > + > +const int cc = d_ptr->m_sourceColumns.size(); > + Please no short hand variable names. `cc` -> `sourceColCount`? REPOSITORY R275 KItemModels REVISION DETAIL https://phabricator.kde.org/D3548 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: lepagevalleeemmanuel, #frameworks Cc: kfunk, ltoscano
[Differential] [Commented On] D3830: Add a new FindGperf module
kfunk added a comment. Windows: We have working gperf recipe in Craft => we're fine. QtWebKit already had an (optional) dependency on gperf. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3830 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: pino, #frameworks, #buildsystem, #windows, kde-mac Cc: kfunk, rjvbb, adridg
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. In https://phabricator.kde.org/D2545#69298, @thiago wrote: > In https://phabricator.kde.org/D2545#69187, @kfunk wrote: > > > In https://phabricator.kde.org/D2545#69091, @thiago wrote: > > > > > There doesn't seem to be a way of doing some clean up before the threads are forcibly killed. > > > > > > Maybe if I abuse qtmain(). > > > > > > This would only fix it for non-console GUI applications if I understand correctly. That's what we have here, but we'd still keep console applications buggy. There's no way to inject code before ExitProcess() with a plain console application. > > > I was reading the ucrt source code yesterday and it does look like it processes atexit() registrations prior to ExitProcess(). So I'll go for that. It does not work with `atext()` -- I still get the same hang if I use that. So: - `std::atexit(...)` -> hang - `qAddPostRoutine(...)` -> no hang > There is a side-effect: I have to bring back the patch that makes QtDBus DLL non-unloadable, since otherwise it could crash during exit if had already been unloaded (was loaded by a plugin). > > BTW, do you know if this problem happens with MinGW? According to this comment here https://bugs.kde.org/show_bug.cgi?id=366596#c5 it also happens when built with MinGW. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Closed] D3691: Update QMake syntax highlighting file
This revision was automatically updated to reflect the committed changes. Closed by commit R216:c3fc1271d6ad: Update QMake syntax highlighting file (authored by kfunk). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D3691?vs=9048&id=9163#toc REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3691?vs=9048&id=9163 REVISION DETAIL https://phabricator.kde.org/D3691 AFFECTED FILES data/generators/qmake-gen.py data/syntax/qmake.xml EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, vkrause Cc: vkrause
[Differential] [Commented On] D3691: Update QMake syntax highlighting file
kfunk added a comment. Bump REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D3691 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Commented On] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
kfunk added a comment. In https://phabricator.kde.org/D2075#66751, @jonathans wrote: > Agreed that would be more robust. In writing the patch I was seeking consistency with those functions that already did the test, so those would also need to be updated. Are there any situations where the two tests would yield a different result, ie d->native is true and d->w is non-null? Probably not. But I think it makes more sense to check the pointer you're actually going to dereference in the next statement. Could you update the patch? And also fix the `nullptr` issues? REPOSITORY R239 KDELibs4Support REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
[Differential] [Closed] D3702: kconfig_compiler: Use nullptr in generated code
This revision was automatically updated to reflect the committed changes. Closed by commit R237:e6c88f67e2cb: kconfig_compiler: Use nullptr in generated code (authored by kfunk). REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3702?vs=9072&id=9075 REVISION DETAIL https://phabricator.kde.org/D3702 AFFECTED FILES autotests/kconfig_compiler/test10.cpp.ref autotests/kconfig_compiler/test4.cpp.ref autotests/kconfig_compiler/test5.cpp.ref autotests/kconfig_compiler/test8b.cpp.ref autotests/kconfig_compiler/test_dpointer.cpp.ref autotests/kconfig_compiler/test_signal.cpp.ref src/kconfig_compiler/kconfig_compiler.cpp EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, davidedmundson
[Differential] [Updated] D3702: kconfig_compiler: Use nullptr in generated code
kfunk added a reviewer: Frameworks. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D3702 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. In https://phabricator.kde.org/D2545#69091, @thiago wrote: > There doesn't seem to be a way of doing some clean up before the threads are forcibly killed. > > Maybe if I abuse qtmain(). This would only fix it for non-console GUI applications if I understand correctly. That's what we have here, but we'd still keep console applications buggy. There's no way to inject code before ExitProcess() with a plain console application. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. For the record, since I don't see an easy fix I'm pondering about patching qtbase in craft.git: Ideas: a ) Add this to QDBusConnectionManager ctor: qAddPostRoutine([]() { QMetaObject::invokeMethod(QDBusConnectionManager::instance(), "quit"); }); b) Just cherry-pick https://codereview.qt-project.org/#/c/147261/1 (tried to fix the same issue, but got abandoned) This might be killing DBus connections too early, but it's better than hanging the process on exit (I'm being pragmatic here). BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. > Here's the other problem: it's possible for threads to simply disappear on Windows. Given that I see "dllmain" in the backtrace (though not DllMain), I can't rule out that this has happened. Qt 5.6 has a workaround to another deadlock caused by Windows. Can you try to cherry-pick 3ec57107cedb154f256e3ad001ea5475cc64fa94 from 5.8? With 3ec57107cedb154f256e3ad001ea5475cc64fa94 applied: Still dead-locking, same backtrace. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Updated, 674 lines] D3691: Update QMake syntax highlighting file
kfunk updated this revision to Diff 9048. kfunk added a comment. Add generator REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3691?vs=9047&id=9048 BRANCH master REVISION DETAIL https://phabricator.kde.org/D3691 AFFECTED FILES data/generators/qmake-gen.py data/syntax/qmake.xml EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Updated] D3691: Update QMake syntax highlighting file
kfunk added a reviewer: Frameworks. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D3691 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks
[Differential] [Updated] D3586: [WIP] Fix logging category usage on Windows
kfunk added a reviewer: Frameworks. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D3586 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: vonreth, sandsmark, kfunk, leinir, #frameworks Cc: kfunk, leinir, mutlaqja
[Differential] [Requested Changes To] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
kfunk requested changes to this revision. kfunk added a reviewer: kfunk. kfunk added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfiledialog.cpp:607 > +if (d->native) { > +return; > +} Should we rather check for `!d->w` here and below? Would make more sense IMO. REPOSITORY R239 KDELibs4Support REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure, kfunk Cc: kfunk, aacid
[Differential] [Updated] D3568: Find the correct path to the cmake command
kfunk edited reviewers, added: Frameworks; removed: Framework: Syntax Hightlighting. BRANCH find-cmake-binary REVISION DETAIL https://phabricator.kde.org/D3568 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: obogdan, kfunk, #frameworks Cc: kfunk
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. In https://phabricator.kde.org/D2545#65976, @dfaure wrote: > Actually, I think Thiago's still waiting for a backtrace of all threads. I think I've already said this via other channels: There's only one thread left at this point. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Updated] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
kfunk added a reviewer: dfaure. REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks, dfaure
[Differential] [Updated] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.
kfunk added a reviewer: Frameworks. REVISION DETAIL https://phabricator.kde.org/D2075 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: jonathans, #frameworks
[Differential] [Planned Changes To] D3393: Make compile against WinXP SDK
kfunk planned changes to this revision. kfunk added inline comments. INLINE COMMENTS > dhaumann wrote in kioglobal_p_win.cpp:87 > Or should it just return false in this case? Thinking about it, yep, probably better that way. Will update the patch. REVISION DETAIL https://phabricator.kde.org/D3393 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, arichardson, #frameworks Cc: dhaumann
[Differential] [Commented On] D2854: New: ECMGenerateApiDox, for generating qch & tag files
kfunk added a comment. Heya Friedrich. You asked me to review this. From a super brief scan of the code I don't see anything blatantly wrong. I trust you that you've tested this good enough. Love the extensive documentation. What's left here from your side? REVISION DETAIL https://phabricator.kde.org/D2854 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kossebau, staniek Cc: kfunk, staniek, winterz, ochurlaud, #kdevelop, #frameworks
[Differential] [Commented On] D3393: Make compile against WinXP SDK
kfunk added a comment. In https://phabricator.kde.org/D3393#63437, @dhaumann wrote: > In general ok (but why is this a review request for KSyntaxHighlighting?) Because the reviewers field auto-completion sucks :) REVISION DETAIL https://phabricator.kde.org/D3393 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, arichardson, #frameworks Cc: dhaumann
[Differential] [Updated] D3393: Make compile against WinXP SDK
kfunk edited reviewers, added: Frameworks; removed: Framework: Syntax Hightlighting. REVISION DETAIL https://phabricator.kde.org/D3393 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, arichardson, #frameworks Cc: dhaumann
[Differential] [Commented On] D3392: winutils_p.h: Restore compatibility with WinXP SDK
kfunk added a comment. And I meant this line: https://code.woboq.org/qt5/qtbase/src/corelib/io/qfsfileengine_win.cpp.html#547 (Woboq bug..., can't link the Windows version of `QFSFileEngine::drivers()`...) BRANCH master REVISION DETAIL https://phabricator.kde.org/D3392 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, brauch
[Differential] [Commented On] D3392: winutils_p.h: Restore compatibility with WinXP SDK
kfunk added a comment. In https://phabricator.kde.org/D3392#63341, @brauch wrote: > Hm, there is at least one race condition here: thread A does old = ::SetErrorMode(...), then if before it resets it thread B does the same, then the old mode might never be restored. We can put a mutex but of course there might be another piece of code which does the same. So *shrug* not sure what to do, I guess it's just bad API and that's why MS replaced it. I really just wouldn't care about it. We likely never ever run in that scenario. Keep in mind, that Qt uses `Get/SetErrorMode` like this since ages: https://code.woboq.org/qt5/qtbase/src/corelib/io/qfsfileengine_unix.cpp.html#_ZN13QFSFileEngine6drivesEv (here and in other places). It's essentially the same code; not protected by a mutex or anything. > Maybe on XP we should just set the flag once on startup? At least that's not racy. REVISION DETAIL https://phabricator.kde.org/D3392 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, brauch
[Differential] [Updated] D3392: winutils_p.h: Restore compatibility with WinXP SDK
kfunk added reviewers: brauch, Frameworks. REVISION DETAIL https://phabricator.kde.org/D3392 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, brauch, #frameworks
[Differential] [Changed Subscribers] D3226: Don't be fatal on File field not being properly parsed
kfunk added inline comments. INLINE COMMENTS > KF5ConfigMacros.cmake:71 > file(READ ${_tmp_FILE} _contents) > - string(REGEX MATCH "File=([^\n]+\\.kcfg)\n" "" "${_contents}") > + string(REGEX MATCH "File=([^\n]+\\.kcfg)c?\n" "" "${_contents}") > set(_kcfg_FILENAME "${CMAKE_MATCH_1}") I suggest to add some commentary here. REVISION DETAIL https://phabricator.kde.org/D3226 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: apol, #frameworks Cc: kfunk
[Differential] [Closed] D3091: Windows: Don't display error dialogs
kfunk closed this revision. kfunk added a comment. Pushed: commit f544380db86301f4833b2149dd46dca47acc8042 Author: Kevin Funk Date: Mon Oct 17 22:02:26 2016 +0200 Windows: Don't display error dialogs REVISION DETAIL https://phabricator.kde.org/D3091 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, vonreth, brauch Cc: vonreth, nalvarez, broulik
[Differential] [Updated, 71 lines] D3091: Windows: Don't display error dialogs
kfunk updated this revision to Diff 7495. kfunk added a comment. Add Q_DISABLE_COPY CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3091?vs=7476&id=7495 BRANCH master REVISION DETAIL https://phabricator.kde.org/D3091 AFFECTED FILES src/solid/devices/backends/win/winstoragevolume.cpp src/solid/devices/backends/win/winutils_p.h EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, brauch Cc: vonreth, nalvarez, broulik
[Differential] [Commented On] D3091: Windows: Don't display error dialogs
kfunk added a comment. In https://phabricator.kde.org/D3091#57279, @nalvarez wrote: > Meanwhile I'm skeptical of the need to "set it back". Is there any case where such error dialog boxes would be desirable? It seems like their existence is an ancient-app-compatibility thing. The documentation for `SetErrorMode` recommends setting `SEM_FAILCRITICALERRORS` at app startup and leaving it that way. I'm using a similar approach as Qt does. If some application decides to use a different "global" error mode, we should not interfere with it just because Solid wants to. This is a general purpose library after all. REVISION DETAIL https://phabricator.kde.org/D3091 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, brauch Cc: nalvarez, broulik
[Differential] [Commented On] D3091: Windows: Don't display error dialogs
kfunk added a comment. In https://phabricator.kde.org/D3091#57277, @broulik wrote: > Can't really comment on this, though. > While I'm a huge fan of RAII you don't seem to be returning early from that function RAII is not only useful for cases where you return early. > ..., can't you just set the value and set it back at the end without this new class? I'm fairly sure, we'll need to guard other WinAPI calls as well. QStorageInfo from qtbase uses `SetErrorMode/GetErrorMode` a lot REVISION DETAIL https://phabricator.kde.org/D3091 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, brauch Cc: nalvarez, broulik
[Differential] [Updated, 70 lines] D3091: Windows: Don't display error dialogs
kfunk updated this revision to Diff 7476. kfunk added a comment. SetErrorMode -> SetThreadErrorMode (thread-safe) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3091?vs=7475&id=7476 BRANCH master REVISION DETAIL https://phabricator.kde.org/D3091 AFFECTED FILES src/solid/devices/backends/win/winstoragevolume.cpp src/solid/devices/backends/win/winutils_p.h EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, brauch Cc: nalvarez, broulik
[Differential] [Updated] D3091: Windows: Don't display error dialogs
kfunk added reviewers: Frameworks, brauch. REVISION DETAIL https://phabricator.kde.org/D3091 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, #frameworks, brauch
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. Here's the backtrace (note: using Qt 5.6 branch): ntdll.dll!NtWaitForSingleObject() Unknown KernelBase.dll!7ffbcb2eaadf() Unknown > Qt5Core.dll!QWaitCondition::wait(QMutex * mutex=0x01c24fd16520, unsigned long time=4294967295) Line 178 C++ Qt5Core.dll!QSemaphore::acquire(int n=1) Line 136 C++ Qt5Core.dll!QMetaObject::activate(QObject * sender=0x01c24a6eb500, int signalOffset, int local_signal_index, void * * argv=0x0081e131f648) Line 3699C++ Qt5Core.dll!QObject::~QObject() Line 913C++ Qt5DBus.dll!QDBusServiceWatcher::`vector deleting destructor'(unsigned int) C++ Qt5Core.dll!QObjectPrivate::deleteChildren() Line 1960 C++ Qt5Core.dll!QObject::~QObject() Line 1034 C++ KF5JobWidgets.dll!KSharedUiServerProxy::~KSharedUiServerProxy() Line 325C++ KF5JobWidgets.dll!``anonymous namespace'::Q_QGS_serverProxy::innerFunction'::`2'::`dynamic atexit destructor for 'holder''()C++ ucrtbase.dll!_time64() Unknown ucrtbase.dll!__crt_seh_guarded_call::operator(),class &,class >(class &&,class &,class &&) Unknown ucrtbase.dll!_execute_onexit_table() Unknown KF5JobWidgets.dll!dllmain_crt_process_detach(const bool is_terminating=true) Line 109 C++ KF5JobWidgets.dll!dllmain_dispatch(HINSTANCE__ * const instance=0x7ffbb8e1, const unsigned long reason=0, void * const reserved=0x0001) Line 207C++ ntdll.dll!LdrpCallInitRoutine() Unknown ntdll.dll!LdrShutdownProcess() Unknown ntdll.dll!RtlExitUserProcess() Unknown kernel32.dll!ExitProcessImplementation() Unknown ucrtbase.dll!swprintf()Unknown ucrtbase.dll!swprintf()Unknown kdevelop.exe!__scrt_common_main_seh() Line 262 C++ kernel32.dll!BaseThreadInitThunk() Unknown ntdll.dll!RtlUserThreadStart() Unknown BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. Bump? REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
[Differential] [Commented On] D2546: Cleanup DBus-related resources before qApp exits
kfunk added a comment. Bump? REVISION DETAIL https://phabricator.kde.org/D2546 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
[Differential] [Updated, 19 lines] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk updated this revision to Diff 6175. kfunk added a comment. Fix typo in commit msg CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D2545?vs=6173&id=6175 BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 AFFECTED FILES src/kuiserverjobtracker.cpp src/kuiserverjobtracker_p.h EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
[Differential] [Updated] D2546: Cleanup DBus-related resources before qApp exits
kfunk updated the test plan for this revision. kfunk added reviewers: dfaure, vonreth. kfunk added a subscriber: Frameworks. REVISION DETAIL https://phabricator.kde.org/D2546 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
[Differential] [Updated] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added reviewers: dfaure, vonreth. kfunk added a subscriber: Frameworks. REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
[Differential] [Closed] D2142: KGlobalAccel: Fix deadlock on exit under Windows
kfunk closed this revision. kfunk added a comment. Pushed. commit 2bdb684a872aff26c01f5e1fc175fbaf663ad8ba Author: Kevin Funk Date: Tue Jul 12 10:18:40 2016 +0200 REVISION DETAIL https://phabricator.kde.org/D2142 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, graesslin, dfaure Cc: graesslin, #frameworks ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[Differential] [Commented On] D2142: KGlobalAccel: Fix deadlock on exit under Windows
kfunk added a comment. FYI: There are more issues in other frameworks, the next deadlock is in kiconthemes... BRANCH master REVISION DETAIL https://phabricator.kde.org/D2142 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth, graesslin Cc: graesslin, #frameworks ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[Differential] [Commented On] D2142: KGlobalAccel: Fix deadlock on exit under Windows
kfunk added a comment. Waiting for a +1 from David. There's one possible point of regression, namely if `KGlobalAccel::self()` is being used after qApp ran all the post routines (and `KGlobalAccelPrivate::cleanup()` has been invoked already). Not sure if this is a problem, though. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2142 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth, graesslin Cc: graesslin, #frameworks ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[Differential] [Updated] D2142: KGlobalAccel: Fix deadlock on exit under Windows
kfunk added reviewers: dfaure, vonreth. kfunk added a subscriber: Frameworks. REVISION DETAIL https://phabricator.kde.org/D2142 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel