D21041: [Fstab] Use folder-decrypted icon for encrypting fuse mounts

2019-05-06 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > fstabdevice.cpp:62 > } > +if ((fstype == QLatin1String("fuse.encfs")) || > +(fstype == QLatin1String("fuse.cryfs"))) { Would it make sense to wrap this in a function with a more expressive name like:

D20995: [Fstab] Add support for non-network filesystems

2019-05-06 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > ngraham wrote in fstabhandling.cpp:126 > Now that the refactoring's been done, it should be easy to add support for > those or remove the conditional. Yes indeed :D I was just wondering why it was made the way it is? Is there a reason for the

D20995: [Fstab] Add support for non-network filesystems

2019-05-06 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > fstabhandling.cpp:126 > +{ > +if (fstype == "fuse.encfs" || > +fstype == "fuse.cryfs") { Why do we limit to these two filesystems? I think it would be really useful to show all kinds of fuse based filesystems, e.g. isofs, sshfs and

D20938: Add Mounts Backend

2019-05-02 Thread David Hallas
hallas added a comment. I have tried to modify the fstab backend to also show fuse mounts and a //very// simple prototype is this: diff --git a/src/solid/devices/backends/fstab/fstabhandling.cpp b/src/solid/devices/backends/fstab/fstabhandling.cpp index 63130c6..3632b52 100644

D20938: Add Mounts Backend

2019-05-02 Thread David Hallas
hallas added a comment. In D20938#459628 , @bruns wrote: > In D20938#459627 , @hallas wrote: > > > In D20938#459218 , @bruns wrote: > > > > > Solid

D20938: Add Mounts Backend

2019-05-02 Thread David Hallas
hallas added a comment. In D20938#459218 , @bruns wrote: > Solid already has a working implementation for reading from /proc/mounts, the fstab backend. > > Contrary to this code, the fstab backend does not poll every second using a timer,

D20938: Add Mounts Backend

2019-05-02 Thread David Hallas
hallas added a comment. In D20938#459149 , @ivan wrote: > I'm torn between two approaches: > > - doing what you have done, maybe with a customization point - `fusermount -u` by default, something else for specific mount types; > -

D20938: Add Mounts Backend

2019-05-01 Thread David Hallas
hallas added a comment. In D20938#459076 , @ivan wrote: > Thanks for working on this. > > I'd probably call this `fusemounts` backend as it handles only FUSE mounts and nothing else. > > As for the //teardown// operation, some FUSE

D20938: Add Mounts Backend

2019-05-01 Thread David Hallas
hallas added a comment. This commit is still work-in-progress, but I would really like to get some feedback to the approach. Does it make sense to add a new backend? Or should this functionality be merged with one of the other backends (I was considering the fstab backend)? REPOSITORY

D20938: Add Mounts Backend

2019-05-01 Thread David Hallas
hallas created this revision. hallas added reviewers: Frameworks, ngraham, elvisangelaccio. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Adds a new devices backend providing information on Fuse

D20860: Fix KBoomarkMenuTest when Bookmarks Editor is not installed

2019-04-27 Thread David Hallas
hallas closed this revision. REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D20860 To: hallas, dfaure, aacid, ngraham Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20860: Fix KBoomarkMenuTest when Bookmarks Editor is not installed

2019-04-27 Thread David Hallas
hallas updated this revision to Diff 57100. hallas marked 4 inline comments as done. hallas added a comment. Make the global functions static. REPOSITORY R294 KBookmarks CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20860?vs=57084=57100 BRANCH

D20860: Fix KBoomarkMenuTest when Bookmarks Editor is not installed

2019-04-27 Thread David Hallas
hallas marked an inline comment as not done. REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D20860 To: hallas, dfaure, aacid, ngraham Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-27 Thread David Hallas
hallas added a comment. In D20209#456311 , @aacid wrote: > In D20209#456213 , @hallas wrote: > > > In D20209#455579 , @aacid wrote: > > > > > @hallas

D20860: Fix KBoomarkMenuTest when Bookmarks Editor is not installed

2019-04-27 Thread David Hallas
hallas created this revision. hallas added reviewers: dfaure, aacid, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Fix KBoomarkMenuTest when Bookmarks Editor is not installed. The test

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-25 Thread David Hallas
hallas added a comment. In D20209#455579 , @aacid wrote: > @hallas but tests are still failing since your previous commit. Can you have a look? https://build.kde.org/job/Frameworks/job/kbookmarks/job/kf5-qt5%20SUSEQt5.10/21/testReport/

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-24 Thread David Hallas
hallas added a comment. In D20209#455579 , @aacid wrote: > @hallas but tests are still failing since your previous commit. Can you have a look? https://build.kde.org/job/Frameworks/job/kbookmarks/job/kf5-qt5%20SUSEQt5.10/21/testReport/

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-24 Thread David Hallas
hallas closed this revision. REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D20209 To: hallas, #frameworks, ngraham, cfeck, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-23 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > dfaure wrote in kbookmarkmenu.cpp:150 > Technically this is only needed if the number of open tabs went from "< 2" to > ">= 2" or vice versa. > When going from, say, 20 to 21, we don't need to refill the menu. > So the code could be > >

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-23 Thread David Hallas
hallas updated this revision to Diff 56837. hallas marked an inline comment as done. hallas added a comment. Review comments REPOSITORY R294 KBookmarks CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20209?vs=56744=56837 BRANCH add_tabs_open (branched from master) REVISION

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-22 Thread David Hallas
hallas added a comment. In D20209#450100 , @dfaure wrote: > Urgh. Indeed. And looking around I find many inline virtuals in apparently public headers... http://www.davidfaure.fr/2019/inline_virtual_dtors.diff (though maybe some of these don't

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-22 Thread David Hallas
hallas updated this revision to Diff 56744. hallas added a comment. Reworked the patch to avoid any ABI breakage. Now the new functionality is in KBookmarkMenu instead. REPOSITORY R294 KBookmarks CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20209?vs=55300=56744 BRANCH

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-04-14 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > davidedmundson wrote in kprocesslist.cpp:69 > With the QSharedDataPointer you can delete this method. > > The point of the shareddatapointer is that when you copy KProcessInfo you > don't copy all the members, we just increase the internal

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-04-14 Thread David Hallas
hallas updated this revision to Diff 56237. hallas added a comment. Review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20007?vs=54949=56237 BRANCH adds_kprocesslist (branched from master) REVISION DETAIL

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-14 Thread David Hallas
hallas added a comment. Hi @dfaure - I looked into this a bit more and have run into a problem, I hope you can help solve. The problem is that when I add a getter and a setter for the number of open tabs, I need to store the value somewhere in `KBookmarkOwner` but I guess I cannot add a

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-14 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > dfaure wrote in kbookmarkowner.h:120 > Every version of KF5 is source and binary compatible (just like Qt5). > The next time for a possible BC breakage will be KF6 (when Qt6 comes out), so > not just yet. > > Two alternative solutions: > > -

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-14 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > dfaure wrote in kbookmarkowner.h:120 > One day we'll probably wonder how many tabs are actually open, to avoid > saying "Bookmark Tabs As Folder" if there's only one tab. So I'd prefer for > that and make this an int rather than a bool. > >

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-11 Thread David Hallas
hallas added a comment. Ping - anyone ;) REPOSITORY R294 KBookmarks REVISION DETAIL https://phabricator.kde.org/D20209 To: hallas, #frameworks, ngraham Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-04-11 Thread David Hallas
hallas added a comment. @davidedmundson - could you please take a look at this again? I have implemented all the stuff you suggested ;) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: vonreth, adridg,

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

2019-04-02 Thread David Hallas
hallas created this revision. hallas added reviewers: Frameworks, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Add support for KBookmarkOwner to communicate if it has tabs open. This is

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-27 Thread David Hallas
hallas added a comment. BTW - I tried locally chaning the D19989 patch to use the new `KProcessList::processInfoList` function and that enables us to achieve the same thing but without the KSysGuard dependency, so that's great :) Another thing,

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-27 Thread David Hallas
hallas added a comment. In D20007#438640 , @adridg wrote: > On FreeBSD, `/proc` is not necessarily mounted (it might be a Linuxism). So while I do **have** `/proc`, it's empty because procfs isn't mounted there. If I **do** mount it, then

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-27 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > davidedmundson wrote in kprocesslisttest.cpp:38 > KUser::KUser() > > (fortunately it's in kcoreaddons!) Nice :) I didn't know that one > davidedmundson wrote in kprocesslisttest.cpp:52 > qint64 QCoreApplication::applicationPid() Got it - I have

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-27 Thread David Hallas
hallas updated this revision to Diff 54949. hallas marked 7 inline comments as done. hallas added a comment. Implemented review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20007?vs=54827=54949 BRANCH adds_kprocesslist (branched from

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-25 Thread David Hallas
hallas added a comment. Ok, I have implemented all the review feedback you gave, I have also done some unit tests. Some of the unit tests or Unix specific though, I still don't know what to do about Windows since I don't have a development environment. Please give it a good look :D

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-25 Thread David Hallas
hallas marked 9 inline comments as done. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-25 Thread David Hallas
hallas updated this revision to Diff 54827. hallas added a comment. Updated with review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20007?vs=54638=54827 BRANCH adds_kprocesslist (branched from master) REVISION DETAIL

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-24 Thread David Hallas
hallas added a comment. Thanks a lot for the feedback guys, I will spend some time and implement it and push an updated patch. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D20007 To: hallas, davidedmundson, broulik Cc: elvisangelaccio, kde-frameworks-devel,

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-23 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > CMakeLists.txt:246 >util/kformat.h >util/kuser.h >util/kshell.h Should util/kprocesslist.h also be added here? > kprocesslist.h:1 > +/** > +** Is

D20007: Add GetProcessList for retrieving the list of currently active processes

2019-03-23 Thread David Hallas
hallas created this revision. hallas added reviewers: davidedmundson, broulik. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Add GetProcessList for retrieving the list of currently active

D19767: Fix malloc/delete mismatch

2019-03-17 Thread David Hallas
hallas closed this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D19767 To: hallas, #frameworks, dfaure, sitter, aacid Cc: aacid, kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns,

D19767: Fix malloc/delete mismatch

2019-03-17 Thread David Hallas
hallas added a comment. Good catch ;) I have changed it to call `ssh_string_free_char` instead REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D19767 To: hallas, #frameworks, dfaure, sitter Cc: aacid, kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh,

D19767: Fix malloc/delete mismatch

2019-03-17 Thread David Hallas
hallas updated this revision to Diff 54054. hallas marked 2 inline comments as done. hallas added a comment. Use ssh_string_free_char instead of free as documentated by libssh REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19767?vs=53911=54054 BRANCH

D19768: Fixes memory leak of ssh_session

2019-03-14 Thread David Hallas
hallas closed this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D19768 To: hallas, #frameworks, sitter, aacid Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp,

D19768: Fixes memory leak of ssh_session

2019-03-14 Thread David Hallas
hallas created this revision. hallas added a reviewer: Frameworks. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Fixes memory leak of ssh_session. The mSession member variable is

D19767: Fix malloc/delete mismatch

2019-03-14 Thread David Hallas
hallas created this revision. hallas added a reviewer: Frameworks. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Fixes pointer was allocated with malloc (by libssh) but freed with

D19170: Fix crash while moving files

2019-03-03 Thread David Hallas
hallas closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: cfeck, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

2019-03-02 Thread David Hallas
hallas marked 4 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: cfeck, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

2019-03-02 Thread David Hallas
hallas updated this revision to Diff 53032. hallas added a comment. Set job to nullptr before stopping job running in parallel REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19170?vs=52643=53032 BRANCH fix_crash_while_moving_files (branched from master)

D19170: Fix crash while moving files

2019-02-26 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > filecopyjob.cpp:495 > +} else if (job == d->m_chmodJob) { > +if (d->m_delJob) { > +d->m_delJob->kill(Quietly); Should we also set d->m_chmodJob to nullptr here, just like the m_putJob handling branch? >

D19170: Fix crash while moving files

2019-02-26 Thread David Hallas
hallas updated this revision to Diff 52643. hallas added a comment. Updated fix. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19170?vs=52240=52643 BRANCH fix_crash_while_moving_files (branched from master) REVISION DETAIL

D19170: Fix crash while moving files

2019-02-26 Thread David Hallas
hallas added a comment. In D19170#418234 , @dfaure wrote: > I think it's an unwanted fact that the chmod job runs in parallel with the del job. > A "return" after creating the chmod job would fix all this. > > But of course it should be

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-25 Thread David Hallas
This revision was automatically updated to reflect the committed changes. Closed by commit R241:f56af9e2ea0a: Fix crash in Dolphin when dropping trashed file in trash (authored by hallas). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19168?vs=52508=52509

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-25 Thread David Hallas
hallas updated this revision to Diff 52508. hallas added a comment. Rebase REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19168?vs=52132=52508 BRANCH fix_crash_when_dropping_trashed_file_in_trash (branched from master) REVISION DETAIL

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-24 Thread David Hallas
hallas added a comment. @dfaure - do you have any other comments for this commit? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19168 To: hallas, #frameworks, elvisangelaccio, ngraham, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

2019-02-22 Thread David Hallas
hallas added a comment. In D19170#417274 , @dfaure wrote: > Let's find out :-) > > Note that SlaveBase already has a warning in case a slave emits finished() or error() twice; on the other hand this might not be the issue here, it could be

D19170: Fix crash while moving files

2019-02-22 Thread David Hallas
hallas added a comment. In D19170#417235 , @dfaure wrote: > Great analysis, thanks! Now this makes a lot more sense ;-) > > The thing that I don't get, is why the subjob would emit anything after result(). I.e. step 7 is not supposed to

D19170: Fix crash while moving files

2019-02-21 Thread David Hallas
hallas edited the summary of this revision. hallas edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19170 To: hallas, #frameworks, elvisangelaccio, dfaure Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D19170: Fix crash while moving files

2019-02-21 Thread David Hallas
hallas updated this revision to Diff 52240. hallas added a comment. Implement a proper fix REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19170?vs=52108=52240 BRANCH fix_crash_while_moving_files (branched from master) REVISION DETAIL

D19170: Fix crash while moving files

2019-02-21 Thread David Hallas
hallas added a comment. I think I have finally found the root cause of this. The following events occurs: 1. A FileCopyJob is created and added as a SubJob to CopyJob 2. CopyJob::slotResult is notified 3. CopyJobPrivate::slotResultCopyingFiles is called because it is in state

D19170: Fix crash while moving files

2019-02-20 Thread David Hallas
hallas added a comment. In D19170#415760 , @dfaure wrote: > If some code is deleting this job from a slot connected to it, that code needs to be fixed. This hack isn't a fix, it will only create more problems.. > > E.g. if some other bad

D19170: Fix crash while moving files

2019-02-20 Thread David Hallas
hallas added a comment. In D19170#416120 , @hallas wrote: > In D19170#416067 , @hallas wrote: > > > In D19170#415760 , @dfaure wrote: > > > > > If some

D19170: Fix crash while moving files

2019-02-20 Thread David Hallas
hallas added a comment. In D19170#416067 , @hallas wrote: > In D19170#415760 , @dfaure wrote: > > > If some code is deleting this job from a slot connected to it, that code needs to be fixed. This

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-20 Thread David Hallas
hallas marked 3 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19168 To: hallas, #frameworks, elvisangelaccio, ngraham, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-20 Thread David Hallas
hallas updated this revision to Diff 52132. hallas added a comment. Implemented review comments REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19168?vs=52105=52132 BRANCH fix_crash_when_dropping_trashed_file_in_trash (branched from master) REVISION DETAIL

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-20 Thread David Hallas
hallas added a comment. In D19168#415767 , @dfaure wrote: > Looks good, just some minor improvement suggestions for the unittest. Thanks for the suggestions - I am still quite new to using QtTest :) Essentially I copied one of the other

D19170: Fix crash while moving files

2019-02-19 Thread David Hallas
hallas created this revision. hallas added reviewers: Frameworks, elvisangelaccio. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Fix crash while moving files. The backtrace points towards the KJob

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-19 Thread David Hallas
hallas added reviewers: elvisangelaccio, ngraham. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D19168 To: hallas, #frameworks, elvisangelaccio, ngraham Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19168: Fix crash in Dolphin when dropping trashed file in trash

2019-02-19 Thread David Hallas
hallas created this revision. hallas added a reviewer: Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Fix crash in Dolphin when dropping trashed file in trash. The actual crash happens

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-10 Thread David Hallas
hallas closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D14666 To: hallas, dfaure Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-10 Thread David Hallas
hallas updated this revision to Diff 39393. hallas added a comment. Rebased REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14666?vs=39265=39393 BRANCH master REVISION DETAIL https://phabricator.kde.org/D14666 AFFECTED FILES

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-08 Thread David Hallas
hallas added a comment. In D14666#305209 , @dfaure wrote: > Very nice work, thanks! > > Do you have a KDE contributor account, to push this commit? No, not yet - do you think I should apply for one? I would really like to continue

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-07 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > dfaure wrote in kurlcombobox.cpp:358 > problem: `i` shouldn't be in increased after this... > > Maybe this should use iterators instead? > > A unitttest is missing for this code path, in any case. In general this class is not covered very well

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-07 Thread David Hallas
hallas updated this revision to Diff 39265. hallas marked 2 inline comments as done. hallas added a comment. Use remove_if for removing entries from itemList and defaultList. Added unit test of KUrlComboBox::removeUrl. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-07 Thread David Hallas
hallas updated this revision to Diff 39252. hallas added a comment. Refactored to use std::vector and std::unique_ptr REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14666?vs=39249=39252 BRANCH master REVISION DETAIL https://phabricator.kde.org/D14666

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-07 Thread David Hallas
hallas added a comment. In D14666#304848 , @dfaure wrote: > Why shared? I don't see any ownership sharing happening. The list owns the pointers. I'm always wary of shared_ptr because it's often overused as "we don't really know who's

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-07 Thread David Hallas
hallas updated this revision to Diff 39249. hallas added a comment. Refactored the code to use std::shared pointers instead. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14666?vs=39240=39249 BRANCH master REVISION DETAIL

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-07 Thread David Hallas
hallas added inline comments. INLINE COMMENTS > dfaure wrote in kurlcombobox.cpp:363 > Doesn't this have the same issue? delete missing Sure has :) I was thinking if a nicer solution would be to use a smart pointer in the itemList instead? That way we wouldn't need to go hunting for this and

D14666: Fixes memory leak in KUrlComboBox::setUrl

2018-08-07 Thread David Hallas
hallas created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Subject: Fixes memory leak in KUrlComboBox::setUrl Details: If setUrl is called

<    1   2