D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. This commit breaks krununittest in CI, can you take a look? https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kio/job/kf5-qt5%20SUSEQt5.12/471/testReport/junit/projectroot/autotests/kiowidgets_krununittest/ Thanks. REPOSITORY R241 KIO

D28131: listen to passiveNotificationRequested

dfaure added a comment. CI is not happy: https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kcmutils/job/kf5-qt5%20SUSEQt5.12/107/console REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D28131 To: mart, broulik Cc: dfaure, kde-frameworks-devel,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. Only if an external browser is set, but yeah. REPOSITORY R241 KIO BRANCH l-krun-ssh (branched from master) REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew Cc: kde-frameworks-devel, LeGast00n,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. OK with me, but check with the fuse people. You test ssh, telnet etc but hasSchemeHandler is also true for at least http. REPOSITORY R241 KIO BRANCH l-krun-ssh (branched from

D28020: New class ProcessLauncherJob in KIOGui

dfaure added a comment. Ah there was still the WId question. I'll remove it before anyone starts using this API. We can always add a setter later if we want to (but if it's just for the unused setLaunchedBy, I'm not convinced...) REPOSITORY R241 KIO REVISION DETAIL

D28020: New class ProcessLauncherJob in KIOGui

dfaure closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28020 To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

dfaure added inline comments. INLINE COMMENTS > davidedmundson wrote in kprocessrunner.cpp:194 > It's the DBus calls that come before start that I want to get async, not the > tiny bit between fork and the child process exec()'ing. > > Obviously we can do that piecemeal later, and it isn't a

D17816: Support for xattrs on kio copy/move

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > jobtest.cpp:118 > +} else { > +qWarning() << "Xatr command not foud."; > +} typo in Xatr, and in "foud". I suggest:

D28020: New class ProcessLauncherJob in KIOGui

dfaure updated this revision to Diff 77682. dfaure added a comment. - Don't block in start(), make it fully async - Add waitForStarted() for KRun (with unittests) - Add test for non-existing executables, with and without kioexec - after making sure that the command isn't trivial, by

D17816: Support for xattrs on kio copy/move

dfaure requested changes to this revision. dfaure added inline comments. INLINE COMMENTS > cochise wrote in jobtest.cpp:573 > xtattr = MacOS command > It uses a single command to read and write. All of these are spelled with two consecutive 't'. The code has a single 't' : getextatr. > dfaure

D28060: Fix exitcode from kioexec when executable doesn't exist (and --tempfiles is set)

dfaure closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28060 To: dfaure, apol, davidedmundson, elvisangelaccio Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

Re: KIO: try to assign an icon to action submenus

On vendredi 13 mars 2020 21:19:42 CET Chloe Kudryavtsev wrote: > On 3/13/20 2:18 PM, David Faure wrote: > > On mardi 3 mars 2020 18:29:47 CET Chloe Kudryavtsev wrote: > >> Currently, action submenus (X-KDE-Submenu) have no icons. > >> This patch makes it inherit t

D28060: Fix exitcode from kioexec when executable doesn't exist (and --tempfiles is set)

dfaure created this revision. dfaure added reviewers: apol, davidedmundson, elvisangelaccio. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. TEST PLAN kioexec --tempfiles does_not_exist /tmp/foo Used to exit with

D28020: New class ProcessLauncherJob in KIOGui

dfaure planned changes to this revision. dfaure added inline comments. INLINE COMMENTS > davidedmundson wrote in kprocessrunner.cpp:39 > WId as an int is problematic for wayland. > > Can we do QWindow*? it'll allow adding support in future. > > For the compatibility path we can loop through

D27947: Port away from deprecated QList::toSet()

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH l-kcoredirlister-toset (branched from master) REVISION DETAIL https://phabricator.kde.org/D27947 To: ahmadsamir, #frameworks, dfaure, meven Cc: apol, kde-frameworks-devel,

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH l-kpasswdserver (branched from master) REVISION DETAIL https://phabricator.kde.org/D27965 To: ahmadsamir, #frameworks, dfaure, meven Cc: apol, kde-frameworks-devel, LeGast00n,

D27986: Allow providing an error message from the application

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R285 KCrash BRANCH master REVISION DETAIL https://phabricator.kde.org/D27986 To: apol, #frameworks, sitter, dfaure Cc: dfaure, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,

