kdepimlibs Coverity Scan Report, Oct 14 2014
Howdy, Attached is the Coverity Scan report for kdepimlibs 4.14 as of today. You might feel like fixing some of the issues. Let me know if you find false positives or stuff we can ignore (like in test programs). CID Type Impact Category File Function 1245732 Uninitialized scalar variable High Uninitialized variables /data/mykde/src/KDE/kdepimlibs/kalarmcal/kaevent.cpp readAlarms 1167377 Non-virtual destructor High Resource leaks /data/mykde/src/KDE/kdepimlibs/kldap/ldapurl.cpp ~LdapUrl 1167326 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kalarmcal/kacalendar.cpp operator - 1167325 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/attributefactory.cpp operator - 1167324 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/typepluginloader.cpp operator - 1167323 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kabc/address.cpp operator - 1167322 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kabc/picture.cpp s_sharedEmpty 1167321 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kalarmcal/kaevent.cpp operator - 1167320 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kmime/tests/auto/headertest.cpp testContentTypeHeader 1167319 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kimap/acl.cpp operator - 1167318 Out-of-bounds write High Memory - corruptions /data/mykde/src/KDE/kdepimlibs/kmime/kmime_codecs.h write 1165782 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/erroroverlay.cpp operator - 258582 Uninitialized scalar variable High Uninitialized variables /data/mykde/src/KDE/kdepimlibs/kxmlrpcclient/query.cpp parseMessageResponse 258579 Uninitialized scalar variable High Uninitialized variables /data/mykde/src/KDE/kdepimlibs/akonadi/tests/entitytreemodeltest.cpp getExpectedSignal 258028 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/mailtransport/tests/attributetest.cpp testRegistrar 258026 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kmime/tests/auto/headertest.cpp testIdentHeader 258024 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kcalcore/tests/testalarm.cpp testAssignment 258023 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kpimidentities/signature.cpp d_func 258021 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/tests/testrunner/config.cpp globalConfig 258018 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/tests/collectionattributetest.cpp testDefaultAttributes 258017 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/tests/attributefactorytest.cpp testRegisteredAttribute 258016 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/tests/attributefactorytest.cpp testUnknownAttribute 258014 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/item.cpp dummyPayload 258012 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/contact/tests/contactmetadataattributetest.cpp clone 257959 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/item.cpp typeInfoToMetaTypeIdMap 257387 Resource leak in object High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/entitylistview.cpp Private 257385 Resource leak in object High Resource leaks /data/mykde/src/KDE/kdepimlibs/akonadi/entitytreeview.cpp Private 256376 Buffer not null terminated High Memory - illegal accesses /data/mykde/src/KDE/kdepimlibs/kcalcore/versit/vobject.c writeGroup 254961 Resource leak High Resource leaks /data/mykde/src/KDE/kdepimlibs/kcalcore/tests/testcalfilter.cpp testValidity 253999 Use after free High Memory - illegal accesses /data/mykde/src/KDE/kdepimlibs/kcal/calendar.cpp removeRelations 253998 Read from pointer after free High Memory - illegal accesses /data/mykde/src/KDE/kdepimlibs/kcal/incidence.cpp ~Incidence 253997 Read from pointer after free High Memory - illegal accesses /data/mykde/src/KDE/kdepimlibs/kcal/incidence.cpp setRelatedTo 1245734 Uninitialized scalar field Medium Uninitialized members /data/mykde/src/KDE/kdepimlibs/akonadi/tests/etmpopulationtest.cpp ModelSignalSpy 1245733 Uninitialized pointer field Medium Uninitialized members /data/mykde/src/KDE/kdepimlibs/akonadi/itemsync.cpp ItemSync 1193546 Uninitialized pointer field Medium Uninitialized members /data/mykde/src/KDE/kdepimlibs/akonadi/tageditwidget.cpp Private 1193544 Uninitialized pointer field Medium Uninitialized members /data/mykde/src/KDE/kdepimlibs/kcalcore/compat.h Compat32PrereleaseVersions 1193543 Uninitialized pointer field Medium Uninitialized members /data/mykde/src/KDE/kdepimlibs/kcalcore/compat.h CompatOutlook9 1193542 Uninitialized pointer field Medium Uninitialized members /data/mykde/src/KDE/kdepimlibs/kcalcore/compat.h CompatPre31 1193541 Uninitialized pointer field Medium Uninitialized members
Re: [Kde-pim] [UPDATE] kdepimlibs Coverity Scan Report, Oct 14 2014
On Tuesday 14 October 2014 17.53:06 Allen Winter wrote: Attached ? -- Georg C. F. Greve Chief Executive Officer Kolab Systems AGMake it your Kolab @ http://mykolab.com Zürich, Switzerland Swiss Secure Collaboration as a Service e: gr...@kolabsys.com t: +41 78 904 43 33 w: http://kolabsys.com pgp: 86574ACA Georg C. F. Greve
Fwd: kdepimlibs Coverity Scan Report, Oct 14 2014
Allen, Just a workflow question : why to export Coverity report to CSV where you can send automatically a mail to devel mailing list when scan is complete, with a a list of new defect found in code. I use Coverity since more than one year with whole digiKam code, and we have already fixed more than 500 entries detected. The Coverity web interface is really more suitable than a export to CSV. Defect are very well explained in source context, with all conditions used to check implementation. The only constrain is to have an account for each contributors who will fixed entries. After one year to use this tool, the feedback is really excellent, especially to check new code from students. Best Gilles Caulier 2014-10-14 14:30 GMT+02:00 Allen Winter allen.d.win...@gmail.com: Howdy, Attached is the Coverity Scan report for kdepimlibs 4.14 as of today. You might feel like fixing some of the issues. Let me know if you find false positives or stuff we can ignore (like in test programs).
Re: kdepimlibs Coverity Scan Report, Oct 14 2014
On Thu, Oct 16, 2014 at 8:53 PM, Gilles Caulier caulier.gil...@gmail.com wrote: Allen, Hi Gilles, Just a workflow question : why to export Coverity report to CSV where you can send automatically a mail to devel mailing list when scan is complete, with a a list of new defect found in code. I use Coverity since more than one year with whole digiKam code, and we have already fixed more than 500 entries detected. The Coverity web interface is really more suitable than a export to CSV. Defect are very well explained in source context, with all conditions used to check implementation. The only constrain is to have an account for each contributors who will fixed entries. I suspect that is why Allen is sending out the HTML/CSV output - because not everyone has access and it is helpful to have this information publicly accessible. After one year to use this tool, the feedback is really excellent, especially to check new code from students. Best Gilles Caulier Thanks, Ben 2014-10-14 14:30 GMT+02:00 Allen Winter allen.d.win...@gmail.com: Howdy, Attached is the Coverity Scan report for kdepimlibs 4.14 as of today. You might feel like fixing some of the issues. Let me know if you find false positives or stuff we can ignore (like in test programs).
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120202/ --- (Updated Oct. 16, 2014, 1:26 p.m.) Review request for KDE Software on Mac OS X and kdelibs. Changes --- Corrects a regression introduced in yesterday's update (I really should get more sleep) Repository: kdelibs Description --- I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed. I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus. - idle time closing of wallets. This feature was not supported in the commited version presented in https://git.reviewboard.kde.org/r/119838/ The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is = the timeout, the wallet is closed. This applies only to KDE keychains, so keychains used by OS X applications should not be affected. - close when last application exits. This requires maintaining a user list which keeps track of what application has what wallet open. I've implemented an internal version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it. So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome. Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work. Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose. Diffs (updated) - kdeui/util/kwallet.h d7f703f kdeui/util/kwallet_mac.h PRE-CREATION kdeui/util/kwallet_mac.cpp 8344ebb kdeui/util/qosxkeychain.h d0934e6 kdeui/util/qosxkeychain.cpp 7cb9a22 Diff: https://git.reviewboard.kde.org/r/120202/diff/ Testing --- OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 . Once finalised, all changes should port easily to KF5's kwallet_mac.cpp . Thanks, René J.V. Bertin
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
On Oct. 15, 2014, 10:07 p.m., Thomas Lübking wrote: Please check https://techbase.kde.org/Policies/Kdelibs_Coding_Style Sure, and guess what I noticed first ;) ``` Nobody is forced to use this style, but to have consistent formatting of the source code files it is recommended to make use of it. ``` I won't mix and match styles in existing code, trying instead to match the style used locally as closely as possible. In files I create from scratch I'd prefer to stick to my own convictions, though - basically the only difference is the absence of a space after a keyword. It's not like that isn't exactly unprecedented ... - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120202/#review68511 --- On Oct. 16, 2014, 1:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120202/ --- (Updated Oct. 16, 2014, 1:26 p.m.) Review request for KDE Software on Mac OS X and kdelibs. Repository: kdelibs Description --- I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed. I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus. - idle time closing of wallets. This feature was not supported in the commited version presented in https://git.reviewboard.kde.org/r/119838/ The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is = the timeout, the wallet is closed. This applies only to KDE keychains, so keychains used by OS X applications should not be affected. - close when last application exits. This requires maintaining a user list which keeps track of what application has what wallet open. I've implemented an internal version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it. So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome. Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work. Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose. Diffs - kdeui/util/kwallet.h d7f703f kdeui/util/kwallet_mac.h PRE-CREATION kdeui/util/kwallet_mac.cpp 8344ebb kdeui/util/qosxkeychain.h d0934e6 kdeui/util/qosxkeychain.cpp 7cb9a22 Diff: https://git.reviewboard.kde.org/r/120202/diff/ Testing --- OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 . Once finalised, all changes should port easily to KF5's kwallet_mac.cpp . Thanks, René J.V. Bertin
Re: Review Request 120376: drKonqi Fix Bug 337742 - Unable to send report: error code 410 from Bugzilla
On sep. 26, 2014, 11:54 matin, Ian Wadham wrote: Hi Frédéric, As announced on KDE Core devel, in http://lists.kde.org/?l=kde-core-develm=141016488132293w=2 about 3 weeks ago, I also am working on Dr Konqi. I am about to publish a general patch, which is aimed at the present problem posed by the change to tokens in Bugzilla https://bugs.kde.org/show_bug.cgi?id=337742, but also intends to avoid such problems in future and to be forward-portable to KF5. My approach is to check the version number of the Bugzilla software and to switch to whichever security method is appropriate for that version: cookies, tokens or passwords-only. Of course, my patch will implement tokens for the time being, but the idea is to switch automatically to passwords-only when the time comes, without any new release of KDE software being necessary. See http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService.html#LOGGING_IN in the documentation for Bugzilla 4.5.5 (the next version), as opposed to 4.4.5 (the current version). Bugzilla 4.5.5 allows tokens, but I believe they will be discontinued at some stage, though I cannot put my finger on where I have seen that discussion. This should avoid users having to experience further bugs in Dr Konqi's connection to bugs.kde.org. My patch is also intended to be extendable, so that Dr Konqi can be updated and re-released, ahead of time, if any further feature change is announced by Bugzilla and could adversely affect Dr Konqi. Frédéric Sheedy wrote: Great, good news! My patch was only a quick fix to what you are doing. Ian Wadham wrote: I did not mean that you should drop what you are doing and discard this review and patch completely... :-) Instead, we should work together and each be aware of what the other is doing. Please revive your patch and this review. From what I can tell, the patch (after review and shipping) will be good immediately and at least until the version after Bugzilla 4.5.x. Also, your patch has some improvements to the testing, which is important. And I think we need to get a fix into the closing versions of KDE 4 ASAP (next deadline Thursday, 9 October). My fixes will be more long-term and I am running short of days to work on them, due to other commitments, and anyway they may take a while to review... So please revive your review and patch, Frédéric. One immediate comment: I found that I could retrieve the token by using the tag token, but I could not use token in the args map. I had to use Bugzilla_token. I have no idea why that is. I am using an Apple OS X platform, but that should not make a difference to a web dialog. Ian Wadham wrote: Frédéric, please have a look at review https://git.reviewboard.kde.org/r/120431/ particularly the comments of the last 24 hours. Somebody is going to have to commit a patch for Dr Konqi before Albert Astals Cid starts tagging KDE 4.14.2 on Thursday night. It will be either your patch, my patch or a simplified version of my patch. If the consensus is to use your patch in KDE 4.14.2 for now, I would like to give it a test on Thursday (Australian time, UTC + 11 hours). I am otherwise engaged today (Wednesday). All being well, I could commit your patch, but do you have commit rights yourself? Frédéric Sheedy wrote: Hi Ian, I do have an account to commit the patch. Let me know of the consensus! Ian Wadham wrote: As you may have seen on https://git.reviewboard.kde.org/r/120431/ the consensus was in favour of a simplified patch, which I edited, tested and later committed on Thursday. It is regrettable that neither of our patches received a review from a KDE core developer who is familiar with the Dr Konqi code. Had that happened, things could have proceeded in a more orderly fashion and I am sure that your patch could have been shipped immediately, to fix bug 337742, and mine could have been refined and shipped within the KDE 4.14.3 or 14.12 timeframe. Frédéric, I think it is important that your fixes for the Dr Konqi test processes should go into KDE 4.14.3 or 14.12 and also into KF5. Thank you very much for your help. Hi Ian, thanks for the answer! As my fixes for Dr Konqui are not for the end users, should I commit it without a review? - Frédéric --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120376/#review67481 --- On oct. 8, 2014, 1:49 matin, Frédéric Sheedy wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120376/ ---
Re: kdepimlibs Coverity Scan Report, Oct 14 2014
On Thu, October 16, 2014 2:06 pm, Gilles Caulier wrote: 2014-10-16 12:29 GMT+02:00 Ben Cooksley bcooks...@kde.org: On Thu, Oct 16, 2014 at 8:53 PM, Gilles Caulier caulier.gil...@gmail.com wrote: Allen, Hi Gilles, Just a workflow question : why to export Coverity report to CSV where you can send automatically a mail to devel mailing list when scan is complete, with a a list of new defect found in code. I use Coverity since more than one year with whole digiKam code, and we have already fixed more than 500 entries detected. The Coverity web interface is really more suitable than a export to CSV. Defect are very well explained in source context, with all conditions used to check implementation. The only constrain is to have an account for each contributors who will fixed entries. I suspect that is why Allen is sending out the HTML/CSV output - because not everyone has access and it is helpful to have this information publicly accessible. All is configurable in Coverity interface. You can invite people and attribute role. Web interface is so far more powerful to use than CSV, and permit a time gain to fix issues. The CSV version doesn't contain line numbers, so it's impossible to know what code some of the issues refer to. I seem to remember that the web interface doesn't have that problem. -- David Jarvie. KDE developer. KAlarm author - http://www.astrojar.org.uk/kalarm
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 1043 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. René J.V. Bertin wrote: Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ... René J.V. Bertin wrote: I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of. Unless it's a remnant from the past that's no longer being used? David Faure wrote: What would be a remnant from the past? trashForMountPoint?? It's called by TrashImpl::findTrashDirectory and by TrashImpl::scanTrashDirectories. It's the way we find the trash dirs on other partitions. Surely that is used. You can e.g. try opening trash:/ in a kde file manager, it will scan for all trash dirs. Well, I've done that; opening the trash in Dolphin is part of my regular testing routine for this. And NEVER have I yet seen debug output from trashForMountPoint... - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 1043 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. René J.V. Bertin wrote: Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ... René J.V. Bertin wrote: I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of. Unless it's a remnant from the past that's no longer being used? David Faure wrote: What would be a remnant from the past? trashForMountPoint?? It's called by TrashImpl::findTrashDirectory and by TrashImpl::scanTrashDirectories. It's the way we find the trash dirs on other partitions. Surely that is used. You can e.g. try opening trash:/ in a kde file manager, it will scan for all trash dirs. René J.V. Bertin wrote: Well, I've done that; opening the trash in Dolphin is part of my regular testing routine for this. And NEVER have I yet seen debug output from trashForMountPoint... Even for the NFS mount mounted under /Volumes (comparable to /media) I just checked: the file I moved ended up in the ~/.Trash directory. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Okt. 14, 2014, 9:13 nachm., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. René J.V. Bertin wrote: Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/classTrashImpl.html On the $TRASH/KDE.trash issue and in general: OS_X stores trashed files directly in ~/.Trash, I take? In that case i foresee a general issue (maybe academic, but possible) with the approach to store the KDE trash stuff there (which OSX will take as trashed files). What happens if you delete a file named info or files or KDE.trash? In either case you'd likely run into a conflict? Ie. either OS_X cannot trash the new file (for there's a directory of that name) or OS_X overrides/invalidates the entire KDE trash or OS_X creates such file and you can't add the KDE stuff there. Yesno? In case: how does the OS_X trash handle hidden (.*) files? Assuming it doesn't put hidden files dotted into the trash, how does it react when adding a hidden file there (whether .info, .files or .KDE.trash)? - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Okt. 15, 2014, 4:26 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Okt. 15, 2014, 4:26 nachm.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. René J.V. Bertin wrote: Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? Thomas Lübking wrote: http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/classTrashImpl.html On the $TRASH/KDE.trash issue and in general: OS_X stores trashed files directly in ~/.Trash, I take? In that case i foresee a general issue (maybe academic, but possible) with the approach to store the KDE trash stuff there (which OSX will take as trashed files). What happens if you delete a file named info or files or KDE.trash? In either case you'd likely run into a conflict? Ie. either OS_X cannot trash the new file (for there's a directory of that name) or OS_X overrides/invalidates the entire KDE trash or OS_X creates such file and you can't add the KDE stuff there. Yesno? In case: how does the OS_X trash handle hidden (.*) files? Assuming it doesn't put hidden files dotted into the trash, how does it react when adding a hidden file there (whether .info, .files or .KDE.trash)? In fact, it works exactly the way the KDE trash works, except for the restore info which (I presume) is stored in the file metadata. So: - deleting a file that has the same name as a file already in the trash will 1) rename that file to something like name 1 and 2) move that file into the trash. (I've been able to see that happen when the system is very busy). In short, it's only if someone already put a KDE.trash in the trash that we'd into trouble, unless it's a directory. - Why wouldn't it put hidden files there? Again, the Finder works like Dolphin or Nautilus: hidden files are nothing special, they're just not shown in the Finder (nor in file selection dialogs). I'm not sure if the trash would show as non-empty if we store everything under ~/.Trash/.KDE.trash but if it does we have the problem that the user won't understand why when s/he opens the Trash in the Finder. And if it doesn't we're more or less back where we started. I think it's important that KDE waste becomes visible in the OS X Trash, preferably in a subdirectory. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.)
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
On Oct. 15, 2014, 10:07 p.m., Thomas Lübking wrote:
Please check https://techbase.kde.org/Policies/Kdelibs_Coding_Style
René J.V. Bertin wrote:
Sure, and guess what I noticed first ;)
```
Nobody is forced to use this style, but to have consistent formatting of
the source code files it is recommended to make use of it.
```
I won't mix and match styles in existing code, trying instead to match
the style used locally as closely as possible. In files I create from
scratch I'd prefer to stick to my own convictions, though - basically the
only difference is the absence of a space after a keyword. It's not like that
isn't exactly unprecedented ...
Thomas Lübking wrote:
Ok, I oversaw that this is entirely your code.
Well if this is actually what makes you happy, that's oc fine. And when
you ever should abandon the code, there's luckily always astyle to the rescue
:-P
the only difference is the absence of a space after a keyword
Not only. You've *sometimes* stuff like
```cpp
void foo()
{ bar();
fooBar();
}
```
and blanks in braces isn't consistent (or at least I fail to see the
pattern) - I frankly thought this was a mixup of xcode and my first day in
vim ;-)
Nope, I thought I'd been avoiding that habit but it's possible a few (only
declarations I hope) slipped through . Can't recall seeing anything making it
not done in the guidelines though, did I miss it?
- René J.V.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/#review68511
---
On Oct. 16, 2014, 1:26 p.m., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/
---
(Updated Oct. 16, 2014, 1:26 p.m.)
Review request for KDE Software on Mac OS X and kdelibs.
Repository: kdelibs
Description
---
I'm still working on (the KDE4-based version of) my OS X keychain backend for
kwallet. I'm at a point where I think I can present a work-in-progress in an
RR because at least one feature has been improved enough to be of interest
for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM
(SystemSettings), and for which I'm working on an implementation not
requiring kwalletd and/or DBus.
- idle time closing of wallets. This feature was not supported in the
commited version presented in https://git.reviewboard.kde.org/r/119838/ The
present patch adds an idleTimer and a shared lastAccessTime member. The
idleTimer is reset each time a client performs one of a series of actions
that I count as wallet accesses, and before resetting I update the idle
timeout value from KConfig. When the timer fires, the elapsed time is
compared to the shared last access time, and if it is = the timeout, the
wallet is closed. This applies only to KDE keychains, so keychains used by
OS X applications should not be affected.
- close when last application exits. This requires maintaining a user
list which keeps track of what application has what wallet open. I've
implemented an internal version of such a registry, mapping wallet name to
application names and the list of wallets they have open (a list of wallet
reference, pid per application name). The registry is functional, but I have
not yet decided (read: figured out) how to make a distributed representation
of it.
So the work-in-progress concerns the distributed user registry. The idea
would be to maintain the registry in shared memory, meaning it'd be reset (=
disappear) when the last application exits, contrary to a file which can go
stale. This would be simple if QSharedMemory objects could be resized, but
apparently they cannot, so I'll have to look at other solutions possibly
involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef
or CFMutableDataRef might be candidates). Suggestions welcome.
Other work in progress concerns a less wheel-reinventing approach that builds
on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't
work, but I must still misunderstand its finer details. The present patch
contains outcommented code that does indeed cause kwalletd to be launched and
slots and signals to become visible e.g. in `qdbusviewer`. But they don't
work, which in turn makes the whole kwallet layer dysfunctional. Here too
feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last
client exits should work out-of-the-box,
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. René J.V. Bertin wrote: Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? Thomas Lübking wrote: http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/classTrashImpl.html On the $TRASH/KDE.trash issue and in general: OS_X stores trashed files directly in ~/.Trash, I take? In that case i foresee a general issue (maybe academic, but possible) with the approach to store the KDE trash stuff there (which OSX will take as trashed files). What happens if you delete a file named info or files or KDE.trash? In either case you'd likely run into a conflict? Ie. either OS_X cannot trash the new file (for there's a directory of that name) or OS_X overrides/invalidates the entire KDE trash or OS_X creates such file and you can't add the KDE stuff there. Yesno? In case: how does the OS_X trash handle hidden (.*) files? Assuming it doesn't put hidden files dotted into the trash, how does it react when adding a hidden file there (whether .info, .files or .KDE.trash)? René J.V. Bertin wrote: In fact, it works exactly the way the KDE trash works, except for the restore info which (I presume) is stored in the file metadata. So: - deleting a file that has the same name as a file already in the trash will 1) rename that file to something like name 1 and 2) move that file into the trash. (I've been able to see that happen when the system is very busy). In short, it's only if someone already put a KDE.trash in the trash that we'd into trouble, unless it's a directory. - Why wouldn't it put hidden files there? Again, the Finder works like Dolphin or Nautilus: hidden files are nothing special, they're just not shown in the Finder (nor in file selection dialogs). I'm not sure if the trash would show as non-empty if we store everything under ~/.Trash/.KDE.trash but if it does we have the problem that the user won't understand why when s/he opens the Trash in the Finder. And if it doesn't we're more or less back where we started. I think it's important that KDE waste becomes visible in the OS X Trash, preferably in a subdirectory. Thomas Lübking wrote: Why wouldn't it put hidden files there? Counterfeit - there's only limited reason to hide a file in the trash (the kioslave simply shows them), so I could have assumed it's renamed .foo - _.foo or so. Does OS_X consider an empty directory in ~/.Trash a non-empty trash (notably if it has no meta/restorage data)? The ideal solution to me seems to ensure there's always OUR (we can hardly re-use a directory the user deleted there
Re: Using Gerrit for code review in KDE
Jan Kundrát wrote: A random data point -- I asked a 3rd-party contributor to send a patch to Trojita through Gerrit earlier today. He accomplished that goal so fast that I asked him for an estimate on how much time it took. The answer was 15 minutes, including reading the docs and setting up the client-side hooks. Quite frankly, I don't think I was faster when I first used ReviewBoard. Strange, because that's not at all the experience I had submitting patches to Qt/Gerrit compared to KDE/ReviewBoard. In ReviewBoard, I export my patch from the git-cola menu and I submit it through a nice web interface that lets me input all the details (target branch, reviewers, etc.). I don't need to fire up a Konsole at all, nor read any documentation. It just works. In Gerrit, I basically get an ugly command-line interface: I have to push to a magic ref encoding all the information (and IIRC, git-cola only lets me enter the basic refs/for/branchname, the special characters in stuff like %r=f...@example.com confuse it, so I'd have to push from a terminal if I want to use those). Setting reviewers requires a special command-line-style parameter appended to the ref that is found in the documentation (that %r= thing). There is also no autocompletion nor client-side validation of the reviewer nicks/addresses, unlike on ReviewBoard's friendly web interface. And the next time I want to submit something to Gerrit, I'm sure I'll have to reread the documentation all over again, whereas ReviewBoard is dead simple. I can see you liking Gerrit if you're used to juggling with obscure git command lines, but as a long-term user of Cervisia, kdesvn and now git-cola, I find a web submission interface much nicer to work with. Kevin Kofler
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 854 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line854 deleteEmptyTrashInfraStructure is implemented on all OSes, but only called on Mac, which seems a bit inconsistent. I looked at the trash spec again, and given the special permissions required on trash dirs in other partitions (DIR/.Trash or DIR/.Trash-$uid), I would feel safer if we didn't delete trash infrastructure. So I would make the entire method OSX only. BTW you should use Q_OS_OSX instead of Q_OS_MAC. iOS for sure doesn't work this way. René J.V. Bertin wrote: KDE on iOS, seriously? Even if so, I'd hope Qt don't use Q_OS_MAC for that, because 1 MAC as in Macintosh refers to a line of desktop and laptop computers, not the iDevices 2 iOS is in fact an embedded form of OS X René J.V. Bertin wrote: Re: iOS: here's how Qt5 defines the platform tokens: ```C++ #if defined(Q_OS_DARWIN) # define Q_OS_MAC # if defined(Q_OS_DARWIN64) # define Q_OS_MAC64 # elif defined(Q_OS_DARWIN32) # define Q_OS_MAC32 # endif # include TargetConditionals.h # if defined(TARGET_OS_IPHONE) TARGET_OS_IPHONE # define Q_OS_IOS # elif defined(TARGET_OS_MAC) TARGET_OS_MAC # define Q_OS_OSX # define Q_OS_MACX // compatibility synonym # endif #endif ``` David Faure wrote: Yes, so Q_OS_MAC means OSX and iOS, while this code is OSX only and won't work on iOS, hence my suggestion to use Q_OS_OSX. In fact, Q_OS_OSX was introduced by Jake Petroules in Qt 5.2, so doesn't exist in Qt 4.8 ... - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 17, 2014, 12:35 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Changes --- New version taking into account the various suggestions. Trash infrastructure is now created only when really needed (please check if I forgot any cases), which does NOT include scanning the trash nor doing something with a trashed file (knowing a trashed file by name should mean the infrastruct. exists). Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120554: Initial frameworks port of kompare
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/#review68582 --- See my point by point review below. Once all the issues are addressed, we should be all set, but I'll have another look before giving it a Ship It. CMakeLists.txt https://git.reviewboard.kde.org/r/120554/#comment47766 No app icon anymore on the proprietary system(s) using those? Looks like the discussion is still ongoing on what to do with that: http://lists.kde.org/?t=14078460994r=1w=2 so I guess we'll have to wait for its outcome. :-( (Thus, no issue raised here.) komparenavtreepart/komparenavtreepart.cpp https://git.reviewboard.kde.org/r/120554/#comment47768 Please just delete this obsolete macro entirely. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47769 This should pass at least QUrl::RemoveUserInfo to toString(), we don't want to echo passwords in error messages. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47770 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47771 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47772 See above. (In this case, you'll also need to add the explicit .toString(QUrl::RemoveUserInfo) call.) komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47773 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47774 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47775 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47776 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment4 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47778 See above. komparepart/kompare_part.cpp https://git.reviewboard.kde.org/r/120554/#comment47779 See above. komparepart/kompareprefdlg.cpp https://git.reviewboard.kde.org/r/120554/#comment47780 If OK is automatically the default button, just delete this line. If it is not, then please replace it with something that does the job, e.g.: button( QDialogButtonBox::Ok )-setDefault( true ); komparepart/kompareprefdlg.cpp https://git.reviewboard.kde.org/r/120554/#comment47781 There goes the button separator. If button separators are generally unwanted now (as seems the case, judging from the porting script that just deletes the showButtonSeparator calls), we should just remove the line entirely rather than commenting it out. If they're wanted, we should find a way to show them. Looking at the dialog in my KDE 4 Kompare, I think the separator is not really needed to begin with, so you can just delete the commented-out line. kompareurldialog.cpp https://git.reviewboard.kde.org/r/120554/#comment47767 There goes the button separator. If button separators are generally unwanted now (as seems the case, judging from the porting script that just deletes the showButtonSeparator calls), we should just remove the line entirely rather than commenting it out. If they're wanted, we should find a way to show them. Looking at the dialog in my KDE 4 Kompare, I think the separator is not really needed to begin with, so you can just delete the commented-out line. libdialogpages/diffpage.cpp https://git.reviewboard.kde.org/r/120554/#comment47783 This one ought to be toLocalFile, I think. Or at least pass QUrl::PreferLocalFile to toString(), but I don't think a URL makes sense as a diff program, does it? - Kevin Kofler On Okt. 16, 2014, 5:24 vorm., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Okt. 16, 2014, 5:24 vorm.) Review request for kdelibs and Kevin Kofler. Repository: kompare Description --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. I'll reread my changes also but wanted to get this out there to be played with also. Diffs - libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png pics/hi22-app-kompare.png pics/hi32-app-kompare.png
Re: Review Request 120554: Initial frameworks port of kompare
On Okt. 16, 2014, 10:47 nachm., Kevin Kofler wrote: komparepart/kompare_part.cpp, line 295 https://git.reviewboard.kde.org/r/120554/diff/4/?file=318659#file318659line295 This should pass at least QUrl::RemoveUserInfo to toString(), we don't want to echo passwords in error messages. (Somebody found this serious enough an issue to file http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-2074 against KHTML, where the fix back then was to use prettyUrl. I hope there aren't more such issues introduced by this sloppy prettyUrl KF5 porting.) - Kevin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/#review68582 --- On Okt. 16, 2014, 5:24 vorm., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Okt. 16, 2014, 5:24 vorm.) Review request for kdelibs and Kevin Kofler. Repository: kompare Description --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. I'll reread my changes also but wanted to get this out there to be played with also. Diffs - libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png pics/hi22-app-kompare.png pics/hi32-app-kompare.png pics/hi48-app-kompare.png pics/hisc-app-kompare.svgz libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f15cfc komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 Diff: https://git.reviewboard.kde.org/r/120554/diff/ Testing --- It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not. Thanks, Jeremy Whiting
Re: Review Request 120554: Initial frameworks port of kompare
On Okt. 16, 2014, 10:47 nachm., Kevin Kofler wrote: komparepart/kompare_part.cpp, line 295 https://git.reviewboard.kde.org/r/120554/diff/4/?file=318659#file318659line295 This should pass at least QUrl::RemoveUserInfo to toString(), we don't want to echo passwords in error messages. Kevin Kofler wrote: (Somebody found this serious enough an issue to file http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-2074 against KHTML, where the fix back then was to use prettyUrl. I hope there aren't more such issues introduced by this sloppy prettyUrl KF5 porting.) Actually, rather than QUrl::RemoveUserInfo, QUrl::RemovePassword is enough, you don't have to (and probably shouldn't) strip the user name. But you definitely don't want to output the password. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/#review68582 --- On Okt. 16, 2014, 5:24 vorm., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Okt. 16, 2014, 5:24 vorm.) Review request for kdelibs and Kevin Kofler. Repository: kompare Description --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. I'll reread my changes also but wanted to get this out there to be played with also. Diffs - libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png pics/hi22-app-kompare.png pics/hi32-app-kompare.png pics/hi48-app-kompare.png pics/hisc-app-kompare.svgz libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f15cfc komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 Diff: https://git.reviewboard.kde.org/r/120554/diff/ Testing --- It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not. Thanks, Jeremy Whiting
Porting KUrl::prettyUrl: please do not reintroduce CVE-2013-2074!
Hi, just a small public service announcement: The correct replacement for: url.prettyUrl() in Qt 5 is NOT: url.toString() // BAD! but: url.toString(QUrl::RemovePassword) The old KUrl::prettyUrl() always removed passwords. You DON'T want to show passwords in user output: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-2074 (I found this reviewing the initial port of Kompare.) Thanks for reading, Kevin Kofler
Re: Review Request 120554: Initial frameworks port of kompare
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Oct. 16, 2014, 7:45 p.m.) Review request for kdelibs and Kevin Kofler. Changes --- Fixed showing passwords in urls. Removed commented out macro and old code. The ok button was default here, It's default unless another button is told to be. Repository: kompare Description --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. I'll reread my changes also but wanted to get this out there to be played with also. Diffs (updated) - CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png pics/hi22-app-kompare.png pics/hi32-app-kompare.png pics/hi48-app-kompare.png pics/hisc-app-kompare.svgz interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f15cfc komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe Diff: https://git.reviewboard.kde.org/r/120554/diff/ Testing --- It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not. Thanks, Jeremy Whiting
Re: Review Request 120554: Initial frameworks port of kompare
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/#review68587 --- Oh, I missed this one: The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. kompare_shell.cpp https://git.reviewboard.kde.org/r/120554/#comment47790 You apparently can't pass a MIME type directly to the static QFileDialog functions. Use QMimeDatabase().mimeTypeForName(text/x-patch).filterString() instead. (The QFileDialog::setMimeTypeFilters convenience method appears not to be exported through the static API.) - Kevin Kofler On Okt. 17, 2014, 1:45 vorm., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Okt. 17, 2014, 1:45 vorm.) Review request for kdelibs and Kevin Kofler. Repository: kompare Description --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. I'll reread my changes also but wanted to get this out there to be played with also. Diffs - CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png pics/hi22-app-kompare.png pics/hi32-app-kompare.png pics/hi48-app-kompare.png pics/hisc-app-kompare.svgz interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f15cfc komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe Diff: https://git.reviewboard.kde.org/r/120554/diff/ Testing --- It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not. Thanks, Jeremy Whiting
Re: Review Request 120554: Initial frameworks port of kompare
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Oct. 16, 2014, 7:48 p.m.) Review request for kdelibs and Kevin Kofler. Repository: kompare Description (updated) --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. Note I'm thinking I'll push this to a frameworks branch for further refinement. no need to merge it to master until it's solid. Diffs - CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png pics/hi22-app-kompare.png pics/hi32-app-kompare.png pics/hi48-app-kompare.png pics/hisc-app-kompare.svgz interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f15cfc komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe Diff: https://git.reviewboard.kde.org/r/120554/diff/ Testing --- It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not. Thanks, Jeremy Whiting
Re: Review Request 120554: Initial frameworks port of kompare
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Oct. 16, 2014, 7:53 p.m.) Review request for kdelibs and Kevin Kofler. Changes --- Fixed QFileDialog mime-type parameter. Repository: kompare Description --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. Note I'm thinking I'll push this to a frameworks branch for further refinement. no need to merge it to master until it's solid. Diffs (updated) - CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f15cfc komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c pics/hi22-app-kompare.png pics/hi32-app-kompare.png pics/hi48-app-kompare.png pics/hisc-app-kompare.svgz pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 Diff: https://git.reviewboard.kde.org/r/120554/diff/ Testing --- It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not. Thanks, Jeremy Whiting
Re: Review Request 120554: Initial frameworks port of kompare
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120554/ --- (Updated Oct. 17, 2014, 2:05 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs and Kevin Kofler. Repository: kompare Description --- I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. Note I'm thinking I'll push this to a frameworks branch for further refinement. no need to merge it to master until it's solid. Diffs - CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f15cfc komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c pics/hi22-app-kompare.png pics/hi32-app-kompare.png pics/hi48-app-kompare.png pics/hisc-app-kompare.svgz pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e pics/hi128-app-kompare.png pics/hi16-app-kompare.png libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 Diff: https://git.reviewboard.kde.org/r/120554/diff/ Testing --- It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc. Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not. Thanks, Jeremy Whiting
Re: Porting KUrl::prettyUrl: please do not reintroduce CVE-2013-2074!
I personally think QUrl should remove the password by default when converting to string and force caller of the API to explicitly request the inclusion of the password say by changing the modifier option to a QUrl::IncludePassword. It is better to be safer out of the box. On Thu, Oct 16, 2014 at 8:53 PM, Kevin Kofler kevin.kof...@chello.at wrote: Hi, just a small public service announcement: The correct replacement for: url.prettyUrl() in Qt 5 is NOT: url.toString() // BAD! but: url.toString(QUrl::RemovePassword) The old KUrl::prettyUrl() always removed passwords. You DON'T want to show passwords in user output: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-2074 (I found this reviewing the initial port of Kompare.) Thanks for reading, Kevin Kofler
