KMix KDED module
This is for any users of "Classic" KMix - that is, the standalone program that works with ALSA and OSS as well as PulseAudio, not the Plasma 'plasma-pa' panel applet. Along with the main executable it also installs and automatically starts a KDED module 'kmixd' which appears in systemsettings as "KMix Daemon". However, it's not clear exactly what is the purpose of this KDED module. It seems to do exactly the same as KMix does itself, watching for hotplugged sound hardware and updating the internal data structures and saved configuration, but without a GUI. It does expose the available mixers and controls via DBus, but that access is available via the same interface on the KMix application itself. I suspect that this was intended for use by other volume control applications such as KMilo which managed laptop hotkeys back in the KDE3 days, but I don't know of any applications that use it now. The Plasma applet does not use KMix or any of its interfaces - indeed, if plasma-pa is installed it installs an update file which disables KMix autostart. Therefore I'd like to remove the KDED module from KMix. For the moment there is some code (commit 6ede7122 in master branch) that unloads the module and disables its autostart via DBus when the KMix application starts up. If this does not cause any problems with KMix itself or any third party applications then later on the module can be removed completely. I have been running KMix with the module disabled for a long time and have not observed any problems. But, for the moment, please report if the module being disabled causes any problems for you or with any other applications. If needed it can be reenabled on the "Background Services" page in System Settings until KMix is next restarted. Best regards, -- Jonathan Marten http://www.keelhaul.me.uk Twickenham, UK j...@keelhaul.me.uk
KCoreDirLister::setMimeExcludeFilter()
Hello, This has been deprecated in 5.100 with the comment "no known users". Unfortunately there is a use in Kooka, where it is used to configure a KDirOperator (via its KDirLister) to only show files and not directories. This happens at https://invent.kde.org/graphics/kooka/-/blob/master/app/thumbview.cpp#L84 Although KDirOperator has setMode(KFile::Modes), setting this to KFile::File or KFile::Files seems to only affect the selection mode and does not stop directories from being shown. The API doc for KCoreDirLister::setMimeExcludeFilter() says "Filtering should be done with KFileFilter", but from the current KFileFilter doc it appears that this will only be able to be an "include this file or MIME type type" filter, not an "exclude" one. Is there currently - or will there be in the future - a way to configure a KDirOperator in a "files only" mode, in the same way as "directories only" is currently supported? If it turns out that KDirOperator::setMode() is intended to be have that effect, then I'd be happy to investigate why it doesn't. Best regards, -- Jonathan Marten http://www.keelhaul.me.uk Twickenham, UK j...@keelhaul.me.uk
Kuickshow updates
There was some mention of Kuickshow here a couple of months ago. Although there are other image viewer applications available I still find it convenient for quick viewing, just doing that with no distractions. Since there is obviously still some interest I've looked at the code and brought it up to date, in particular: - Works with either Imlib1 (ancient, which some distros don't provide any more) or Imlib2 (the current supported version). The choice is made automatically at build time depending on which library is available. CMake options are available to force either or none. - Can also be built with no Imlib library, using only Qt for image loading and display. This may support newer image formats (e.g. WebP) than Imlib does. Image modifications (brightness, contrast, gamma) are not yet supported without Imlib. - Does not use raw Xlib operations, so hopefully should work on Wayland. X11 is still needed at build time if using Imlib. - A new "Duplicate Window" action. - General code clean up, eliminating warnings and using modern coding standards. Currently these changes are pushed to https://invent.kde.org/graphics/kuickshow in the 'work/marten/modernise' branch. If you use Kuickshow and can compile from source, pull this branch and try it out. Feedback welcome! Best regards, -- Jonathan Marten http://www.keelhaul.me.uk Twickenham, UK j...@keelhaul.me.uk
Re: https://invent.kde.org/graphics/kuickshow and imlib1
Marius P writes: > 1. Can someone build kuickshow? If so, how? > 2. Can we please archive https://invent.kde.org/graphics/kuickshow ? Yes, it builds for me here - again with Gentoo. That distro packages imlib1 separately from imlib2 so they can be coinstalled and appears to have no plans to remove it, although as you say imlib1 has not been updated since 2004. But it still works, even for new image formats(e.g. WebP), and Kuickshow is great as a fast and uncluttered image viewer. Yes, Gwenview has lots more features for browsing and viewing image collections, but sometimes all that is needed is a quick look. So I'd prefer to see Kuickshow still available even if it has to be moved to unmaintained. > In fact kuickshow is a wrapper around imlib1/imlibwidget.cpp. Having looked again at the source it doesn't actually seem to use imlib for much at all - image loading and transformation yes, but it doesn't use the library for displaying the image, only raw X11. (Which means that Kuickshow as it is now will never work on Wayland). So it could be made future proof by either porting to imlib2, which wouldn't involve very much work (only 4 source files actually call imlib), or even eliminating both X11 and imlib and using the corresponding Qt facilities instead (more work, but removes a dependency). Hopefully Kuickshow will live on! -- Jonathan Marten http://www.keelhaul.me.uk Twickenham, UK j...@keelhaul.me.uk
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
Re: Maintainence status of Kooka?
Adriaan de Groot writes: > Kooka had its last release in 2011, as a KDE3 application. However, master is > a KF5-based application, with version number 0.90 in CMakeLists.txt. Is there > any intention to do a release? (That's mostly a question for Jonathan) We > revived it in FreeBSD packaging, calling current master 0.61.296 (from git > describe), but it feels a little weird to ship packages of unreleased > software. >From my point of view (Kooka maintainer, not to be confused with Jonathan Riddell), I'd be happy to make Kooka "released". As far as I am aware it is fully ported to KF5 and has no major outstanding bugs or issues. I'm assuming that this does not mean making it an official part of Applications, although it could adopt their YY.MM version numbering scheme for consitency. It would be better to be included in what used to be called Extragear (now Self Released?). I'll start to work through the review process checklist (https://community.kde.org/ReleasingSoftware), and then try to submit Kooka to kdereview - although that probably won't be able to be until after the summer holidays. Best regards, Jonathan -- Jonathan Marten http://www.keelhaul.me.uk Twickenham, UK j...@keelhaul.me.uk
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 129448: Dolphin Part: Fix problem with Konqueror paste action
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129448/ --- (Updated Dec. 6, 2016, 7:01 a.m.) Status -- This change has been marked as submitted. Review request for Dolphin and KDE Base Apps. Changes --- Submitted with commit 16db90c7938d09bfa8916afef8cce8dd258fa00d by Jonathan Marten to branch master. Bugs: 369523 https://bugs.kde.org/show_bug.cgi?id=369523 Repository: dolphin Description --- When Konqueror is being used as a file manager, the enable state of its "paste" action is not being correctly updated when changing to or showing a new directory. More information is in the bug report, but what seems to be happening is that Dolphin as an application updates the state of its paste action both at the beginning of a directory listing operation and at the end. Only at the latter time is the paste information correctly up to date. When Konqueror uses the Dolphin part, its paste action is only updated at the beginning of a listing, which does not reflect the correct state. This change connects an additional signal within the part so that the paste action is correctly updated at the end of the listing. It does not affect Dolphin the application, because that does not use itself as a part. Diffs - src/dolphinpart.cpp aa9ab298 Diff: https://git.reviewboard.kde.org/r/129448/diff/ Testing --- Built Dolphin and Konqueror with this change, checked correct operation and enable state of the paste action when changing directories and between applications. Also verified by bug submitter. Thanks, Jonathan Marten
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
Re: Review Request 129448: Dolphin Part: Fix problem with Konqueror paste action
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129448/#review101299 --- Ping anyone? - Jonathan Marten On Nov. 20, 2016, 3:23 p.m., Jonathan Marten wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129448/ > --- > > (Updated Nov. 20, 2016, 3:23 p.m.) > > > Review request for Dolphin and KDE Base Apps. > > > Bugs: 369523 > https://bugs.kde.org/show_bug.cgi?id=369523 > > > Repository: dolphin > > > Description > --- > > When Konqueror is being used as a file manager, the enable state of its > "paste" action is not being correctly updated when changing to or showing a > new directory. More information is in the bug report, but what seems to be > happening is that Dolphin as an application updates the state of its paste > action both at the beginning of a directory listing operation and at the end. > Only at the latter time is the paste information correctly up to date. When > Konqueror uses the Dolphin part, its paste action is only updated at the > beginning of a listing, which does not reflect the correct state. > > This change connects an additional signal within the part so that the paste > action is correctly updated at the end of the listing. It does not affect > Dolphin the application, because that does not use itself as a part. > > > Diffs > - > > src/dolphinpart.cpp aa9ab298 > > Diff: https://git.reviewboard.kde.org/r/129448/diff/ > > > Testing > --- > > Built Dolphin and Konqueror with this change, checked correct operation and > enable state of the paste action when changing directories and between > applications. Also verified by bug submitter. > > > 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 129518: Konqueror: restore "Default web browser engine" setting
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129518/ --- (Updated Nov. 22, 2016, 3:20 a.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps and David Faure. Changes --- Submitted with commit 7adf64addec6c9ca62dec8277e164c3ba93f837c by Jonathan Marten to branch master. Repository: konqueror Description --- This settings was originally commented out in the sources, requiring a port to Qt5. There was not actually much porting to be done, apart from setting the correct QStandardPaths location for the mimeapps.list file, but I've also tidied up and removed a lot of the old cruft and unused code. Diffs - settings/konqhtml/generalopts.cpp facec057 Diff: https://git.reviewboard.kde.org/r/129518/diff/ Testing --- Built Konqueror with these changes, checked correct operation of the Settings - Configure Konqueror - General dialogue and the setting of the preferred association for the HTML MIME types. Thanks, Jonathan Marten
Re: Review Request 129518: Konqueror: restore "Default web browser engine" setting
> On Nov. 21, 2016, 12:44 p.m., David Faure wrote: > > settings/konqhtml/generalopts.cpp, line 277 > > <https://git.reviewboard.kde.org/r/129518/diff/1/?file=486354#file486354line277> > > > > That is actually not fully correct anymore, any process making a trader > > query should get updated information automatically, the sycoca rebuilding > > happening in-process if necessary. Can you try removing it? Have tried removing the KBuildSycocaProgressDialog::rebuildKSycoca(this) and it doesn't seem to work. What happens (under a full Plasma 5 desktop) is: Start with engine set to WebKit kcmshell5 khtml_general, change web engine to KHTML then OK => mimeapps.list is updated with khtml.desktop at the front of the list, but there is no kbuildsycoca activity konqueror www.kde.org => still opens in WebKit keditfiletype5 text/html => "Embedding" tab still has WebKit at the top of the list kbuildsycoca5 konqueror www.kde.org => now opens in KHTML keditfiletype5 text/html => "Embedding" tab now has KHTML at the top of the list - Jonathan --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129518/#review101013 --- On Nov. 21, 2016, 12:25 p.m., Jonathan Marten wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129518/ > --- > > (Updated Nov. 21, 2016, 12:25 p.m.) > > > Review request for KDE Base Apps and David Faure. > > > Repository: konqueror > > > Description > --- > > This settings was originally commented out in the sources, requiring a port > to Qt5. There was not actually much porting to be done, apart from setting > the correct QStandardPaths location for the mimeapps.list file, but I've also > tidied up and removed a lot of the old cruft and unused code. > > > Diffs > - > > settings/konqhtml/generalopts.cpp facec057 > > Diff: https://git.reviewboard.kde.org/r/129518/diff/ > > > Testing > --- > > Built Konqueror with these changes, checked correct operation of the Settings > - Configure Konqueror - General dialogue and the setting of the preferred > association for the HTML MIME types. > > > Thanks, > > Jonathan Marten > >
Review Request 129518: Konqueror: restore "Default web browser engine" setting
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129518/ --- Review request for KDE Base Apps and David Faure. Repository: konqueror Description --- This settings was originally commented out in the sources, requiring a port to Qt5. There was not actually much porting to be done, apart from setting the correct QStandardPaths location for the mimeapps.list file, but I've also tidied up and removed a lot of the old cruft and unused code. Diffs - settings/konqhtml/generalopts.cpp facec057 Diff: https://git.reviewboard.kde.org/r/129518/diff/ Testing --- Built Konqueror with these changes, checked correct operation of the Settings - Configure Konqueror - General dialogue and the setting of the preferred association for the HTML MIME types. Thanks, Jonathan Marten
Review Request 129448: Dolphin Part: Fix problem with Konqueror paste action
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129448/ --- Review request for Dolphin and KDE Base Apps. Bugs: 369523 https://bugs.kde.org/show_bug.cgi?id=369523 Repository: dolphin Description --- When Konqueror is being used as a file manager, the enable state of its "paste" action is not being correctly updated when changing to or showing a new directory. More information is in the bug report, but what seems to be happening is that Dolphin as an application updates the state of its paste action both at the beginning of a directory listing operation and at the end. Only at the latter time is the paste information correctly up to date. When Konqueror uses the Dolphin part, its paste action is only updated at the beginning of a listing, which does not reflect the correct state. This change connects an additional signal within the part so that the paste action is correctly updated at the end of the listing. It does not affect Dolphin the application, because that does not use itself as a part. Diffs - src/dolphinpart.cpp aa9ab298 Diff: https://git.reviewboard.kde.org/r/129448/diff/ Testing --- Built Dolphin and Konqueror with this change, checked correct operation and enable state of the paste action when changing directories and between applications. Also verified by bug submitter. Thanks, Jonathan Marten
Re: Review Request 129423: keditbookmarks: add standard icons and shortcuts to Undo/Redo actions
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129423/ --- (Updated Nov. 18, 2016, 1:55 p.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps. Changes --- Submitted with commit aa247198f16179cf86768cfdeed94e7c557e864c by Jonathan Marten to branch master. Repository: keditbookmarks Description --- These actions are created by QUndoStack in order that it can manage the action text. However, this means that they do not get the standard KDE action icon or shortcuts set. This change sets those by reference to the appropriate KStandardAction. Diffs - src/kbookmarkmodel/commandhistory.cpp 53a8931 Diff: https://git.reviewboard.kde.org/r/129423/diff/ Testing --- Built keditbookmarks (split repository master version) with this change, observed correct appearance and operation of Undo/Redo actions. Thanks, Jonathan Marten
Review Request 129423: keditbookmarks: add standard icons and shortcuts to Undo/Redo actions
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129423/ --- Review request for KDE Base Apps. Repository: keditbookmarks Description --- These actions are created by QUndoStack in order that it can manage the action text. However, this means that they do not get the standard KDE action icon or shortcuts set. This change sets those by reference to the appropriate KStandardAction. Diffs - src/kbookmarkmodel/commandhistory.cpp 53a8931 Diff: https://git.reviewboard.kde.org/r/129423/diff/ Testing --- Built keditbookmarks (split repository master version) with this change, observed correct appearance and operation of Undo/Redo actions. 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 127944: KDE Platform Theme: set file dialog overwrite option appropriately for saving
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/
---
(Updated May 20, 2016, 9:22 a.m.)
Review request for kde-workspace and Plasma.
Changes
---
Use a temporary file for the overwrite check, instead of assuming a name that
should exist.
Bugs: 360666
https://bugs.kde.org/show_bug.cgi?id=360666
Repository: plasma-integration
Description
---
The referenced bug says that, by default, there is no file overwrite check when
using QFileDialog to save a file. Indeed, on closer investigating it appears
that there is no way to even explictly force an overwrite check when using the
KDE platform theme, because of this code in
plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
// overwrite option
if
(options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite))
{
dialog->m_fileWidget->setConfirmOverwrite(false);
}
The default for KFileWidget is already for no overwrite check (as set in
kio/src/filewidgets/kfilewidget.cpp which initialises
KFileWidgetPrivate::confirmOverwrite to false). There is no way to override
this from the calling application through the platform plugin.
Suggest that the default option should be the same as that defined by Qt for
QFileDialog: always perform an ovewrwrite check on saving, unless the caller
has set the QFileDialog::DontConfirmOverwrite option. This is also a sensible
default to have from the user's point of view. This change does that in the
platform theme plugin, for all saving operations.
Diffs (updated)
-
autotests/kfiledialog_unittest.cpp 59915da
src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
tests/qfiledialogtest.cpp 1d69ea1
Diff: https://git.reviewboard.kde.org/r/127944/diff/
Testing
---
Built plasma-intergration with this change, confirmed correct operation of file
dialogues and that confirmation is requested when overwriting an existing file,
unless the QFileDialog::DontConfirmOverwrite option is specified.
Thanks,
Jonathan Marten
Re: Review Request 127944: KDE Platform Theme: set file dialog overwrite option appropriately for saving
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/
---
(Updated May 18, 2016, 11:07 a.m.)
Review request for kde-workspace and Plasma.
Bugs: 360666
https://bugs.kde.org/show_bug.cgi?id=360666
Repository: plasma-integration
Description
---
The referenced bug says that, by default, there is no file overwrite check when
using QFileDialog to save a file. Indeed, on closer investigating it appears
that there is no way to even explictly force an overwrite check when using the
KDE platform theme, because of this code in
plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
// overwrite option
if
(options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite))
{
dialog->m_fileWidget->setConfirmOverwrite(false);
}
The default for KFileWidget is already for no overwrite check (as set in
kio/src/filewidgets/kfilewidget.cpp which initialises
KFileWidgetPrivate::confirmOverwrite to false). There is no way to override
this from the calling application through the platform plugin.
Suggest that the default option should be the same as that defined by Qt for
QFileDialog: always perform an ovewrwrite check on saving, unless the caller
has set the QFileDialog::DontConfirmOverwrite option. This is also a sensible
default to have from the user's point of view. This change does that in the
platform theme plugin, for all saving operations.
Diffs (updated)
-
autotests/kfiledialog_unittest.cpp 59915da
src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
tests/qfiledialogtest.cpp 1d69ea1
Diff: https://git.reviewboard.kde.org/r/127944/diff/
Testing
---
Built plasma-intergration with this change, confirmed correct operation of file
dialogues and that confirmation is requested when overwriting an existing file,
unless the QFileDialog::DontConfirmOverwrite option is specified.
Thanks,
Jonathan Marten
Re: Review Request 127944: KDE Platform Theme: set file dialog overwrite option appropriately for saving
> On May 17, 2016, 3:29 p.m., Martin Gräßlin wrote:
> > The test does not verify the problem. I just pulled the patch, undid your
> > change, but the test passed nevertheless.
>
> Martin Gräßlin wrote:
> ah now I see. You adjusted the test application, but not the autotest.
>
> Jonathan Marten wrote:
> Now I see that there are autotests after all. How do I run them?
>
> Martin Gräßlin wrote:
> you can go to the build directory and just do:
> make test
>
> or just run the individial test binary created in build/autotests. The
> relevant one would be kfiledialog_unittest and kfiledialogqml_unittest
>
> Jonathan Marten wrote:
> Ok, found them and how - thanks.
>
> It doesn't appear to be possible to just check that the QFileDialog
> option is passed correctly through to the file widget, because KFileWidget
> has no way to read back the option set by setConfirmOverwrite. So it won't
> be a simple test like the setFileMode tests - it will have to look for the
> message box being shown. Is that worth doing (and reliable enough)?
>
> Martin Gräßlin wrote:
> hmm yeah, tricky. I think it would be ok to check for the messagebox
> being shown as that's kind of also how the autotest for the dialog works in
> general. But I also don't have an idea on how to check whether the messagebox
> got opened. So maybe just push without it.
>
> Btw. please push for the Plasma/5.6 branch.
Ok, have worked out an autotest. It's not pretty, but it does work - passing
with the modified platform theme and failing with the original. It puts up a
save box for a file which already exists, clicks the OK button and checks that
a message box appears or not according to the QFileDialog option. The message
box has to be checked in a QTimer slot because, if it does appear, it runs its
own event loop which halts the test until the user clicks one of its buttons.
Go easy with it - it's my first ever Frameworks autotest :-)
- Jonathan
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/#review95535
---
On May 18, 2016, 11:07 a.m., Jonathan Marten wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127944/
> ---
>
> (Updated May 18, 2016, 11:07 a.m.)
>
>
> Review request for kde-workspace and Plasma.
>
>
> Bugs: 360666
> https://bugs.kde.org/show_bug.cgi?id=360666
>
>
> Repository: plasma-integration
>
>
> Description
> ---
>
> The referenced bug says that, by default, there is no file overwrite check
> when using QFileDialog to save a file. Indeed, on closer investigating it
> appears that there is no way to even explictly force an overwrite check when
> using the KDE platform theme, because of this code in
> plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
>
> // overwrite option
> if
> (options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite))
> {
> dialog->m_fileWidget->setConfirmOverwrite(false);
> }
>
> The default for KFileWidget is already for no overwrite check (as set in
> kio/src/filewidgets/kfilewidget.cpp which initialises
> KFileWidgetPrivate::confirmOverwrite to false). There is no way to override
> this from the calling application through the platform plugin.
>
> Suggest that the default option should be the same as that defined by Qt for
> QFileDialog: always perform an ovewrwrite check on saving, unless the caller
> has set the QFileDialog::DontConfirmOverwrite option. This is also a
> sensible default to have from the user's point of view. This change does
> that in the platform theme plugin, for all saving operations.
>
>
> Diffs
> -
>
> autotests/kfiledialog_unittest.cpp 59915da
> src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
> tests/qfiledialogtest.cpp 1d69ea1
>
> Diff: https://git.reviewboard.kde.org/r/127944/diff/
>
>
> Testing
> ---
>
> Built plasma-intergration with this change, confirmed correct operation of
> file dialogues and that confirmation is requested when overwriting an
> existing file, unless the QFileDialog::DontConfirmOverwrite option is
> specified.
>
>
> Thanks,
>
> Jonathan Marten
>
>
Re: Review Request 127944: KDE Platform Theme: set file dialog overwrite option appropriately for saving
> On May 17, 2016, 3:29 p.m., Martin Gräßlin wrote:
> > The test does not verify the problem. I just pulled the patch, undid your
> > change, but the test passed nevertheless.
>
> Martin Gräßlin wrote:
> ah now I see. You adjusted the test application, but not the autotest.
Now I see that there are autotests after all. How do I run them?
- Jonathan
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/#review95535
---
On May 17, 2016, 3:20 p.m., Jonathan Marten wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127944/
> ---
>
> (Updated May 17, 2016, 3:20 p.m.)
>
>
> Review request for kde-workspace and Plasma.
>
>
> Bugs: 360666
> https://bugs.kde.org/show_bug.cgi?id=360666
>
>
> Repository: plasma-integration
>
>
> Description
> ---
>
> The referenced bug says that, by default, there is no file overwrite check
> when using QFileDialog to save a file. Indeed, on closer investigating it
> appears that there is no way to even explictly force an overwrite check when
> using the KDE platform theme, because of this code in
> plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
>
> // overwrite option
> if
> (options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite))
> {
> dialog->m_fileWidget->setConfirmOverwrite(false);
> }
>
> The default for KFileWidget is already for no overwrite check (as set in
> kio/src/filewidgets/kfilewidget.cpp which initialises
> KFileWidgetPrivate::confirmOverwrite to false). There is no way to override
> this from the calling application through the platform plugin.
>
> Suggest that the default option should be the same as that defined by Qt for
> QFileDialog: always perform an ovewrwrite check on saving, unless the caller
> has set the QFileDialog::DontConfirmOverwrite option. This is also a
> sensible default to have from the user's point of view. This change does
> that in the platform theme plugin, for all saving operations.
>
>
> Diffs
> -
>
> src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
> tests/qfiledialogtest.cpp 1d69ea1
>
> Diff: https://git.reviewboard.kde.org/r/127944/diff/
>
>
> Testing
> ---
>
> Built plasma-intergration with this change, confirmed correct operation of
> file dialogues and that confirmation is requested when overwriting an
> existing file, unless the QFileDialog::DontConfirmOverwrite option is
> specified.
>
>
> Thanks,
>
> Jonathan Marten
>
>
Re: Review Request 127944: KDE Platform Theme: set file dialog overwrite option appropriately for saving
> On May 17, 2016, 2:35 p.m., Martin Gräßlin wrote:
> > The change looks sensible to me, but I would appreciate a test case for it.
> > There are already some tests for the file dialog, so it should be easy to
> > extend.
There is no autotest, but have updated tests/qfiledialogtest with options
'--confirmOverwrite' (on or off) and '--nativeDialog' (on or off). Checked
operation of these options and overwrite warning.
- Jonathan
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/#review95532
---
On May 17, 2016, 3:20 p.m., Jonathan Marten wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127944/
> ---
>
> (Updated May 17, 2016, 3:20 p.m.)
>
>
> Review request for kde-workspace and Plasma.
>
>
> Bugs: 360666
> https://bugs.kde.org/show_bug.cgi?id=360666
>
>
> Repository: plasma-integration
>
>
> Description
> ---
>
> The referenced bug says that, by default, there is no file overwrite check
> when using QFileDialog to save a file. Indeed, on closer investigating it
> appears that there is no way to even explictly force an overwrite check when
> using the KDE platform theme, because of this code in
> plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
>
> // overwrite option
> if
> (options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite))
> {
> dialog->m_fileWidget->setConfirmOverwrite(false);
> }
>
> The default for KFileWidget is already for no overwrite check (as set in
> kio/src/filewidgets/kfilewidget.cpp which initialises
> KFileWidgetPrivate::confirmOverwrite to false). There is no way to override
> this from the calling application through the platform plugin.
>
> Suggest that the default option should be the same as that defined by Qt for
> QFileDialog: always perform an ovewrwrite check on saving, unless the caller
> has set the QFileDialog::DontConfirmOverwrite option. This is also a
> sensible default to have from the user's point of view. This change does
> that in the platform theme plugin, for all saving operations.
>
>
> Diffs
> -
>
> src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
> tests/qfiledialogtest.cpp 1d69ea1
>
> Diff: https://git.reviewboard.kde.org/r/127944/diff/
>
>
> Testing
> ---
>
> Built plasma-intergration with this change, confirmed correct operation of
> file dialogues and that confirmation is requested when overwriting an
> existing file, unless the QFileDialog::DontConfirmOverwrite option is
> specified.
>
>
> Thanks,
>
> Jonathan Marten
>
>
Re: Review Request 127944: KDE Platform Theme: set file dialog overwrite option appropriately for saving
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/
---
(Updated May 17, 2016, 3:20 p.m.)
Review request for kde-workspace and Plasma.
Changes
---
Added test
Bugs: 360666
https://bugs.kde.org/show_bug.cgi?id=360666
Repository: plasma-integration
Description
---
The referenced bug says that, by default, there is no file overwrite check when
using QFileDialog to save a file. Indeed, on closer investigating it appears
that there is no way to even explictly force an overwrite check when using the
KDE platform theme, because of this code in
plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
// overwrite option
if
(options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite))
{
dialog->m_fileWidget->setConfirmOverwrite(false);
}
The default for KFileWidget is already for no overwrite check (as set in
kio/src/filewidgets/kfilewidget.cpp which initialises
KFileWidgetPrivate::confirmOverwrite to false). There is no way to override
this from the calling application through the platform plugin.
Suggest that the default option should be the same as that defined by Qt for
QFileDialog: always perform an ovewrwrite check on saving, unless the caller
has set the QFileDialog::DontConfirmOverwrite option. This is also a sensible
default to have from the user's point of view. This change does that in the
platform theme plugin, for all saving operations.
Diffs (updated)
-
src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
tests/qfiledialogtest.cpp 1d69ea1
Diff: https://git.reviewboard.kde.org/r/127944/diff/
Testing
---
Built plasma-intergration with this change, confirmed correct operation of file
dialogues and that confirmation is requested when overwriting an existing file,
unless the QFileDialog::DontConfirmOverwrite option is specified.
Thanks,
Jonathan Marten
Review Request 127944: KDE Platform Theme: set file dialog overwrite option appropriately for saving
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/
---
Review request for kde-workspace and Plasma.
Bugs: 360666
https://bugs.kde.org/show_bug.cgi?id=360666
Repository: plasma-integration
Description
---
The referenced bug says that, by default, there is no file overwrite check when
using QFileDialog to save a file. Indeed, on closer investigating it appears
that there is no way to even explictly force an overwrite check when using the
KDE platform theme, because of this code in
plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
// overwrite option
if
(options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite))
{
dialog->m_fileWidget->setConfirmOverwrite(false);
}
The default for KFileWidget is already for no overwrite check (as set in
kio/src/filewidgets/kfilewidget.cpp which initialises
KFileWidgetPrivate::confirmOverwrite to false). There is no way to override
this from the calling application through the platform plugin.
Suggest that the default option should be the same as that defined by Qt for
QFileDialog: always perform an ovewrwrite check on saving, unless the caller
has set the QFileDialog::DontConfirmOverwrite option. This is also a sensible
default to have from the user's point of view. This change does that in the
platform theme plugin, for all saving operations.
Diffs
-
src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
Diff: https://git.reviewboard.kde.org/r/127944/diff/
Testing
---
Built plasma-intergration with this change, confirmed correct operation of file
dialogues and that confirmation is requested when overwriting an existing file,
unless the QFileDialog::DontConfirmOverwrite option is specified.
Thanks,
Jonathan Marten
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 127138: Konqueror: Fix window title showing as "/" when URL ends with that
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127138/ --- (Updated Feb. 23, 2016, 11:46 a.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps and David Faure. Changes --- Submitted with commit f868ba3b34b4f55111c3b6fb1ff0e6bcaa1d2797 by Jonathan Marten to branch frameworks. Repository: kde-baseapps Description --- This happens when a local file URL which ends in "/" is navigated to. For example, going to "/home/user" correctly shows "user" as the window title, while going to "/home/user/" shows "/" because the fileName() of that URL is empty. This happens in particular when using the "Up" action, as it goes from "/home/user/foo" to "/home/user/". This change ensures that trailing slashes are removed from URLs before using fileName() on them. Diffs - konqueror/src/konqview.cpp 3707c7a Diff: https://git.reviewboard.kde.org/r/127138/diff/ Testing --- Built kde-baseapps with these changes, observed correct display of base name in Konqueror window title while navigating. Thanks, Jonathan Marten
Re: Review Request 127138: Konqueror: Fix window title showing as "/" when URL ends with that
> On Feb. 23, 2016, 7:49 a.m., David Faure wrote:
> > konqueror/src/konqview.cpp, line 692
> > <https://git.reviewboard.kde.org/r/127138/diff/1/?file=445029#file445029line692>
> >
> > Your code is correct in that it's equivalent to the old code. However I
> > wonder if this couldn't be simplified.
> >
> > If we want to display the filename of this->url(), unless "caption" was
> > set to something completely different by the part, we could just compare
> > path() instead of fileName()...
> >
> > const QUrl captionUrl(QUrl::fromUserInput(caption));
> > if (captionUrl.isValid() && captionUrl.isLocalFile() &&
> > captionUrl.path() == url().path()) {
> > adjustedCaption =
> > captionUrl.adjusted(QUrl::StripTrailingSlash).fileName();
> > // if empty...
> > }
> >
> > This looks a bit simpler, I think? If you agree, can you test it, and
> > push it if it works?
Tested and working, thanks for the suggestion!
- Jonathan
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127138/#review92652
---
On Feb. 22, 2016, 3:12 p.m., Jonathan Marten wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127138/
> ---
>
> (Updated Feb. 22, 2016, 3:12 p.m.)
>
>
> Review request for KDE Base Apps and David Faure.
>
>
> Repository: kde-baseapps
>
>
> Description
> ---
>
> This happens when a local file URL which ends in "/" is navigated to. For
> example, going to "/home/user" correctly shows "user" as the window title,
> while going to "/home/user/" shows "/" because the fileName() of that URL is
> empty. This happens in particular when using the "Up" action, as it goes
> from "/home/user/foo" to "/home/user/".
>
> This change ensures that trailing slashes are removed from URLs before using
> fileName() on them.
>
>
> Diffs
> -
>
> konqueror/src/konqview.cpp 3707c7a
>
> Diff: https://git.reviewboard.kde.org/r/127138/diff/
>
>
> Testing
> ---
>
> Built kde-baseapps with these changes, observed correct display of base name
> in Konqueror window title while navigating.
>
>
> Thanks,
>
> Jonathan Marten
>
>
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 127138: Konqueror: Fix window title showing as "/" when URL ends with that
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127138/ --- Review request for KDE Base Apps and David Faure. Repository: kde-baseapps Description --- This happens when a local file URL which ends in "/" is navigated to. For example, going to "/home/user" correctly shows "user" as the window title, while going to "/home/user/" shows "/" because the fileName() of that URL is empty. This happens in particular when using the "Up" action, as it goes from "/home/user/foo" to "/home/user/". This change ensures that trailing slashes are removed from URLs before using fileName() on them. Diffs - konqueror/src/konqview.cpp 3707c7a Diff: https://git.reviewboard.kde.org/r/127138/diff/ Testing --- Built kde-baseapps with these changes, observed correct display of base name in Konqueror window title while navigating. Thanks, Jonathan Marten
Re: Review Request 127006: konqueror: eliminate signal and shortcut warnings
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127006/ --- (Updated Feb. 22, 2016, 2:46 p.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps. Changes --- Submitted with commit fc5e0160d81c4fad2e54e2cf31e09395634d7783 by Jonathan Marten to branch frameworks. Repository: kde-baseapps Description --- These changes to Konqueror's setting up of actions eliminate the runtime debug messages: konqueror(3420)/default err_method_notfound: QObject::connect: No such signal QAction::triggered(Qt::MouseButtons,Qt::KeyboardModifiers) in /ws/frameworks/applications/kdebaseapps/konqueror/src/konqmainwindow.cpp:3637 and one for each action: konqueror(3420)/default KXMLGUIFactoryPrivate::saveDefaultActionProperties: Shortcut for action "new_window" "New " set with QAction::setShortcut()! Use KActionCollection::setDefaultShortcut(s) instead. and also a number of compiler warnings: konqueror/src/konqmainwindow.cpp:3705: warning: ‘KShortcut’ is deprecated Diffs - konqueror/src/konqmainwindow.cpp c7a81c8 Diff: https://git.reviewboard.kde.org/r/127006/diff/ Testing --- Built Konqueror with these changes. Observed absence of those compiler and runtime error messages, checked correct operation of key shortcuts. Thanks, Jonathan Marten
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
Review Request 127006: konqueror: eliminate signal and shortcut warnings
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127006/ --- Review request for KDE Base Apps. Repository: kde-baseapps Description --- These changes to Konqueror's setting up of actions eliminate the runtime debug messages: konqueror(3420)/default err_method_notfound: QObject::connect: No such signal QAction::triggered(Qt::MouseButtons,Qt::KeyboardModifiers) in /ws/frameworks/applications/kdebaseapps/konqueror/src/konqmainwindow.cpp:3637 and one for each action: konqueror(3420)/default KXMLGUIFactoryPrivate::saveDefaultActionProperties: Shortcut for action "new_window" "New " set with QAction::setShortcut()! Use KActionCollection::setDefaultShortcut(s) instead. and also a number of compiler warnings: konqueror/src/konqmainwindow.cpp:3705: warning: ‘KShortcut’ is deprecated Diffs - konqueror/src/konqmainwindow.cpp c7a81c8 Diff: https://git.reviewboard.kde.org/r/127006/diff/ Testing --- Built Konqueror with these changes. Observed absence of those compiler and runtime error messages, checked correct operation of key shortcuts. Thanks, Jonathan Marten
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
Re: Review Request 115183: KRunner: reset history when showing (to be consistent with shells)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115183/ --- (Updated Feb. 15, 2014, 9:21 a.m.) Status -- This change has been marked as submitted. Review request for kde-workspace. Bugs: 314690 http://bugs.kde.org/show_bug.cgi?id=314690 Repository: kde-workspace Description --- The referenced bug describes a recent change in the KRunner command history which makes recalling previous commands confusing, and different to the way that history recall works in Bash and other shells. I don't know whether this change was deliberate or accidental, but in the interests of consistency this patch ensures that the command history is reset to the beginning when showing the runner. Diffs - krunner/interfaces/default/interface.cpp 505e0aa Diff: https://git.reviewboard.kde.org/r/115183/diff/ Testing --- Built kde-workspace with this change, verified operation of runner history. Also verified by another bug reporter. Thanks, Jonathan Marten
Review Request 115183: KRunner: reset history when showing (to be consistent with shells)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115183/ --- Review request for kde-workspace. Bugs: 314690 http://bugs.kde.org/show_bug.cgi?id=314690 Repository: kde-workspace Description --- The referenced bug describes a recent change in the KRunner command history which makes recalling previous commands confusing, and different to the way that history recall works in Bash and other shells. I don't know whether this change was deliberate or accidental, but in the interests of consistency this patch ensures that the command history is reset to the beginning when showing the runner. Diffs - krunner/interfaces/default/interface.cpp 505e0aa Diff: https://git.reviewboard.kde.org/r/115183/diff/ Testing --- Built kde-workspace with this change, verified operation of runner history. Also verified by another bug reporter. Thanks, Jonathan Marten
Re: Moving Baloo forward
Vishesh Handa m...@vhanda.in writes: http://community.kde.org/Baloo Thanks for producing that useful page, and of course for all your past and current work on Nepomuk and Baloo. There is one statement there that makes me a little concerned: This metadata is now stored with the extended attributes of the file instead of storing it in a separate database. Am I right in assuming that this means xattr? If so, would there be implications for cases where those extended attributes may not get preserved or are not supported: - simple copy/move of a file using cp/mv (with the default options) - copy/move of a file using KIO - network file systems (NFS or SMB) - backup/restore - FAT filesystems - platforms other then Linux Hoping that none of those will make normal use impossible, but if there would be problems or workarounds needed with any of these then they ought to be addressed early. Regards, Jonathan -- Jonathan Marten http://www.keelhaul.demon.co.uk Twickenham, UK j...@keelhaul.demon.co.uk
Review Request 114924: Konqueror: fix crash wnen switching between view modes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114924/ --- Review request for KDE Base Apps and Dawit Alemayehu. Bugs: 322686 http://bugs.kde.org/show_bug.cgi?id=322686 Repository: kde-baseapps Description --- The referenced bug describes an assert crash which happens when switching view modes in Konqueror, using the View - View Mode menu. It does not happen when switching view modes via the Dolphin Part toolbar. Although the problem does not happen for adawit, this changes fixes the crash for me. Diffs - konqueror/src/konqmainwindow.cpp 8a21c1b Diff: https://git.reviewboard.kde.org/r/114924/diff/ Testing --- Built kde-baseapps with these changes. Checked no crash when switching view mode both by toolbar and menu, and that the correct items in each case are checked. Thanks, Jonathan Marten