Re: KIO: try to assign an icon to action submenus

other actions do. Wouldn't it be better to be able to explicitly specify the icon for the submenu? -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5

D28020: New class ProcessLauncherJob in KIOGui

dfaure created this revision. dfaure added reviewers: apol, davidedmundson, nicolasfella, vkrause, broulik. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. REVISION SUMMARY This is meant to replace

D27966: KParts: add PartLoader as replacement to KMimeTypeTrader for parts

dfaure added a comment. Yeah I guess "looks reasonable" is the most I can expect :-) Any chance you could take a look at https://phabricator.kde.org/D27967 which is needed for this to land? Thanks. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27966 To:

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. Thanks, looks better. One possible problem left: if KIO has never been installed, I guess QStandardPaths::findExecutable(QStringLiteral("ktelnetservice5")) won't find it in builddir/bin. You could push and see what CI says, or you could test locally what happens

D28016: KWindowSystem: deprecate KStartupInfoData::launchedBy, unused

dfaure created this revision. dfaure added reviewers: zzag, broulik, davidedmundson. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. REVISION SUMMARY KRun bothers with a WId in many places just to end up calling

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > krununittest.cpp:245 > +QCOMPARE(KShell::joinArgs(parser.resultingArguments()), > + QStringLiteral("/usr/bin/ktelnetservice5 >

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

dfaure added a comment. @mlaurent compilation is broken because a method got deprecated, and you set -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x06 so the use of that method no longer compiles. You can hardly blame other people for deprecating methods, it's a rather natural thing to do at

D27986: Allow providing an error message from the application

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Every other static variable here is a char* so that there's no global constructor being called at application startup. Why not do the same? Just `strdup` the result of the

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. Thanks for the unittest with the fix. INLINE COMMENTS > krununittest.cpp:238 > +{ > +const QString ktelnet = QStringLiteral(CMAKE_SOURCE_DIR) + > QStringLiteral("/src/ioslaves/telnet/ktelnetservice5.desktop"); > +const KService service(ktelnet);

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kpasswdserver.cpp:201 > > bool KPasswdServer::hasPendingQuery(const QString , const KIO::AuthInfo > ) > { I wonder why the whole method isn't const >

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

dfaure added a comment. Fixed in https://commits.kde.org/kio/440bec1b3524f168fc04898a09ac51457c79ed3b REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25010 To: meven, #frameworks, dfaure, kossebau Cc: dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,

D27966: KParts: add PartLoader as replacement to KMimeTypeTrader for parts

dfaure updated this revision to Diff 77474. dfaure added a comment. Document how to load a part from a given KPluginMetaData REPOSITORY R306 KParts CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27966?vs=77381=77474 REVISION DETAIL https://phabricator.kde.org/D27966 AFFECTED

D27953: Properly read the X-Flatpak-RenamedFrom string list from desktop files

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R309 KService BRANCH master REVISION DETAIL https://phabricator.kde.org/D27953 To: apol, dfaure Cc: broulik, dfaure, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25315: KDirModel: implement showing a root node for the requested URL

dfaure added inline comments. INLINE COMMENTS > ahmadsamir wrote in kdirmodeltest_gui.cpp:92 > I did test the /usr/share/fonts path :); it's just that starting at "/" > looked "normal", whereas starting at a specific dir conveyed the goal of this > change better, to me anyway. It doesn't look

D27953: Properly read string lists from desktop files when parsing the files

dfaure added a comment. How? You think we should change all our config files to use ';'? Welcome to migration hell. (I looked at the KConfig history, and in fact the separator was initially a method argument which defaulted to comma, I was wrong about it being ';' initially. No idea

D27953: Properly read string lists from desktop files when parsing the files

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. This sounds wrong. The XDG-mandated entries use XDG separators, but the KDE entries don't (for instance, grepping my installed desktop files I can see those:

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27965 To: ahmadsamir, #frameworks, dfaure, meven Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure added a comment. In D27965#625676 , @ahmadsamir wrote: > In D27965#625526 , @apol wrote: > > > Having the iterated value change under the hood will eventually break. I'd suggest preferring

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

dfaure added a comment. Yep, that's what "Accepted" means ;) REPOSITORY R241 KIO BRANCH arcpatch-D25010 REVISION DETAIL https://phabricator.kde.org/D25010 To: meven, #frameworks, dfaure, kossebau Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

dfaure accepted this revision. REPOSITORY R241 KIO BRANCH arcpatch-D25010 REVISION DETAIL https://phabricator.kde.org/D25010 To: meven, #frameworks, dfaure, kossebau Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27910: Make sure warning output is enabled before testing if the correct warning is printed

dfaure added a comment. I suggest a better solution: QStandardPaths::setTestModeEnabled(true) in initTestCase(). Then the user settings won't interfere with the unittest. REPOSITORY R243 KArchive REVISION DETAIL https://phabricator.kde.org/D27910 To: sandsmark, dfaure Cc: apol,

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Huh. Sorry about the delays and the need to update version numbers once more. My last two comments are still not fixed. To see the issues, grep for "direcly" ('t' missing) and "Details

D27966: KParts: add PartLoader as replacement to KMimeTypeTrader for parts

dfaure updated this revision to Diff 77381. dfaure added a comment. Ensure no duplicates in partsForMimeType REPOSITORY R306 KParts CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27966?vs=77340=77381 REVISION DETAIL https://phabricator.kde.org/D27966 AFFECTED FILES

D27966: KParts: add PartLoader as replacement to KMimeTypeTrader for parts

dfaure added inline comments. INLINE COMMENTS > aacid wrote in partloader.h:59 > What's the use for this? The function below doesn't let me chose which one i > want (i guess it always uses the one with the most preference?), so why would > i need to query which parts are available? > > Maybe

T12173: KService: provide solution to migrate away from KServiceTypeTrader/KMimeTypeTrader for loading plugins and parts

dfaure added a revision: D27966: KParts: add PartLoader as replacement to KMimeTypeTrader for parts. TASK DETAIL https://phabricator.kde.org/T12173 To: dfaure Cc: #frameworks, nicolasfella, dfaure, mart, davidre, GB_2, ekasprzak, ahmadsamir, ngraham, kpiwowarski, usta, asturmlechner, jucato,

T12173: KService: provide solution to migrate away from KServiceTypeTrader/KMimeTypeTrader for loading plugins and parts

dfaure added a revision: D27967: KParts: add unittest for PartLoader, required shuffling things around. TASK DETAIL https://phabricator.kde.org/T12173 To: dfaure Cc: #frameworks, nicolasfella, dfaure, mart, davidre, GB_2, ekasprzak, ahmadsamir, ngraham, kpiwowarski, usta, asturmlechner,

D27966: KParts: add PartLoader as replacement to KMimeTypeTrader for parts

dfaure created this revision. dfaure added reviewers: aacid, nicolasfella, kossebau. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. REVISION SUMMARY It's based on the JSON metadata embedded into the plugins in

D25302: Remove stale symlink

dfaure requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R243 KArchive REVISION DETAIL https://phabricator.kde.org/D25302 To: sandsmark, dfaure Cc: dfaure, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25302: Remove stale symlink

dfaure added inline comments. INLINE COMMENTS > karchive.cpp:986 > +// link target or be non-relative > +if (QFile::exists(linkName) && linkName != > curEntry->symLinkTarget()) { > +QFile::remove(linkName); This is comparing the name of the

D25315: KDirModel: implement showing a root node for the requested URL

dfaure marked an inline comment as done. dfaure added inline comments. INLINE COMMENTS > ahmadsamir wrote in kdirmodel.h:79 > I suggest: > s/its children/the first child/ OR > s/at its children/directly at the first child/ That sounds more confusing to me, depending on how one thinks about all

D25315: KDirModel: implement showing a root node for the requested URL

dfaure updated this revision to Diff 77224. dfaure added a comment. Add missing braces around one-line if() REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25315?vs=77155=77224 BRANCH 2019_11_kdirmodel_root REVISION DETAIL

D25315: KDirModel: implement showing a root node for the requested URL

dfaure updated this revision to Diff 77155. dfaure added a comment. Strip trailing slashes, add test REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25315?vs=76701=77155 BRANCH 2019_11_kdirmodel_root REVISION DETAIL https://phabricator.kde.org/D25315

D25315: KDirModel: implement showing a root node for the requested URL

dfaure added a comment. OK, reproduced by adding a test for a URL with a trailing slash. Fix coming up. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25315 To: dfaure, stefanocrocco, elvisangelaccio, meven, apol Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2,

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

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig BRANCH l-kconfiggui (branched from master) REVISION DETAIL https://phabricator.kde.org/D27811 To: ahmadsamir, #frameworks, dfaure, davidedmundson, cfeck, ervin, meven, bport Cc:

D27865: [KFileFilterCombo] Don't add invalid QMimeType to mimes filter

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH l-kfilefiltercombo (branched from master) REVISION DETAIL https://phabricator.kde.org/D27865 To: ahmadsamir, #frameworks, dfaure, meven, sitter, broulik Cc: kde-frameworks-devel,

D27614: build: fix the build where install prefix is not user-writable

dfaure added inline comments. INLINE COMMENTS > CMakeLists.txt:17 > +add_custom_target(breeze-generate-24px-versions-dark ALL > +COMMAND ${BASH_EXE} ${CMAKE_SOURCE_DIR}/generate-24px-versions.sh > ${CMAKE_CURRENT_BINARY_DIR}/generated/ > WORKING_DIRECTORY

D27865: [KFileFilterCombo] Don't add invalid QMimeType to mimes filter

dfaure added inline comments. INLINE COMMENTS > kfilefiltercombo.cpp:156 > +if (!type.isValid()) { > +qCDebug(KIO_KFILEWIDGETS_KFILEFILTERCOMBO) << *it << "is not a > valid mimeType"; > +continue; IMHO it's a programmer error so it should be a qCWarning (visible

D9734: [KFileItemActions] Allow specifying the number of selected files required for an action

dfaure added a comment. Or upload a patch :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9734 To: broulik, #frameworks, dfaure, michaelh, mlaurent Cc: ngraham, kde-frameworks-devel, juansimon, LeGast00n, cblack, GB_2, michaelh, bruns

D27898: [autotests/*] Port foreach (deprecated) to range-based for loop

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D27898 To: ahmadsamir, #frameworks, dfaure, meven Cc: kde-frameworks-devel, LeGast00n, cblack,

D27876: KRearrangeColumnsProxyModel: reset in setSourceColumns()

dfaure closed this revision. REPOSITORY R275 KItemModels REVISION DETAIL https://phabricator.kde.org/D27876 To: dfaure, kossebau, vkrause, davidedmundson, ahiemstra Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27876: KRearrangeColumnsProxyModel: reset in setSourceColumns()

dfaure added a comment. It did indeed... Thanks, fixing. REPOSITORY R275 KItemModels BRANCH master REVISION DETAIL https://phabricator.kde.org/D27876 To: dfaure, kossebau, vkrause, davidedmundson, ahiemstra Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27876: KRearrangeColumnsProxyModel: reset in setSourceColumns()

dfaure updated this revision to Diff 77047. dfaure added a comment. Use initializer lists REPOSITORY R275 KItemModels CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27876?vs=77044=77047 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27876 AFFECTED FILES

D27876: KRearrangeColumnsProxyModel: reset in setSourceColumns()

dfaure created this revision. dfaure added reviewers: kossebau, vkrause, davidedmundson, ahiemstra. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. REVISION SUMMARY Otherwise the view won't update, when doing this

D27865: [KFileFilterCombo] Don't add inavlid QMimeType to mime filter

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfilefiltercombo.cpp:31 > +Q_DECLARE_LOGGING_CATEGORY(KIO_KFILEWIDGETS_KFILEFILTERCOMBO) > +Q_LOGGING_CATEGORY(KIO_KFILEWIDGETS_KFILEFILTERCOMBO, >

D27865: [KFileFilterCombo] Don't add inavlid QMimeType to mime filter

dfaure added a comment. Maybe add a warning about the invalid type? After all it's a programmer error to send us invalid mimetypes here, right? Are you also fixing the source of the issue in gwenview? Thanks! REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27865

D27864: KIO::iconNameForUrl(): handle the case of a file/folder under trash:/

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Yay for unittests :-) REPOSITORY R241 KIO BRANCH l-trash-stuff (branched from master) REVISION DETAIL https://phabricator.kde.org/D27864 To: ahmadsamir, #frameworks, dfaure, meven,

D27846: Fix KNewFileMenuTest

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks REPOSITORY R241 KIO BRANCH l-KNewFileMenuTest (branched from master) REVISION DETAIL https://phabricator.kde.org/D27846 To: ahmadsamir, #frameworks, dfaure Cc:

D25315: KDirModel: implement showing a root node for the requested URL

dfaure added a comment. Well, I don't have your code. Which calls are you making into KDirModel? (openUrl(args=?), expandToUrl(args=?)) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25315 To: dfaure, stefanocrocco, elvisangelaccio, meven, apol Cc:

D25315: KDirModel: implement showing a root node for the requested URL

dfaure updated this revision to Diff 76701. dfaure retitled this revision from "KDirModel: implement showing "/" as a root node, optionally" to "KDirModel: implement showing a root node for the requested URL". dfaure edited the summary of this revision. dfaure removed a subscriber: rrosch.

D25315: KDirModel: implement showing "/" as a root node, optionally

dfaure added a comment. In D25315#619727 , @rrosch wrote: > > Keep only matters for further calls to openUrl, not the first one. It's about whether to *add* or *replace* the currently open URL. > > KDirModel takes care of that. > > Ah ok,

D27731: Improve KDirModel to avoid showing '+' if there are no subdirs

dfaure closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27731 To: dfaure, apol, ahmadsamir, meven, rrosch Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27735: [KConfigGui] Clear styleName font property for Regular font sytles

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Wow that Qt bug report has really extensive investigation, very impressive. The fix looks ok to me (without being an expert on the topic, it does seem to match what that bug report is

D25315: KDirModel: implement showing "/" as a root node, optionally

dfaure planned changes to this revision. dfaure added a comment. In D25315#619436 , @rrosch wrote: > Tested on patched KF5 5.64, on Fedora 30 (32bit)... root node shows up! I had to change the call in my code that was originally: > >

D27731: Improve KDirModel to avoid showing '+' if there are no subdirs

dfaure added a reviewer: rrosch. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27731 To: dfaure, apol, ahmadsamir, meven, rrosch Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27731: Improve KDirModel to avoid showing '+' if there are no subdirs

dfaure created this revision. dfaure added reviewers: apol, ahmadsamir, meven. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. REVISION SUMMARY We either have the information at hand (in populated dirs) or we query

D27654: [kio] Fix running konsole on Wayland

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Hum. OK. Weird. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27654 To: wbauer, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D21795: [KAuth] Add support for action details in Polkit1 backend.

dfaure added inline comments. INLINE COMMENTS > sitter wrote in Polkit1Backend.cpp:239 > M. M. I don't really have a suggestion here, but this is an > incredibly dangerous change. Nested event loops can cause all sorts of > negative effects. That's why the isValid had a note in the

D27654: [kio] Fix running konsole on Wayland

dfaure added a comment. Since this is konsole specific, it's fine to use a Qt-specific argument. But IMHO you could also just remove the %i altogether. I never understood the idea of "take the icon from the desktop file, not the one the application would use by default". I mean, with

D27097: Port from QRegExp to QRegularExpression

dfaure added a comment. Deprecate the attribute, keep the code. There is more KF5-based code out there than the one visible by lxr.kde.org. Some of it not even public. We make the same promise as Qt, preserving SC and BC in major versions. OK for "Invalid PCRE pattern syntax"

D27133: kconfig_compiler : generate kconfig settings with subgroup

dfaure added a comment. OK. `man ascii` says 0x1d is "GS (group separator)", not completely bogus :) I wonder how/why it becomes in XML, but quick googling says that entity seems to be '␝' which indeed reads GS :) At the KConfig level, this is good for catching people who don't use

D27133: kconfig_compiler : generate kconfig settings with subgroup

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

D27153: port sftp to Result system to force serialization of error/finish condition

dfaure accepted this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D27153 To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven,

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH arcpatch-D27539 REVISION DETAIL https://phabricator.kde.org/D27539 To: meven, ngraham, #frameworks, dfaure, broulik, sitter Cc: sitter, kde-frameworks-devel, LeGast00n, cblack,

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > global.cpp:240 > // Let KFileItem::iconName handle things for us > -if (i == unknown || i.isEmpty() || mt.isDefault()) { > +if

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

dfaure added a comment. This is about an icon name. Apps don't (shouldn't) "check the value". We should return application-octet-stream if we did find the file, but mimetype determination failed. That's what this mimetype and its icon are about. We should return unknown if we have

D27097: Port from QRegExp to QRegularExpression

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. OK for deprecating Incremental. A full revert (at KF6 time) of commit 9ac82e27ad0322e444c16 looks

D27153: port sftp to Result system to force serialization of error/finish condition

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Nice work! Found 2 bugs though. INLINE COMMENTS > kio_sftp.cpp:1306 > rc = ssh_channel_poll(mSftp->channel, 1); > -} > - > -if (rc < 0) { > +} else if (rc <

D27451: Drop KToolInvocation support from KRun::runService

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. This makes runService and runApplication almost the same, right? KRecentDocument::add seems missing in runApplication, but that's a bug, so one could just call the other? REPOSITORY

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

dfaure added a comment. kfileitemtest still passes? REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D27539 To: meven, ngraham, #frameworks, dfaure, broulik, sitter Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27459: Port QLinkedList which is deprecated in qt5.15

dfaure added a comment. This code only seems to do append() and contains(), a QVector (contiguous area of memory) is better than a std::list (individual nodes spread all over the memory, more cache lines to load) REPOSITORY R263 KXmlGui BRANCH port_QLinkedList (branched from master)

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

dfaure accepted this revision. dfaure added inline comments. INLINE COMMENTS > kcoreconfigskeleton.h:459 > +// given we can't, I've shadowed the method. This isn't pretty, but given > +// the docs say it should generally only be used from auto generated > code, > void

Re: 2 kirigami fixes for a point release

ss that QtQuick doesn't make it easy, but that's part of the problem...). -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5

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

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Well, I'm not the KConfig maintainer, mdawson is :-) INLINE COMMENTS > kcoreconfigskeleton.h:89 > + */ > +void setGroup(const KConfigGroup ); > + Missing @since 5.68 >

T12641: Refactor KFileProtocol::copy

dfaure added a comment. I don't really understand the relation to ksmserver, surely kioslaves are not part of session management. I also don't really understand what would happen in the new event loop (select/poll) of the kioslave while the non-blocking I/O is happening. It's not like

Re: 2 kirigami fixes for a point release

gt; That's an easy thing to enforce, and we know it makes a huge difference to > code. > > Even if no-one comments, the extra delay of it running on your own > system delivers a lot. I fully agree. (CC'ing the developer lists) -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5

D27291: install smb as both smb:// and cifs://

dfaure added a comment. feverfew: you're probably looking at master while this is a patch for the 19.12 branch. See https://phabricator.kde.org/D26358 which happened in master. I guess that makes this commit ok for 19.12, but it has to be redone differently in master. REPOSITORY

D27291: install smb as both smb:// and cifs://

dfaure added a comment. Both are supported, but indeed json is preferred over installed .protocol files. You can use protocoltojson (from kio) to do the conversion. This is all technically unrelated to this commit, except that the nice cmake hack to generate two files isn't even

Re: Frameworks 5.67 re-spin request

26acddd9fe4ce0642ed2500a076429ea5fe4424fdf634f12e68 sources/kirigami2-5.67.1.tar.xz qqc2-desktop-style v5.67.1 a992466f2fe596b1ae34da91fd7e95e21b973a52 b2af96084e85096506cc109634234150cfc88a9fcf2d6887c55ef1787c046a44 sources/qqc2-desktop-style-5.67.1.tar.xz https://kde.org/info/kde-frameworks-5.67.0.

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > statjob.cpp:97 > + > +void StatJob::setDetails(short int details) > +{ When the declaration is in #if, the definition should be too (with s/ENABLED/BUILD/).

D26468: Tests: don't build benchmark test by default

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Building is still a good idea, to detect compilation breakage. Otherwise the code just bitrots over time. It's the running that should be made optional. REPOSITORY R241

D27158: Fix some compiler warnings

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig BRANCH l-compiler-warnings (branched from master) REVISION DETAIL https://phabricator.kde.org/D27158 To: ahmadsamir, #frameworks, dfaure, apol Cc: kde-frameworks-devel, LeGast00n,

D27106: Convert license statements to SPDX markers

dfaure added a comment. Can you add the repository name in the title of the review request? Otherwise they all look the same in emails. REPOSITORY R235 Attica REVISION DETAIL https://phabricator.kde.org/D27106 To: cordlandwehr Cc: dfaure, cgiboudeaux, kde-frameworks-devel, LeGast00n,

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. The approach makes sense to me. INLINE COMMENTS > kmountpoint.cpp:420 > +#else > +const QString realname = path; > #endif You should rename this variable, it's no longer

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