D29738: Fix service file specifying 'Run in terminal' giving an error code 100
This revision was automatically updated to reflect the committed changes. Closed by commit R241:6452a34cf01d: Fix service file specifying Run in terminal giving an error code 100 (authored by marten). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29738?vs=82823=82874 REVISION DETAIL https://phabricator.kde.org/D29738 AFFECTED FILES src/core/desktopexecparser.cpp src/gui/kprocessrunner.cpp To: marten, #frameworks, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29738: Fix service file specifying 'Run in terminal' giving an error code 100
marten created this revision. marten added reviewers: Frameworks, dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. marten requested review of this revision. REVISION SUMMARY https://bugs.kde.org/show_bug.cgi?id=421374 describes how a service file specified to run in a terminal gives an error saying that it cannot find 'konsole' or the user's configured terminal program. The problem is that KIO::DesktopExecParser::resultingArguments(), if service.terminal() is true, prepends the terminal application to the command line. If this is a relative path, as it is most likely to be (and will be in the default 'konsole' case), the "realExecutable" check in KProcessRunner::KProcessRunner() is triggered and the job aborts with an error message. This change expands the specified terminal executable into a full path in KIO::DesktopExecParser::resultingArguments(), and returns an error immediately if it cannot be found. This is then prepended to the command line. When KProcessRunner::KProcessRunner() checks the realExecutable (the first word of the command line) it will be an absolute path and the check will not fail. The order of the code blocks in KProcessRunner::KProcessRunner() is adjusted so that execParser.resultingArguments() is checked for being empty (an error return) before the first word of the command is accessed. This means that the "realExecutable = execParser.resultingArguments().at(0)" test will not assert if resultingArguments() is an empty list. TEST PLAN Built kio with this change, tested with the sample desktop file at https://bugs.kde.org/show_bug.cgi?id=421374#c8. The command correctly runs in a terminal. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29738 AFFECTED FILES src/core/desktopexecparser.cpp src/gui/kprocessrunner.cpp To: marten, #frameworks, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28647: Fix KIO::Scheduler::emitReparseSlaveConfiguration() to work if called twice in same process
marten added a comment. I've thought about this a bit more and can't find any explanation as to why the DBus signal does not loop back to the sending process as well as all other listeners. It may be an unspecified detail of the DBus implementation that could possibly change at any time and start happening. So maybe it would be better to retain the boolean flag and fix the ordering - in other words, keep things simple and implement the change as in the original diff. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28647 To: marten, #frameworks Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28647: Fix KIO::Scheduler::emitReparseSlaveConfiguration() to work if called twice in same process
marten added a comment.
I'm happy to work on the refactoring if you think it's the right thing to do.
Do you mean splitting SchedulerPrivate::slotReparseSlaveConfiguration() up
into two halves, the first part (KProtocolManager::reparseConfiguration through
to NetRC::self()->reload - reparsing the configuration in the current process)
being called directly and by the DBus signal, while the second half (check that
'proto' is applicable then iterate through the allSlaves() list) being called
only from emitReparseSlaveConfiguration? Something like:
void Scheduler::emitReparseSlaveConfiguration()
{
schedulerPrivate()->slotReparseSlaveConfiguration(...);
schedulerPrivate()->reparseOtherSlaves();
}
void SchedulerPrivate::slotReparseSlaveConfiguration(...)
{
KProtocolManager::reparseConfiguration();
,,,
NetRC::self()->reload();
}
void SchedulerPrivate::reparseOtherSlaves()
{
check protocol, return if not applicable
iterate over allSlaves()
{
slave->send(CMD_REPARSECONFIGURATION); slave->resetHost();
}
}
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D28647
To: marten, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28755: Breeze Icons cannot be built from read-only source location
This revision was automatically updated to reflect the committed changes. Closed by commit R266:0a5dd2972b62: Allow building from a read-only source location (authored by marten). REPOSITORY R266 Breeze Icons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28755?vs=79867=80219 REVISION DETAIL https://phabricator.kde.org/D28755 AFFECTED FILES CMakeLists.txt validate_svg.sh To: marten, #breeze, ngraham Cc: ngraham, pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D28755: Breeze Icons cannot be built from read-only source location
marten updated this revision to Diff 79867. marten added a comment. Yes, that would mean fewer changes to the validate_svg.sh script. REPOSITORY R266 Breeze Icons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28755?vs=79845=79867 REVISION DETAIL https://phabricator.kde.org/D28755 AFFECTED FILES CMakeLists.txt validate_svg.sh To: marten, #breeze Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28755: Breeze Icons cannot be built from read-only source location
marten created this revision. marten added a reviewer: Breeze. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. marten requested review of this revision. REVISION SUMMARY In the situation, for example, where a master source tree may be shared among a number of build systems via NFS (each build system needs its own writeable build directory, of course). However, building the Breeze icons needs write access to the source tree for the SVG validation check, which is run in CMAKE_CURRENT_SOURCE_DIR: [100%] Validating SVG breezeicons/validate_svg.sh: line 3: xmlerrors: Read-only file system rm: cannot remove 'xmlerrors': No such file or directory make[2]: *** [CMakeFiles/breeze-validate-svg.dir/build.make:58: CMakeFiles/breeze-validate-svg] Error 1 make[1]: *** [CMakeFiles/Makefile2:231: CMakeFiles/breeze-validate-svg.dir/all] Error 2 make: *** [Makefile:130: all] Error 2 This change writes the temporary XML error file to CMAKE_CURRENT_BINARY_DIR, which can be relied on to be writeable. TEST PLAN Build Breeze Icons with this change, observed that the SVG validation check completes with no 'read-only file system' errors. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D28755 AFFECTED FILES CMakeLists.txt validate_svg.sh To: marten, #breeze Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28647: Fix KIO::Scheduler::emitReparseSlaveConfiguration() to work if called twice in same process
marten created this revision. marten added reviewers: Frameworks, dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. marten requested review of this revision. REVISION SUMMARY I've been looking at porting Konqueror's User Agent Changer plugin to current KF5. The GUI is ported and working, but trying to change the user agent more than once in any given invocation of the browser does not seem to work. After changing the user agent string in kio_httprc, the plugin calls KIO::Scheduler::emitReparseSlaveConfiguration() to inform all running ioslaves of the change. This first of all calls slotReparseSlaveConfiguration() directly to update in the current process, and then sets m_ignoreConfigReparse to true and emits the reparseSlaveConfiguration() signal. The signal calls slotReparseSlaveConfiguration() via DBus; when activated in the same process slotReparseSlaveConfiguration() ignores the signal because m_ignoreConfigReparse is set, it is reset to false and simply returns. However, it appears that the signal does not get looped back to the current process; in other words, slotReparseSlaveConfiguration() is not called via the DBus signal. This means that m_ignoreConfigReparse is never reset to false and, the next time that KIO::Scheduler::emitReparseSlaveConfiguration() is called it has no effect. This can be confirmed by uncommenting the "Ignoring signal sent by myself" debug line in slotReparseSlaveConfiguration(), the message is never printed. The change fixes this by explicitly setting m_ignoreConfigReparse to false before the direct call of slotReparseSlaveConfiguration(), then to true before the DBus call. This inhibits the loopback signal in case it does happen, but ensures that the direct call is not ignored. TEST PLAN Tested with https://kluge.in-chemnitz.de/tools/browser.php to show the user agent as sent. Observed that, with this fix, the user agent can be changed as many times as required. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28647 AFFECTED FILES src/core/scheduler.cpp To: marten, #frameworks, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D27148: update d->m_file in ReadOnlyPart::setUrl()
marten added a comment. @ahmadsamir: Yes, the problem could be fixed in Dolphin, but that leaves a potential trap for any other KPart that reimplements openUrl(). As a minimum, if it is decided that the fix belongs in the application then KParts::ReadOnlyPart should have a caution in the API documentation that localFilePath() will only return a valid result if openUrl() or setLocalFilePath() has previously been called. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27148 To: pdabrowski, elvisangelaccio, ngraham, #frameworks, dfaure Cc: ahmadsamir, marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D27148: update d->m_file in ReadOnlyPart::setUrl()
marten added a comment. Yes, I'd concluded that the real place to fix the problem was at the KParts level, but not being a KParts expert wanted to leave that decision to its maintainers. +1 for the elegant fix. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27148 To: pdabrowski, elvisangelaccio, ngraham Cc: marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D7820: man ioslave: spurious numbers included in clang(1) man page
marten abandoned this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D7820 To: marten, #plasma, kfm-devel, mkoller Cc: ltoscano, kde-frameworks-devel, plasma-devel, aprcela, fprice, LeGast00n, sbergeron, jraleigh, fbampaloukas, alexde, GB_2, feverfew, ragreen, Pitel, meven, michaelh, spoorun, navarromorales, ZrenBot, firef, ngraham, andrebarros, bruns, himcesjf, emmanuelp, lesliezhai, ali-mohamed, mikesomov, jensreuterberg, abetts, sebas, apol, mart
D7820: man ioslave: spurious numbers included in clang(1) man page
marten added a comment. Confirmed that man:clang(1) now correctly displays the man page with no spurious numbers shown. Would be happy to abandon this review request. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D7820 To: marten, #plasma, kfm-devel, mkoller Cc: ltoscano, kde-frameworks-devel, plasma-devel, fprice, LeGast00n, jraleigh, fbampaloukas, alexde, GB_2, feverfew, ragreen, Pitel, meven, michaelh, spoorun, navarromorales, ZrenBot, firef, ngraham, andrebarros, bruns, himcesjf, emmanuelp, lesliezhai, ali-mohamed, mikesomov, jensreuterberg, abetts, sebas, apol, mart
D19332: KProcess compile fix for obsolete QProcess members in 5.13
marten abandoned this revision. marten added a comment. Appears to have been already implemented by https://phabricator.kde.org/R244:b2b873d612a018d33866ddfd497f87bb7dd79573 REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D19332 To: marten, #frameworks Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19439: kio_smb: Change incorrect use of QUrl::adjusted()
This revision was automatically updated to reflect the committed changes. Closed by commit R320:1b56cc62c93d: kio_smb: Change incorrect use of QUrl::adjusted() (authored by marten). REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19439?vs=52884=54927 REVISION DETAIL https://phabricator.kde.org/D19439 AFFECTED FILES smb/kio_smb_internal.cpp smb/kio_smb_internal.h To: marten, #plasma, #frameworks, dfaure, sitter Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D19439: kio_smb: Change incorrect use of QUrl::adjusted()
marten added a comment. Ping - anyone able to review please? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D19439 To: marten, #plasma, #frameworks Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D19847: Delete/Trash confirmation dialogue: Fix misleading title and make consistent
This revision was automatically updated to reflect the committed changes. Closed by commit R241:534ebe58c4ca: Delete/Trash confirmation dialogue: Fix misleading title (authored by marten). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19847?vs=54145=54178 REVISION DETAIL https://phabricator.kde.org/D19847 AFFECTED FILES src/widgets/jobuidelegate.cpp To: marten, #frameworks, dfaure, #vdg, ngraham Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D19847: Delete/Trash confirmation dialogue: Fix misleading title and make consistent
marten created this revision. marten added reviewers: Frameworks, dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. marten requested review of this revision. REVISION SUMMARY The misleading title that brought this dialogue to my attention is where there is only one selected item to move to the trash. In this case the question wording is correct but the message box title says "Delete Permanently?". This is the code for "case Trash" and "if (prettyList.count() == 1)" in KIO::JobUiDelegate::askDeleteConfirmation(). This should be corrected to say "Move to Trash" as in the other conditional below. There are some other anomalies which are also corrected in this diff: a) The "Delete Permanently" message box title has a question mark, but "Move to Trash" does not. I can't find a HIG rule for which of those is correct, but for consistency and in accordance with "Do not use the title to explain what to do in the dialog – that’s the purpose of the main instruction" the question marks are removed. b) Capitalisation of "Trash" made consistent in the question for trash of multiple items. TEST PLAN Built KIO with these changes, checked all combinations of delete/trash operations in Dolphin and verified that the correct message box title and question is shown. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19847 AFFECTED FILES src/widgets/jobuidelegate.cpp To: marten, #frameworks, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19332: KProcess compile fix for obsolete QProcess members in 5.13
marten updated this revision to Diff 52898. marten added a comment. Use the same condition as the Qt header uses REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19332?vs=52601=52898 REVISION DETAIL https://phabricator.kde.org/D19332 AFFECTED FILES src/lib/io/kprocess.h To: marten, #frameworks Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns
D7820: man ioslave: spurious numbers included in clang(1) man page
marten added a reviewer: kfm-devel. Herald added projects: Dolphin, Frameworks. Herald added a subscriber: kde-frameworks-devel. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D7820 To: marten, #plasma, kfm-devel Cc: kde-frameworks-devel, plasma-devel, jraleigh, alexde, GB_2, feverfew, ragreen, Pitel, michaelh, spoorun, navarromorales, ZrenBot, firef, ngraham, andrebarros, bruns, emmanuelp, lesliezhai, ali-mohamed, mikesomov, jensreuterberg, abetts, sebas, apol, mart
D9033: man ioslave: asserts trying to display pam(8)
marten added a reviewer: kfm-devel. Herald added projects: Dolphin, Frameworks. Herald added a subscriber: kde-frameworks-devel. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D9033 To: marten, #plasma, kfm-devel Cc: kde-frameworks-devel, apol, plasma-devel, jraleigh, gennad, alexde, GB_2, feverfew, ragreen, Pitel, michaelh, spoorun, navarromorales, ZrenBot, firef, ngraham, andrebarros, bruns, skadinna, emmanuelp, lesliezhai, ali-mohamed, mikesomov, jensreuterberg, abetts, sebas, mart
D19439: kio_smb: Change incorrect use of QUrl::adjusted()
marten created this revision. marten added reviewers: Plasma, Frameworks. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. marten requested review of this revision. REVISION SUMMARY Compiling this module produces a number of warnings all referring to the same source: kio-extras/smb/kio_smb_internal.h: In member function 'void SMBUrl::setFileName(const QString&)': kio-extras/smb/kio_smb_internal.h:96: warning: ignoring return value of 'QUrl QUrl::adjusted(QUrl::FormattingOptions) const', declared with attribute nodiscard The problem code is trying to make a URL for a partial file being uploaded/copied. However, because QUrl::adjusted() does not modify the URL in place, the copy operation works but does not use the intended partial file name. For example, if the destination file is "smb://server/dir/foobar.ext", the partial file name used will be "smb://server/dir/foobar.extfoobar.ext.part". The complete file will eventually be saved under the correct name, but the name will be wrong if the transfer is aborted and the user wants to find the partial file. There is also the possibility of an error if the file name is long and the destination server has a length limit, because it effectively doubles in length. The change fixes the problem and simplifies the code by simply appending ".part" to the supplied URL path directly. No other part of the ioslave uses SMBUrl::setFileName() or SMBUrl::partUrl(), so this does not raise any compatibility problems. TEST PLAN Built kio_smb with this change. Observed no compiler warning messages, correct copying to and from SMB, and use of the correct partial file name. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D19439 AFFECTED FILES smb/kio_smb_internal.cpp smb/kio_smb_internal.h To: marten, #plasma, #frameworks Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D19371: Tags ioslave: Do not crash for the "tags:/" URL
This revision was automatically updated to reflect the committed changes. Closed by commit R293:604a2f402da7: Check string length to avoid crash for tags:/ URL (authored by marten). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D19371?vs=52681=52703#toc REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19371?vs=52681=52703 REVISION DETAIL https://phabricator.kde.org/D19371 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp To: marten, #baloo, ngraham Cc: kde-frameworks-devel, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D19371: Tags ioslave: Do not crash for the "tags:/" URL
marten created this revision. marten added a reviewer: Baloo. Herald added projects: Frameworks, Baloo. Herald added a subscriber: kde-frameworks-devel. marten requested review of this revision. REVISION SUMMARY Bug https://bugs.kde.org/show_bug.cgi?id=400594 was fixed by https://commits.kde.org/baloo/f4dd3f7bab790734b47a31c70fbb68297d4d3062 which checks for the URL starting with "tags://" and considering this to be an invalid URL. This is fine, but the problem is that if the URL "tags:/" is passed in the string is not long enough and QString::at() asserts. This is seen as a crash of the ioslave every time the right click menu is popped up in Konqueror (on any sort of view), or from the command line by doing "kioclient5 ls tags:/": kio_tags(11869) unknown: ASSERT: "uint(i) < uint(size())" in file /usr/include/qt5/QtCore/qstring.h, line 934 #10 0x7f3d402fe0bc in qt_assert(char const*, char const*, int) () from /usr/lib64/libQt5Core.so.5 #11 0x7f3d36eec337 in QString::at (i=6, this=0x7ffd4531d8a8) at /usr/include/qt5/QtCore/qstring.h:934 #12 Baloo::TagsProtocol::parseUrl (this=this@entry=0x7ffd4531da60, url=..., flags=...) at baloo/baloo/src/kioslaves/tags/kio_tags.cpp:299 #13 0x7f3d36eedda6 in Baloo::TagsProtocol::listDir (this=0x7ffd4531da60, url=...) at baloo/baloo/src/kioslaves/tags/kio_tags.cpp:59 #14 0x7f3d3663e1de in KIO::SlaveBase::dispatch (this=0x7ffd4531da70, command=71, data=...) at kio/src/core/slavebase.cpp:1176 #15 0x7f3d3663f78e in KIO::SlaveBase::dispatchLoop (this=0x7ffd4531da70) at kio/src/core/slavebase.cpp:325 #16 0x7f3d36ef1943 in kdemain (argc=, argv=) at baloo/baloo/src/kioslaves/tags/kio_tags.cpp:486 The QString::at() needs to be guarded by a length check. TEST PLAN Built Baloo with this change, verified that no crash happens when using the URL "tags:/" REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D19371 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp To: marten, #baloo Cc: kde-frameworks-devel, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D19332: KProcess compile fix for obsolete QProcess members in 5.13
marten added inline comments. INLINE COMMENTS > apol wrote in kprocess.h:332 > Also should check for QT_DISABLE_DEPRECATED_BEFORE no? Do you mean conditionalise instead on #if !QT_DEPRECATED_SINCE(5, 13), so that if the functions are not disabled in Qt then they are hidden here? That would work, although QT_DEPRECATED_SINCE is not public API or defined in the Qt documentation. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D19332 To: marten, #frameworks Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19332: KProcess compile fix for obsolete QProcess members in 5.13
marten created this revision. marten added a reviewer: Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. marten requested review of this revision. REVISION SUMMARY The functions QProcess::setReadChannelMode() and QProcess::readChannelMode() are obsolete, but were not specially treated in any way in earlier Qt versions. In 5.13 they are actually disabled in the header files, using QT_DEPRECATED_SINCE(5, 13). KDE PIM optimistically sets QT_DISABLE_DEPRECATED_BEFORE=0x06 (since May 2017), so the build fails because these functions are no longer available: /usr/include/KF5/KCoreAddons/kprocess.h:332: error: no members matching 'QProcess::setReadChannelMode' in 'class QProcess' /usr/include/KF5/KCoreAddons/kprocess.h:333: error: no members matching 'QProcess::readChannelMode' in 'class QProcess' This does not affect the remainder of Frameworks or Plasma (yet), because they do not use this QT_DISABLE_DEPRECATED_BEFORE setting. This change makes the use of these two members conditional on the Qt version (5.12 or earlier). TEST PLAN Built kcoreaddons with this change, checked that applications using this header build correctly without the above errors. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D19332 AFFECTED FILES src/lib/io/kprocess.h To: marten, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten added a comment. In https://phabricator.kde.org/D7823#164879, @cfeck wrote: > Are the copies in the attic directory still needed for compatibility reasons, or can they get removed? No needed by anything within Frameworks, Plasma or Applications as far as I'm aware. Shall I remove them? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 To: marten, #frameworks, #build_system, cgiboudeaux Cc: cgiboudeaux, cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
This revision was automatically updated to reflect the committed changes. Closed by commit R240:ee5b036c1df4: Add FindGLIB2.cmake and FindPulseAudio.cmake (authored by marten). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D7823?vs=19611=21961#toc REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7823?vs=19611=21961 REVISION DETAIL https://phabricator.kde.org/D7823 AFFECTED FILES docs/find-module/FindGLIB2.rst docs/find-module/FindPulseAudio.rst find-modules/FindGLIB2.cmake find-modules/FindPulseAudio.cmake To: marten, #frameworks, #build_system, cgiboudeaux Cc: cgiboudeaux, cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten added a comment. Ping anyone? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 To: marten, #frameworks, #build_system Cc: cgiboudeaux, cfeck, heikobecker
D8296: Use Ctrl+, as the standard shortcut for "Configure "
marten added a comment. FYI: KMail uses Ctrl+comma and Ctrl+dot as standard shortcuts for "Collapse all threads" and "Expand all threads" respectively. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D8296 To: ngraham, #frameworks, #vdg Cc: marten, graesslin, broulik, #frameworks
D7841: kioexec: Do not start to watch a temporary copy until after the copy is complete
This revision was automatically updated to reflect the committed changes. Closed by commit R241:c76c1486ec79: kioexec: Watch the file when it has finished copying (authored by marten). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7841?vs=19864=19885 REVISION DETAIL https://phabricator.kde.org/D7841 AFFECTED FILES src/kioexec/main.cpp To: marten, #frameworks, dfaure Cc: elvisangelaccio, dfaure
D7841: kioexec: Do not start to watch a temporary copy until after the copy is complete
marten updated this revision to Diff 19864. marten added a comment. Updated in accordance with review comments. Go easy - it's the first lambda that I've submitted... REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7841?vs=19565=19864 REVISION DETAIL https://phabricator.kde.org/D7841 AFFECTED FILES src/kioexec/main.cpp To: marten, #frameworks, dfaure Cc: elvisangelaccio, dfaure
D7841: kioexec: Do not start to watch a temporary copy until after the copy is complete
marten marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7841 To: marten, #frameworks, dfaure Cc: elvisangelaccio, dfaure
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten marked an inline comment as done. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 To: marten, #frameworks, #build_system Cc: cgiboudeaux, cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten updated this revision to Diff 19611. marten added a comment. Diff updated to move library target definition after find_package_handle_standard_args. If these modules are accepted into ECM then KMix needs a further commit anyway, in order to remove the final requirement for KDELibs4Support. So the correct variable names can be set at this point (I have already tested this configuration). Other packages requiring PA and/or GLib may need to do the same when transitioning to use the ECM versions. REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7823?vs=19576=19611 REVISION DETAIL https://phabricator.kde.org/D7823 AFFECTED FILES find-modules/FindGLIB2.cmake find-modules/FindPulseAudio.cmake To: marten, #frameworks, #build_system Cc: cgiboudeaux, cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten marked an inline comment as done. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 To: marten, #frameworks, #build_system Cc: cgiboudeaux, cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten updated this revision to Diff 19576. marten marked an inline comment as done. marten added a comment. Updated as per review comments. REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7823?vs=19570=19576 REVISION DETAIL https://phabricator.kde.org/D7823 AFFECTED FILES find-modules/FindGLIB2.cmake find-modules/FindPulseAudio.cmake To: marten, #frameworks, #build_system Cc: cgiboudeaux, cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten marked 17 inline comments as done. marten added a comment. Diff updated. INLINE COMMENTS > cgiboudeaux wrote in FindPulseAudio.cmake:8-19 > It's a new module, change PULSEAUDIO to PulseAudio to match the module name. PULSEAUDIO -> PulseAudio changed throughout REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 To: marten, #frameworks, #build_system Cc: cgiboudeaux, cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten updated this revision to Diff 19570. marten added a comment. Modules updated with standard header, documentation and copyright; endif() used throughout. Yes, Phonon does not use ECM but the modules there are very different to those in ECM anyway. Within Plasma+applications these modules are needed by plasma-desktop (applets/kimpanel and kcms/phonon) as well as KMix. kcms/phonon does not have a local copy, so this will break or a local copy will be needed if it is ported away from KDELibs4Support. REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7823?vs=19531=19570 REVISION DETAIL https://phabricator.kde.org/D7823 AFFECTED FILES find-modules/FindGLIB2.cmake find-modules/FindPulseAudio.cmake To: marten, #frameworks, #build_system Cc: cgiboudeaux, cfeck, heikobecker
D7841: kioexec: Do not start to watch a temporary copy until after the copy is complete
marten created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY As described in bug https://bugs.kde.org/show_bug.cgi?id=384500, there appears to be a problem when the receiving application of a file needs a temporary copy to be made (because of %F/%f in its desktop file). The kded file watching module is told to watch the file too early, before the ioslave has even started to copy it. This means that when the copy is complete it will receive a dirty signal (on file creation) and the user will immediately be prompted to re-upload. This change moves the file watch operation to after the file copy job is complete. At this point the file is in a stable state and hence the dirty signal and the prompt will not happen, unless the file really is subsequently modified. TEST PLAN Built and installed kio with this change, checked remote file opening as per the bug. Verified that the kded module is not told to watch the file until after the copy is complete, and that there is no re-upload prompt unless the file is actually modified. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7841 AFFECTED FILES src/kioexec/main.cpp To: marten, #frameworks Cc: elvisangelaccio, dfaure
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten added a comment. It's the same source really - the only differences between those in kdelibs4support and ecm/attic are that the former uses endif(same_as_if) and the latter uses endif(). Nothing else within Frameworks, Plasma or applications uses anything from ecm/attic directly. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 To: marten, #frameworks, #build_system Cc: cfeck, heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten added a subscriber: heikobecker. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 To: marten, #frameworks, #build_system Cc: heikobecker
D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
marten created this revision. Restricted Application added projects: Frameworks, Build System. REVISION SUMMARY These modules are used in a number of places within Frameworks, Plasma and dependencies: FindGLIB2: plasma-desktop/applets/kimpanel/cmake/FindGLIB2.cmake kdelibs4support/cmake/modules/FindGLIB2.cmake ecm/attic/modules/FindGLIB2.cmake phonon/cmake/FindGLIB2.cmake polkit-qt/cmake/modules/FindGLIB2.cmake FindPulseAudio: kdelibs4support/cmake/modules/FindPulseAudio.cmake ecm/attic/modules/FindPulseAudio.cmake phonon/cmake/FindPulseAudio.cmake Apart from those in Phonon, each set is all the same apart from minor formatting differences. These two modules will also be required by KMix when it is finally ported away from KDElibs4Support. Since these common files are used in multiple places, it makes sense to have them included in ECM instead of making local copies. TEST PLAN Built and installed ECM with these two modules added, configured KMix with KDELibs4Support removed. GLIB2 and PulseAudio are found correctly using the modules installed by ECM. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7823 AFFECTED FILES find-modules/FindGLIB2.cmake find-modules/FindPulseAudio.cmake To: marten, #frameworks, #build_system
D5871: Remove obviously wrongly-named symbolic links
This revision was automatically updated to reflect the committed changes. Closed by commit R267:1b641f89d752: Remove obviously wrongly-named symbolic links (authored by marten). REPOSITORY R267 Oxygen Icons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5871?vs=14560=14564 REVISION DETAIL https://phabricator.kde.org/D5871 AFFECTED FILES 128x128/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 16x16/mimetypes/text-x-generic.svapplication-x-awk.png 16x16/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 22x22/mimetypes/text-x-generic.svapplication-x-awk.png 22x22/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 256x256/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 32x32/mimetypes/text-x-generic.svapplication-x-awk.png 32x32/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 48x48/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 64x64/mimetypes/text-x-generic.svapplication-x-awk.png 64x64/mimetypes/text-x-generic.svapplicatiopn-x-awk.png To: marten, #plasma, davidedmundson Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D5871: Remove obviously wrongly-named symbolic links
marten created this revision. Restricted Application added projects: Plasma, Frameworks. Restricted Application added subscribers: Frameworks, plasma-devel. REVISION SUMMARY It's not clear where these entries came from, but they are clearly a mistake - either scripting or copy-and-paste. The intended 'text-x-generic' and 'application-x-awk' icons still exist. TEST PLAN Built and installed oxygen-icons5 with these changes, checked correct installation of icons. REPOSITORY R267 Oxygen Icons REVISION DETAIL https://phabricator.kde.org/D5871 AFFECTED FILES 128x128/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 16x16/mimetypes/text-x-generic.svapplication-x-awk.png 16x16/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 22x22/mimetypes/text-x-generic.svapplication-x-awk.png 22x22/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 256x256/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 32x32/mimetypes/text-x-generic.svapplication-x-awk.png 32x32/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 48x48/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 64x64/mimetypes/text-x-generic.svapplication-x-awk.png 64x64/mimetypes/text-x-generic.svapplicatiopn-x-awk.png To: marten, #plasma Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
[Differential] [Closed] D4463: Konqueror can no longer be selected as default file manager
This revision was automatically updated to reflect the committed changes. Closed by commit R226:c27a79e2dbab: Restore the ability to select Konqueror as the default file manager (authored by marten). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D4463?vs=10973=10978#toc REPOSITORY R226 Konqueror CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4463?vs=10973=10978 REVISION DETAIL https://phabricator.kde.org/D4463 AFFECTED FILES CMakeLists.txt kfmclient_dir.desktop EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks, dfaure
[Differential] [Request, 105 lines] D4463: Konqueror can no longer be selected as default file manager
marten created this revision.
marten added reviewers: Frameworks, dfaure.
marten set the repository for this revision to R226 Konqueror.
REVISION SUMMARY
Konqueror was originally able to be selected as the default file manager by
using kcmshell5 componentchooser ("Default Applications"). Now it does not
appear in the list of file manager applications.
This appears to be a result of commit
https://phabricator.kde.org/R226:456dec00e158ede269ec876cb28280f5b8d23fbb of 26
Sep 2016 ("More cleanup of the "profile" concept") removing
kfmclient_dir.desktop and no longer installing it. It's not clear whether this
was intentional, but this change restores that file and therefore the ability
to select Konqueror as a file manager.
TEST PLAN
Built konqueror with this change, checked that it can be selected as the
default file manager and that it is used correctly for that.
REPOSITORY
R226 Konqueror
REVISION DETAIL
https://phabricator.kde.org/D4463
AFFECTED FILES
CMakeLists.txt
kfmclient_dir.desktop
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: marten, #frameworks, dfaure
[Differential] [Commented On] D4188: Suppress warning message "No metadata file in the package..." when using desktop slideshow
marten added a comment. Agreed that a decision needs to be made, but I'm not an expert on the KPackage system and so wasn't sure whether the warning may be useful to package developers in some cases - in which case leaving it commented out would make it easier to reinstate than removing it entirely. REPOSITORY R290 KPackage REVISION DETAIL https://phabricator.kde.org/D4188 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks, #plasma Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Request, 2 lines] D4188: Suppress warning message "No metadata file in the package..." when using desktop slideshow
marten created this revision. marten added reviewers: Frameworks, Plasma. marten set the repository for this revision to R290 KPackage. Restricted Application added projects: Plasma, Frameworks. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY As described in https://bugs.kde.org/show_bug.cgi?id=350148, this message appears three times every time the slideshow plugin changes the background image. It cannot be disabled via Qt logging rules. Apart from possible interest to package developers this message serves no useful purpose, so suppressing it would avoid clutter in the user's session log. TEST PLAN Built KPackage with this change, all autotests pass. Observed correct operation of Plasma desktop and slideshow background, with no log messages. REPOSITORY R290 KPackage REVISION DETAIL https://phabricator.kde.org/D4188 AFFECTED FILES src/kpackage/package.cpp EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks, #plasma Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Closed] D4080: KAboutData: Document that bug email address can also be a URL
This revision was automatically updated to reflect the committed changes. Closed by commit R244:bbf76499d9e2: KAboutData: Document that bug email address can also be a URL (authored by marten). REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4080?vs=10032=10124 REVISION DETAIL https://phabricator.kde.org/D4080 AFFECTED FILES src/lib/kaboutdata.cpp src/lib/kaboutdata.h EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks, mpyne Cc: mpyne
[Differential] [Updated] D4072: Bug reporter: Allow a URL (not just as an email address) for custom reporting
marten added a dependent revision: D4080: KAboutData: Document that bug email address can also be a URL. REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D4072 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks Cc: aacid
[Differential] [Updated] D4080: KAboutData: Document that bug email address can also be a URL
marten added a dependency: D4072: Bug reporter: Allow a URL (not just as an email address) for custom reporting. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D4080 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks
[Differential] [Request, 28 lines] D4080: KAboutData: Document that bug email address can also be a URL
marten created this revision. marten added a reviewer: Frameworks. marten set the repository for this revision to R244 KCoreAddons. Restricted Application added a project: Frameworks. REVISION SUMMARY Differential https://phabricator.kde.org/D4072 requests a change to the bug reporting system in order that the reporting address can be a URL, in addition to an email address as as present. This requires no actual code changes to KAboutData, but as suggested the parameters and private variables are renamed and the API documentation updated to indicate this. TEST PLAN Built kcoreaddons with these changes, all autotests pass. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D4080 AFFECTED FILES src/lib/kaboutdata.cpp src/lib/kaboutdata.h EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks
[Differential] [Commented On] D4072: Bug reporter: Allow a URL (not just as an email address) for custom reporting
marten added a comment. Yes, I'll submit a separate diff for that. Fortunately BC, since there are no actual code changes needed - only the parameter name. The getter/setter bugAddress/setBugAddress will suffice as they are (with apidoc changes). REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D4072 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks Cc: aacid
[Differential] [Request, 89 lines] D4072: Bug reporter: Allow a URL (not just as an email address) for custom reporting
marten created this revision. marten added a reviewer: Frameworks. marten set the repository for this revision to R263 KXmlGui. Restricted Application added a project: Frameworks. REVISION SUMMARY The rather old bug https://bugs.kde.org/show_bug.cgi?id=277142, although raised against KDE4 kdelibs, still applies to KDE Frameworks. The bug reporting address is assumed to be an email address in the "Report Bug" dialogue (KBugReport) and in the "About" dialogue "Author" tab (the "Please report bugs to..." link). This patch corrects this for the KXmlGui framework. If submission is not to bugs.kde.org then KAboutApplicationDialog attempts to parse the bug address as a URL; only if the resulting URL has no scheme is "mailto:; assumed. This is then presented to the user as the clickable link. KBugReport currently handles only two cases, either the wizard at bugs.kde.org or an email address. This is extended to handle a third case, a custom URL. To distinguish the three cases the d->submitBugWeb boolean is changed to d->bugDestination, a KBugReportPrivate::BugDestination enum. In the third case, the dialogue is shown in the same way as for bugs.kde.org reporting, but with different wording for the message and button. The manual test at tests/kbugreporttest.cpp is extended to test this third case. TEST PLAN Checked for correct operation of tests/kbugreporttest for the three cases. Checked for correct appearance and operation of the "About application" dialogue and "Report Bug" for KWrite, a standard KDE application reporting to bugs.kde.org. Checked appearance and operation of those for my own application using a custom bug reporting address, as an email address and as a http: URL. Checked for correct operation of DrKonqi with a standard bug reporting address. REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D4072 AFFECTED FILES src/kaboutapplicationdialog.cpp src/kbugreport.cpp tests/kbugreporttest.cpp EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: marten, #frameworks
Re: Review Request 129617: Restore the "Embeddable Image Viewer" provided by KHTML
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129617/ --- (Updated Dec. 6, 2016, 6:53 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and David Faure. Changes --- Submitted with commit 842ac3256137b0d4fde129844ff6d5aac9da0090 by Jonathan Marten to branch master. Repository: khtml Description --- This is currently built and installed, but it is not usable because it needed to be converted to the new plugin system. Trying to use the viewer gives the standard Qt message "Failed to extract plugin meta data...". This change uses kservice_desktop_to_json and K_PLUGIN_FACTORY_WITH_JSON to embed the plugin data as is now required, eliminating the KHTMLImageFactory. Yes, the viewer is primitive... but is ought to be made to work as part of a basic installation, as it is the only image viewer part available if nether gwenview nor okular is installed. Diffs - src/CMakeLists.txt 99b6ce6e src/khtmlimage.cpp 2f1efe11 src/khtmlimage.desktop 00ccd72a src/khtmlimage.h eaaf5c76 src/khtmlimage_init.cpp ff93bb4c Diff: https://git.reviewboard.kde.org/r/129617/diff/ Testing --- Built khtml with this change, checked operation of the embedded viewer part in Konqueror and other applications. Thanks, Jonathan Marten
Review Request 129617: Restore the "Embeddable Image Viewer" provided by KHTML
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129617/ --- Review request for KDE Frameworks and David Faure. Repository: khtml Description --- This is currently built and installed, but it is not usable because it needed to be converted to the new plugin system. Trying to use the viewer gives the standard Qt message "Failed to extract plugin meta data...". This change uses kservice_desktop_to_json and K_PLUGIN_FACTORY_WITH_JSON to embed the plugin data as is now required, eliminating the KHTMLImageFactory. Yes, the viewer is primitive... but is ought to be made to work as part of a basic installation, as it is the only image viewer part available if nether gwenview nor okular is installed. Diffs - src/CMakeLists.txt 99b6ce6e src/khtmlimage.cpp 2f1efe11 src/khtmlimage.desktop 00ccd72a src/khtmlimage.h eaaf5c76 src/khtmlimage_init.cpp ff93bb4c Diff: https://git.reviewboard.kde.org/r/129617/diff/ Testing --- Built khtml with this change, checked operation of the embedded viewer part in Konqueror and other applications. Thanks, Jonathan Marten
Re: Review Request 128116: KStandardAction::showStatusbar: fix copy/paste error
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128116/ --- (Updated June 7, 2016, 2:45 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 3f7465b8d7c1649b54eb6ce72e88d2a3080e2234 by Jonathan Marten to branch master. Repository: kconfigwidgets Description --- This function is intended to return the action for the StandardAction of ShowStatusbar. However, there seems to be an obvious copy/paste error here; the action for ShowMenubar is retrieved instead. This patch corrects this. Diffs - src/kstandardaction.cpp 29b7e9b Diff: https://git.reviewboard.kde.org/r/128116/diff/ Testing --- Built tier3/kconfigwidgets with this change, and tested with an application which looks up the action by name. Without this change there is a segfault because the wrong action is found; with the change the correct action is found and works. The autotest does not detect this problem because it only checks that an action is returned; it doesn't check that it is the correct one! Fixing this would need an extra test of each returned action to check that its name() is correct - not sure whether this is necessary? Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127929: K4TimeZoneWidget: flag images not displayed because of incorrect path
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127929/ --- (Updated May 16, 2016, 6:03 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 75a55d449f2f070c5410e47a9529a984403c3297 by Jonathan Marten to branch master. Repository: kdelibs4support Description --- In KDE4 this widget displayed the appropriate country flag at the start of the continent/city column. This no longer happens because the path used to find the flag image files is incorrect: it is still using the old share/locale/l10n/CC/flag.png location. In KF5 this is now share/kf5/locale/countries/CC/flag.png. This change corrects the path and comment. Diffs - src/kdeui/k4timezonewidget.cpp 70ff79f Diff: https://git.reviewboard.kde.org/r/127929/diff/ Testing --- Built kdelibs4support with this change, checked correct display of flag column in dialogue. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 127929: K4TimeZoneWidget: flag images not displayed because of incorrect path
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127929/ --- Review request for KDE Frameworks. Repository: kdelibs4support Description --- In KDE4 this widget displayed the appropriate country flag at the start of the continent/city column. This no longer happens because the path used to find the flag image files is incorrect: it is still using the old share/locale/l10n/CC/flag.png location. In KF5 this is now share/kf5/locale/countries/CC/flag.png. This change corrects the path and comment. Diffs - src/kdeui/k4timezonewidget.cpp 70ff79f Diff: https://git.reviewboard.kde.org/r/127929/diff/ Testing --- Built kdelibs4support with this change, checked correct display of flag column in dialogue. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127657: Improve the layout of the BrowserOpenOrSaveQuestion dialogue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127657/ --- (Updated April 16, 2016, 4:27 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and David Faure. Changes --- Submitted with commit ab2f9fb2b7780c913a417b01700e33d3bb986f9c by Jonathan Marten to branch master. Repository: kparts Description --- This dialogue is used, e.g. in Konqueror, when asking to open or save the target of a clicked link. As can be seen from the screen shot, its layout is squashed due to explicitly setting layout margin and spacing instead of allowing Qt to use the standard style values. This change removes the explicit setting of margin/spacing. Diffs - src/browseropenorsavequestion.cpp ce47525 Diff: https://git.reviewboard.kde.org/r/127657/diff/ Testing --- Built kparts with this change, checked appearance and operation of the dialogue (see screen shot). File Attachments Screen shot before https://git.reviewboard.kde.org/media/uploaded/files/2016/04/15/89690d9d-7868-425b-85b0-c4f9936eaf85__konqueror-openwith-frameworks-breeze-before.png Screen shot after https://git.reviewboard.kde.org/media/uploaded/files/2016/04/15/60aa5103-533a-40e9-bc92-f0da3b9b5b3e__konqueror-openwith-frameworks-breeze-after.png Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 127657: Improve the layout of the BrowserOpenOrSaveQuestion dialogue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127657/ --- Review request for KDE Frameworks and David Faure. Repository: kparts Description --- This dialogue is used, e.g. in Konqueror, when asking to open or save the target of a clicked link. As can be seen from the screen shot, its layout is squashed due to explicitly setting layout margin and spacing instead of allowing Qt to use the standard style values. This change removes the explicit setting of margin/spacing. Diffs - src/browseropenorsavequestion.cpp ce47525 Diff: https://git.reviewboard.kde.org/r/127657/diff/ Testing --- Built kparts with this change, checked appearance and operation of the dialogue (see screen shot). File Attachments Screen shot before https://git.reviewboard.kde.org/media/uploaded/files/2016/04/15/89690d9d-7868-425b-85b0-c4f9936eaf85__konqueror-openwith-frameworks-breeze-before.png Screen shot after https://git.reviewboard.kde.org/media/uploaded/files/2016/04/15/60aa5103-533a-40e9-bc92-f0da3b9b5b3e__konqueror-openwith-frameworks-breeze-after.png Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127046: Move popup menu image actions into a submenu
> On Feb. 12, 2016, 2:37 p.m., Thomas Pfeiffer wrote: > > I wouldn't say that all actions for images are rarely used. > > I don't see why viewing, copying or saving an image is less frequently done > > than the same things regarding the link. Can't we have those three still on > > the first level and then a "More..." option with further actions? > > Jonathan Marten wrote: > I wouldn't have said "rarely used": certainly in my experience I use all > of these options occasionally, but nowhere near as often as "Open in new > tab", "Save link as", "Open with" etc. > > Obviously any of the image options could appear at the top level instead > of in a submenu, but however they are arranged there will be dispute over > what should be where. The only sensible options I can think of to satisfy > everyone would be "everything at top level" (as of now), or "everything in a > submenu" (which groups them all together, provides a place to show the name > of the image, and minimises the size of the menu). > > Luigi Toscano wrote: > What other browsers do? Mozilla: has "View Image", "Copy Image", "Copy Image Location", (separator), "Save Image As", "Share this image", "Email Image", "Set as desktop background", "View Image Info". Far too normal many IMHO, the menu height doubles over an image. Chromium: has "Open image in new tab", "Save image as", "Copy image", "Copy image address", "Search Google for image". - Jonathan ------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127046/#review92299 --- On Feb. 11, 2016, 6:18 p.m., Jonathan Marten wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127046/ > --- > > (Updated Feb. 11, 2016, 6:18 p.m.) > > > Review request for KDE Frameworks and KDE Usability. > > > Repository: khtml > > > Description > --- > > The popup menu over an image (with actions "Save Image As..." etcetera, see > screen shot) has in total 6 image actions which are not frequently used but > make the menu very long. Moving these actions onto a submenu makes the top > level menu smaller and more logically grouped. > > > Diffs > - > > src/khtml_ext.cpp 0f522f4 > > Diff: https://git.reviewboard.kde.org/r/127046/diff/ > > > Testing > --- > > Built KHTML with these changes, tested in Konqueror. > > > File Attachments > > > Popup menu before > > https://git.reviewboard.kde.org/media/uploaded/files/2016/02/11/55c12311-73a3-472a-8a9a-6b6b9fce9de7__before.png > Popup menu after > > https://git.reviewboard.kde.org/media/uploaded/files/2016/02/11/313dbe3b-efb4-475c-bf76-8516f5bc85b4__after.png > > > Thanks, > > Jonathan Marten > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127046: Move popup menu image actions into a submenu
> On Feb. 12, 2016, 2:37 p.m., Thomas Pfeiffer wrote: > > I wouldn't say that all actions for images are rarely used. > > I don't see why viewing, copying or saving an image is less frequently done > > than the same things regarding the link. Can't we have those three still on > > the first level and then a "More..." option with further actions? I wouldn't have said "rarely used": certainly in my experience I use all of these options occasionally, but nowhere near as often as "Open in new tab", "Save link as", "Open with" etc. Obviously any of the image options could appear at the top level instead of in a submenu, but however they are arranged there will be dispute over what should be where. The only sensible options I can think of to satisfy everyone would be "everything at top level" (as of now), or "everything in a submenu" (which groups them all together, provides a place to show the name of the image, and minimises the size of the menu). - Jonathan --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127046/#review92299 ------- On Feb. 11, 2016, 6:18 p.m., Jonathan Marten wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127046/ > --- > > (Updated Feb. 11, 2016, 6:18 p.m.) > > > Review request for KDE Frameworks and KDE Usability. > > > Repository: khtml > > > Description > --- > > The popup menu over an image (with actions "Save Image As..." etcetera, see > screen shot) has in total 6 image actions which are not frequently used but > make the menu very long. Moving these actions onto a submenu makes the top > level menu smaller and more logically grouped. > > > Diffs > - > > src/khtml_ext.cpp 0f522f4 > > Diff: https://git.reviewboard.kde.org/r/127046/diff/ > > > Testing > --- > > Built KHTML with these changes, tested in Konqueror. > > > File Attachments > > > Popup menu before > > https://git.reviewboard.kde.org/media/uploaded/files/2016/02/11/55c12311-73a3-472a-8a9a-6b6b9fce9de7__before.png > Popup menu after > > https://git.reviewboard.kde.org/media/uploaded/files/2016/02/11/313dbe3b-efb4-475c-bf76-8516f5bc85b4__after.png > > > Thanks, > > Jonathan Marten > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 127046: Move popup menu image actions into a submenu
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127046/ --- Review request for KDE Frameworks and KDE Usability. Repository: khtml Description --- The popup menu over an image (with actions "Save Image As..." etcetera, see screen shot) has in total 6 image actions which are not frequently used but make the menu very long. Moving these actions onto a submenu makes the top level menu smaller and more logically grouped. Diffs - src/khtml_ext.cpp 0f522f4 Diff: https://git.reviewboard.kde.org/r/127046/diff/ Testing --- Built KHTML with these changes, tested in Konqueror. File Attachments Popup menu before https://git.reviewboard.kde.org/media/uploaded/files/2016/02/11/55c12311-73a3-472a-8a9a-6b6b9fce9de7__before.png Popup menu after https://git.reviewboard.kde.org/media/uploaded/files/2016/02/11/313dbe3b-efb4-475c-bf76-8516f5bc85b4__after.png Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126385: PartManager: stop tracking a widget even if it is no longer a top level window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126385/ --- (Updated Jan. 6, 2016, 6:04 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 5ac5dfb28014035e3704ae8bf8076001e5955ceb by Jonathan Marten to branch master. Bugs: 355711 https://bugs.kde.org/show_bug.cgi?id=355711 Repository: kparts Description --- This appears to be the cause of a crash when exiting System Settings. More information in the bug report, but basically what happens is that the manager keeps track of widgets that it is managing in d->m_managedTopLevelWidgets. If a widget is a top level widget when it is added, but is no longer top level when it is destroyed, it is not removed from the list which results in an access-to-deleted-object in the destructor. This change unconditionally removes the widget even if it is no longer top level. Removing the widget from the list has no ill effects, the list is only actually used in Partmanager::eventFilter() which will never get an event for a deleted widget anyway. Aside: The problematic 'foreach (const QWidget *w, d->m_managedTopLevelWidgets)' loop in PartManager::PartManager() is really superfluous, since all signal connections to 'this' are removed on destruction anyway. Diffs - src/partmanager.cpp 81bf73f Diff: https://git.reviewboard.kde.org/r/126385/diff/ Testing --- Built KParts with this change, and also systemsettings5 with the associated change (see associated review). Observed no crash when exiting the application. Also checked correct operation of Konqueror and Kate. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126423: KPluginSelector::addPlugins: do not assert if 'config' parameter is null
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126423/ --- (Updated Dec. 21, 2015, 8:55 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 1ae59b5f0ea4f50b7350f03fd2b1fe223f66a261 by Jonathan Marten to branch master. Bugs: 352471 https://bugs.kde.org/show_bug.cgi?id=352471 Repository: kcmutils Description --- The API documentation says that the 4-argument form of KPluginSelector::addPlugins() can be called with the last 'config' parameter as null (the default), meaning the standard application config file. However, there seems to be a misplaced assert which means that this will not be accepted. Diffs - src/kpluginselector.cpp 34a7897 Diff: https://git.reviewboard.kde.org/r/126423/diff/ Testing --- Built kcmutils with this change, checked with Konqueror's "Configure Extensions" dialogue as described in the bug report. Observed assert/crash without this change, and correct operation with it. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 126423: KPluginSelector::addPlugins: do not assert if 'config' parameter is null
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126423/ --- Review request for KDE Frameworks. Bugs: 352471 https://bugs.kde.org/show_bug.cgi?id=352471 Repository: kcmutils Description --- The API documentation says that the 4-argument form of KPluginSelector::addPlugins() can be called with the last 'config' parameter as null (the default), meaning the standard application config file. However, there seems to be a misplaced assert which means that this will not be accepted. Diffs - src/kpluginselector.cpp 34a7897 Diff: https://git.reviewboard.kde.org/r/126423/diff/ Testing --- Built kcmutils with this change, checked with Konqueror's "Configure Extensions" dialogue as described in the bug report. Observed assert/crash without this change, and correct operation with it. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 126385: PartManager: stop tracking a widget even if it is no longer a top level window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126385/ --- Review request for KDE Frameworks. Bugs: 355711 https://bugs.kde.org/show_bug.cgi?id=355711 Repository: kparts Description --- This appears to be the cause of a crash when exiting System Settings. More information in the bug report, but basically what happens is that the manager keeps track of widgets that it is managing in d->m_managedTopLevelWidgets. If a widget is a top level widget when it is added, but is no longer top level when it is destroyed, it is not removed from the list which results in an access-to-deleted-object in the destructor. This change unconditionally removes the widget even if it is no longer top level. Removing the widget from the list has no ill effects, the list is only actually used in Partmanager::eventFilter() which will never get an event for a deleted widget anyway. Aside: The problematic 'foreach (const QWidget *w, d->m_managedTopLevelWidgets)' loop in PartManager::PartManager() is really superfluous, since all signal connections to 'this' are removed on destruction anyway. Diffs - src/partmanager.cpp 81bf73f Diff: https://git.reviewboard.kde.org/r/126385/diff/ Testing --- Built KParts with this change, and also systemsettings5 with the associated change (see associated review). Observed no crash when exiting the application. Also checked correct operation of Konqueror and Kate. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 126245: Cookie dialogue: make Accept/Reject buttons work, and other fixes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126245/ --- Review request for KDE Frameworks. Repository: kio Description --- The "You received a cookie from..." dialogue appears, but there are a number of things that do not work as intended. This patch corrects them. 1. The first two buttons were presumably intended to be "Accept" and "Reject" as for KDE4, but they actually said "Reject" and "No". This was a simple cut/paste error. 2. Clicking either of these buttons did nothing. They needed to be connected to accept() and reject() in order to make the exec() called from KCookieServer::checkCookies() return a result. 3. The "Set or modify the cookie information" button text was too wide, making the dialogue width far wider than needed for the cookie information. The dialogue looks better with this changed back to "Details" (with the same icon as for KDE4) with the full text in a tooltip. 4. The state of the hide/show details was not being saved correctly. Using !isHidden() instead of isVisible() gets the correct information. Diffs - src/ioslaves/http/kcookiejar/kcookiewin.cpp 56a283f Diff: https://git.reviewboard.kde.org/r/126245/diff/ Testing --- Built kio with these changes, checked operation of the cookie dialogue in Konqueror. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 125115: kcmshell5 cookies: fix DBus names for kded5
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125115/ --- Review request for KDE Frameworks. Repository: kio Description --- When starting this module, from the command line or similar, the error message is shown: Unable to start the cookie handler service. You will not be able to manage the cookies that are stored on your computer. and similarly for any operation attempted from the resulting dialogue. The reason for this appears to be that the module is connecting to the DBus name 'org.kde.kded', which should now be 'org.kde.kded5' (verified with qdbusviewer and in kded/src/org.kde.kded5.service.in). The patch here corrects these names throughout. Diffs - src/kcms/kio/kcookiesmain.cpp c4e36ac src/kcms/kio/kcookiesmanagement.cpp c041fc2 src/kcms/kio/kcookiespolicies.cpp 7470616 src/kcms/kio/ksaveioconfig.cpp 82ca6d9 Diff: https://git.reviewboard.kde.org/r/125115/diff/ Testing --- Built kio with these changes, cookie dialogue and management operates correctly. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125115: kcmshell5 cookies: fix DBus names for kded5
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125115/ --- (Updated Sept. 9, 2015, 7:55 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 3bdd02a2fbcac6d7bc6a188ce2396160c4f22ed7 by Jonathan Marten to branch master. Repository: kio Description --- When starting this module, from the command line or similar, the error message is shown: Unable to start the cookie handler service. You will not be able to manage the cookies that are stored on your computer. and similarly for any operation attempted from the resulting dialogue. The reason for this appears to be that the module is connecting to the DBus name 'org.kde.kded', which should now be 'org.kde.kded5' (verified with qdbusviewer and in kded/src/org.kde.kded5.service.in). The patch here corrects these names throughout. Diffs - src/kcms/kio/kcookiesmain.cpp c4e36ac src/kcms/kio/kcookiesmanagement.cpp c041fc2 src/kcms/kio/kcookiespolicies.cpp 7470616 src/kcms/kio/ksaveioconfig.cpp 82ca6d9 Diff: https://git.reviewboard.kde.org/r/125115/diff/ Testing --- Built kio with these changes, cookie dialogue and management operates correctly. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
