from:"David Hallas"

D27152: Introduce FilesystemEntry class

2020-04-22 Thread David Hallas

hallas marked an inline comment as done.
hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:374
> The `id()` method and members are still part of the FstabEntry, where they do 
> not belong.
> 
> The UDIs go through some further mangling in `FstabHandling::deviceList()`, 
> which should be covered by the unit-tests as well.
> 
> Conceptually, the 
> `_k_deviceNameForMountpoint`/`_k_deviceNameForFilesystemEntry` should be part 
> of the m_mtabCache/m_fstabCache class (which currently is just a 
> `QMultiHash<>`). This class can then be unit tested easily.
> 
> For now, keep the `_k_deviceNameForMountpoint` inside fstabhandling.cpp

@bruns - I am little confused now. You wrote previously that you could see the 
point in the `id()` function, but now you want it moved? 
What kind of unit test are you missing? I can't quite follow the added mangling 
in `FstabHandling::deviceList()`? I don't think we have (or ever have) had any 
unit test of FstabHandling, but that was one of the things I am trying to 
achieve was this patch series that breaks up and decouples the code pieces into 
smaller bits.

Could I ask you to take a look at it again? Sorry for nagging, I just really 
want to close this up soon and move on with the next patches ;D

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-04-11 Thread David Hallas

hallas marked an inline comment as done.
hallas added a comment.


  Hi @bruns  - did you have a chance to go through this patch again? Am I 
missing anything to move on with this?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-03-09 Thread David Hallas

hallas marked 4 inline comments as done.
hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:374
> For now just keep it here, I can see now benefit of making it public (even 
> unexported).
> 
> And thanks for baring with my nitpicking ;-), this now is pretty much what I 
> hoped for.

No problem, I really appreciate the thorough review feedback you provide. I 
definitely prefer to take more time and get things right, then to rush things 
and potentially get them wrong.

So, have you had a chance to test out the patch? Do you think it is ready to 
land?

I will also start to prepare the next patch as part of splitting up the 
mega-refactor patch from D26600 

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-03-09 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:374
> Why is the id() a property of the entry now?
> 
> - it does not correspond to anything from the fstab/mtab
> - it is only used here to generate the key for the cache
> 
> Please move the id generation back here, eventually changing it from 
> `_k_deviceNameForMountpoint(const QString , const QString 
> ,const QString )` to `_k_deviceIdForFsentry(const 
> FilesystemEntry&)`

I guess the main reason is that the id is a function of a `FilesystemEntry`, 
meaning that the id is computed from the contents of it. You are absolutely 
right that it is not picked up directly from the fstab/mtab file, but it is 
computed from the contents. I think it should either be a member function of 
the `FilesystemEntry` class or a static member function of some other class 
(taking the `FilesystemEntry` as an input paramter) - it at least has to be in 
a place where it is possible to write a unit test for it's functionality. 
especially since the id() is used as part of the public API. Therefore I don't 
think we should make this an internal function in fstabhandling. Another reason 
is that, we might need the id in other places then the fstabhandling, at least 
we need it for the rest of the fstab handling refactor I posted in the other 
review.

So long story short :)
I think we should either keep it here or make it a public (static) method 
somewhere else - @bruns what do you think?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-03-04 Thread David Hallas

hallas updated this revision to Diff 76990.
hallas marked 7 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27152?vs=76898=76990

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  autotests/filesystem_type_test.cpp
  autotests/filesystem_type_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/filesystem_type.cpp
  src/solid/devices/backends/fstab/filesystem_type.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-03-04 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in filesystem_entry.cpp:31
> This is a behavioral change:
> 
> - overlayfs is no longer handled
> - fuse.cryfs is no longer handled
> 
> - encfs gets an extra '@', and the prefix is changed from  to 
> 
> The identifier is API, don't change it. E.g. dolphin may reference it.

I have added handling of overlayfs and CryFs and removed the '@' separator. 
Actually I don't know why I added the '@', I was sure that I had seen it 
somewhere in the code, but I guess that is not the case ;)

I tried to do a solid-hardware5 list details and compare the before and after 
output, and it should be the same now. I tested with both encfs and cryfs 
filesystems.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-03-03 Thread David Hallas

hallas updated this revision to Diff 76898.
hallas marked 9 inline comments as done.
hallas added a comment.


  Addressed review comments

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27152?vs=75961=76898

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  autotests/filesystem_type_test.cpp
  autotests/filesystem_type_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/filesystem_type.cpp
  src/solid/devices/backends/fstab/filesystem_type.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-03-03 Thread David Hallas

hallas added a comment.


  @bruns  - thanks a lot for the feedback, I have updated the patch with your 
suggestions.

INLINE COMMENTS

> bruns wrote in filesystem_entry.cpp:77
> Why don't you just keep the string?

It was merely meant as an optimization, I don't know if it makes sense - I'll 
remove it.

> bruns wrote in fstabdevice.cpp:41
> Ircs, raw pointers ... you newer know who owns them ... It is a pointer to an 
> entry in a QHash, it may be deleted/moved when you don't expect it.
> 
> Unfortunately, std::optional is C++17 only.
> 
> Return it by value, return a default constructed instance if no found, and 
> compare `fsEntry.device() != m_device`

Good point :) I have changed this as you suggested, but I have added a method 
to `FilesystemEntry` called `isValid` to check if it is valid

> bruns wrote in fstabhandling.cpp:264
> this looks wrong ...

Yes definitely :) I have removed the line

> bruns wrote in fstabhandling.cpp:316
> this is bad, bad, bad, ...

Fixed

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-03-02 Thread David Hallas

hallas added a comment.


  Hi @bruns  - I have updated this patch with the changes you requested and 
would really like your feedback :)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-02-19 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> meven wrote in filesystem_entry.cpp:47
> This sound sane to me.
> Ideally we would store only the string in case of an unknown fs or the enum 
> but not both for a same entry, the choice could be done in ctor.

I have added a new enum class type `FilesystemType` to represent the known 
filesystem types. Currently I have just added a single known filesystem type, 
but this should be extended over time. I have also added the necessary from/to 
string conversion functions.

> bruns wrote in filesystem_entry.h:34
> wrap long lines, also below.

I have wrapped all lines to fit 80 characters in width

> bruns wrote in filesystem_entry.h:48
> Whats wrong with mountPoint? Its used everywhere else here, and is a well 
> known term.

Good point :)
Actually, I don't know why I called the function `mountPath` instead of 
`mountPoint` - but I have reamed it, along with the member etc.

> bruns wrote in fstabhandling.cpp:259
> detaches m_fstabCache, also below.

The code didn't need the for loop anyway, so I have rewritten it

> meven wrote in fstabhandling.cpp:388
> Detaches m_mtabCache, just qAsConst or temporary const variable

The code didn't need the for loop anyway, so I have rewritten it

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27504: WIP: RFC: smb faster copy to local

2020-02-19 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kio_smb_dir.cpp:49
> Segment does not free its memory. Why not use QByteArray or similar with 
> automatic storage allocation?

Yes, you could (and should ;) ) use standard C++ for this, i.e.:

  std::unique_ptr buf;
  buf = std::make_unique(segmentSize);

> kio_smb_dir.cpp:168
> +std::condition_variable m_cond;
> +Segment *m_buffer[4]; // should be at least 3 to give smbc some read 
> ahead leeway
> +const size_t m_capacity = 4;

Use standard C++ for this, i.e.:

  std::array, m_capacity> m_buffer;

This makes `m_buffer` iterable and it also ensures that the segments are always 
correctly freed.

> kio_smb_dir.cpp:494
> +Buffer buffer(st.st_size);
> +QFuture future = QtConcurrent::run([, ]() -> int {
> +while (true) {

What is the reason to use `QFuture` over `std::future`?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27504

To: sitter, ngraham, cfeck
Cc: hallas, anthonyfieroni, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, 
GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, mikesomov

D27152: Introduce FilesystemEntry class

2020-02-18 Thread David Hallas

hallas updated this revision to Diff 75961.
hallas marked 17 inline comments as done.
hallas added a comment.


  Updated patch with review comments

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27152?vs=74977=75961

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  autotests/filesystem_type_test.cpp
  autotests/filesystem_type_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/filesystem_type.cpp
  src/solid/devices/backends/fstab/filesystem_type.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-02-04 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> meven wrote in filesystem_entry.cpp:47
> It would be great to expose an enum (KIO's KFileSystemType does this a 
> little).
> But given the number of filesystems this days, it may be overcomplicated.

Yeah I agree ;) But I would really like for this class to be used more 
generically so having a list of filesystems here would probably make it hard 
for that. We could also have two methods, one similar to this one given the 
"raw" filesystem type, and one that would give an enum entry from a so called 
"known" list, but would return "unknown" for a filesystem type not in the list, 
then you would have access to both? What do you think?

> meven wrote in filesystem_entry.h:88
> Is it necessary to keep if it is  commented out ?

No, I only put them here for completeness sake. Basically I sat down with the 
fstab man page and the header file documenting the mntent structure and based 
this class around that, but if this code should be exported an made generally 
available then we should probably add these, what do you think? Should I remove 
them now, and then we can always add them if needed?

REPOSITORY
  R245 Solid

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

2020-02-03 Thread David Hallas

hallas created this revision.
hallas added reviewers: Frameworks, bruns, meven.
hallas added a project: Frameworks.
hallas requested review of this revision.

REVISION SUMMARY
  Introdue a dedicated type for representing a FilesystemEntry. A
  FilesystemEntry is either mounted by the system or can be mounted by the
  system. It is created from the information found in filesystem table
  files, this can be fstab, mtab or any other mtab type file.
  
  This commit is a small part of a larger fstab refactor patch which can
  bee seen here D26600 

TEST PLAN
  Unit tests
  Tested manually with Plasma Vaults

REPOSITORY
  R245 Solid

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26600: Refactor fstab handling

2020-01-18 Thread David Hallas

hallas added a comment.


  In D26600#596631 , @bruns wrote:
  
  > and this is definitely too much code being moved around. Please split this 
up into multiple reviews.
  >
  > You should likely start with the introduction of the FilesystemEntry class, 
use that from the existing code.
  
  
  Ok I can try ;) But it will take some time to do so.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D26600

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26600: Refactor fstab handling

2020-01-18 Thread David Hallas

hallas added a comment.


  In D26600#596533 , @bruns wrote:
  
  > Moving existing code to new files does not make you the copyright owner.
  
  
  Yes, sorry about that - I need to fix that. This is just my editor, when I 
create new files this is the default template. I will make sure to put all the 
original authors in.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D26600

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2020-01-18 Thread David Hallas

hallas added a comment.


  I have refactored the fstab handling to make supporting fuseiso _much_ 
simpler - so please take a look at D26600  
- once that is merged I will push a new review of this patch.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26600: Refator fstab handling

2020-01-11 Thread David Hallas

hallas added a comment.


  @meven - I am a little unsure if I have broken the fix you have done in 
commit c97f0b2a3076731b35435f200bd09a22859f3e03 
<https://phabricator.kde.org/R245:c97f0b2a3076731b35435f200bd09a22859f3e03> - 
could you please check?
  
  I have not tested with NFS or SMB mounts.
  
  Finally, I think this code could be moved to a more general library in KDE 
Frameworks, because it appears that we have this functionality in multiple 
places. We have at least a partial copy of this in 
solid/src/solid/devices/backends/hal and probably also other places.

INLINE COMMENTS

> call_system_command.cpp:1
> +/***
> + *   Copyright (C) 2019 by David Hallas  *

I think the Copyright notice needs to include the original authors of this 
function

> filesystem_table_parser.cpp:2
> +/***
> + *   Copyright (C) 2019 by David Hallas  *
> + * *

I think the Copyright notice needs to include the original authors of this 
function

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D26600

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26600: Refator fstab handling

2020-01-11 Thread David Hallas

hallas created this revision.
hallas added reviewers: Frameworks, bruns, meven.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
hallas requested review of this revision.

REVISION SUMMARY
  Refactor fstab handling
  
  Splits the fstab/mtab handling code into smaller pieces and allow unit
  testing of each one. The reafacting allows us to use the various classes
  individually, e.g. use the FilesystemTableParser for parsing any
  fstab/mtab style file.
  
  This refactoring is a prerequisite for adding proper support for
  fuseiso, since fuseiso maintains it's own mtab file, which we would like
  to parse and use.

TEST PLAN
  Unit tests
  Tested manually with Plasma Vaults

REPOSITORY
  R245 Solid

BRANCH
  fstabhandling_rewrite

REVISION DETAIL
  https://phabricator.kde.org/D26600

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  autotests/filesystem_table_parser_test.cpp
  autotests/filesystem_table_parser_test.h
  autotests/system_filesystem_table_test.cpp
  autotests/system_filesystem_table_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/call_system_command.cpp
  src/solid/devices/backends/fstab/call_system_command.h
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/filesystem_table_parser.cpp
  src/solid/devices/backends/fstab/filesystem_table_parser.h
  src/solid/devices/backends/fstab/filesystem_table_watcher.cpp
  src/solid/devices/backends/fstab/filesystem_table_watcher.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h
  src/solid/devices/backends/fstab/fstabmanager.cpp
  src/solid/devices/backends/fstab/fstabmanager.h
  src/solid/devices/backends/fstab/fstabstorageaccess.cpp
  src/solid/devices/backends/fstab/fstabstorageaccess.h
  src/solid/devices/backends/fstab/fstabwatcher.cpp
  src/solid/devices/backends/fstab/fstabwatcher.h
  src/solid/devices/backends/fstab/system_filesystem_table.cpp
  src/solid/devices/backends/fstab/system_filesystem_table.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-12-15 Thread David Hallas

hallas added a comment.


  Sorry for breaking the build :/
  
  @kossebau  - thanks for fixing it so quickly! I think your fix looks fine :D

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23205

To: hallas, davidedmundson, broulik, mpyne
Cc: kossebau, bcooksley, mpyne, apol, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-12-14 Thread David Hallas

hallas closed this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23205

To: hallas, davidedmundson, broulik, mpyne
Cc: mpyne, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-11-16 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:301
> The API is totally awkward.
> 
> You end up with something significantly cleaner when you create a new class 
> for it:
> 
>   QStringMultiHash m_mtabCache;
>   QHash m_mtabFstypeCache;
>   bool m_mtabCacheValid;
> 
> and make the parser a member function.
> 
> Then instantiate the class for the fuseiso case and for the globalFstabCache.

Yeah, I agree - it isn't the prettiest of code :D My main reasoning for doing 
it like this was to make the change as small as possible. But, let me take a 
stab at refactoring it a bit and push a new commit.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-11-14 Thread David Hallas

hallas added a comment.


  @davidedmundson - ping ;)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23205

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-11-14 Thread David Hallas

hallas added a comment.


  @bruns  - ping ;)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25125: Use KListOpenFilesJob for retrieving apps blocking unmount

2019-11-03 Thread David Hallas

hallas added a comment.

  In D25125#558043 , @anthonyfieroni 
wrote:

  > Set QT_PLUGIN_PATH to you local path with plugin after that set the system 
path for others
  >  `QT_PLUGIN_PATH=/local/path:/system/path executable`

  Thanks for the info :) But what executable would load the plugins? Would I 
have to set the `QT_PLUGIN_PATH` globally on my system, and use my installed 
kde to test? Or do we have a command line tool to load these plugins?

INLINE COMMENTS

> anthonyfieroni wrote in ksolidnotify.h:50
> This is public signal, are you sure none use it?

I think ;) As far as I can tell this is an internal class so it's API is not 
exported. I have also done a `git grep` in the plasma-workspace repository for 
any users, but I have found none.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25125

To: hallas, #frameworks, broulik, bruns
Cc: anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25125: Use KListOpenFilesJob for retrieving apps blocking unmount

2019-11-03 Thread David Hallas

hallas added a comment.


  Note that I haven't tested this patch locally because I don't know how to 
"run" the locally compiled devicenotifier. I can see that I get a 
`plasma_engine_devicenotifications.so` library from building the project, but I 
don't know how to "run" it, do we have a cli tool to load the shared library 
for testing? Or how do you guys test?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25125

To: hallas, #frameworks, broulik, bruns
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25125: Use KListOpenFilesJob for retrieving apps blocking unmount

2019-11-03 Thread David Hallas

hallas created this revision.
hallas added reviewers: Frameworks, broulik, bruns.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
hallas requested review of this revision.

REVISION SUMMARY
  Use the KListOpenFilesJob class from KCoreAddons to retrieve the
  applications that are blocking an unmount. The KListOpenFilesJob is a
  generalized version of the code here.

TEST PLAN
  Insert a USB stick
  Open a file from the USB stick
  Remove the USB stick using the Device Notifier
  Notice the error message

REPOSITORY
  R120 Plasma Workspace

BRANCH
  use_klistopenfilesjob (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25125

AFFECTED FILES
  dataengines/devicenotifications/CMakeLists.txt
  dataengines/devicenotifications/ksolidnotify.cpp
  dataengines/devicenotifications/ksolidnotify.h

To: hallas, #frameworks, broulik, bruns
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D21235: Add handling of fuseiso filesystem type

2019-11-03 Thread David Hallas

hallas added a comment.


  @bruns - ping :) I have updated this patch with the changes you requested, I 
hope you are ok with it now.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-27 Thread David Hallas

hallas marked an inline comment as done.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-21 Thread David Hallas

hallas updated this revision to Diff 68472.
hallas added a comment.


  Reorder functions to make diff smaller

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21235?vs=68324=68472

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-21 Thread David Hallas

hallas marked an inline comment as done.
hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:301
> Please keep these functions at their current position, just adds unnecessary 
> noise in the diff.

Good point :)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-20 Thread David Hallas

hallas marked 3 inline comments as done.
hallas added a comment.


  @bruns - I have now refactored the patch so that it uses the `getmntent` 
functions for parsing the mtab file, so I think this patch is pretty much ready 
for a serious review ;)
  
  Also - please take a look at the strings that the end user sees, I am not 
sure if they are optimal.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-20 Thread David Hallas

hallas updated this revision to Diff 68324.
hallas added a comment.


  Rewrite to use the getmntent function for parsing the mtab file

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21235?vs=68230=68324

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-18 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabdevice.cpp:56
> fuseiso itself uses setmntent/addmntent/endmntent, so I see no reason 
> getmntent should not work.
> Most importantly, the same implementation should be used for writing and 
> reading.
> 
> https://sourceforge.net/p/fuseiso/code/HEAD/tree/trunk/src/fuseiso.c#l112

Yes i know :) But when I played around with the existing code in 
`fstabhandling.cpp` for reading out the fsname it kept being empty. But now I 
just wrote my own little test program that dumps every field in the `mntent` 
struct and there the fsname contains the full path to the iso file. So, I must 
have screwed up when fiddling with the `fstabhandling.cpp` code, let me give it 
another go, I think it would be much nicer to reuse that code then to have our 
own parser :)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-18 Thread David Hallas

hallas added a comment.


  Currently you have the 'unmount' action if you right click on the device in 
dolphin, but it cannot unmount, so should we hide it? Or should we fix it so 
that it can actually unmount?

INLINE COMMENTS

> fstabdevice.cpp:56
> +
> +QString GetFilePathFromFuseIsoMtabFile(const QString& mountPath)
> +{

This implements a crude parser of the fuseiso mtab file. I can't use the 
regular getmnt suite of functions to retrieve the path to the iso file, so that 
is why I implemented this :/

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-18 Thread David Hallas

hallas updated this revision to Diff 68230.
hallas added a comment.


  Implemented parsing of the fuseiso mtab file

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21235?vs=58147=68230

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-10-18 Thread David Hallas

hallas added a comment.


  @davidedmundson  ping :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23205

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24434: Fix include path to kjob.h

2019-10-06 Thread David Hallas

hallas added a comment.


  In D24434#542440 , @dfaure wrote:
  
  > Argh, I retagged, but this wasn't pushed yet.
  >  Landing and retagging again...
  
  
  Thanks a lot and sorry for the screwups :D

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24434

To: hallas, dfaure, meven, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24434: Fix include path to kjob.h

2019-10-05 Thread David Hallas

hallas added a comment.


  @dfaure  - I just found that if you include `KListOpenFilesJob` from e.g. 
Dolphin then it fails because it cannot include `jobs/kjob.h` :(
  
  It would be really nice to have this commit in the coming Frameworks release, 
do you think that is possible?
  
  Sorry for all these late fixes :/

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24434

To: hallas, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24434: Fix include path to kjob.h

2019-10-05 Thread David Hallas

hallas created this revision.
hallas added reviewers: dfaure, meven.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
hallas requested review of this revision.

REVISION SUMMARY
  Fix include path to kjob.h. When kcoreaddons is installed all the
  headers files are installed in the same directory, so we shouldn't use
  jobs/ when including files from public header files.

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24434

AFFECTED FILES
  src/lib/util/klistopenfilesjob.h

To: hallas, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24389: Fix klistopenfilesjob header file not being installed

2019-10-05 Thread David Hallas

hallas added a comment.


  In D24389#541644 , @dfaure wrote:
  
  > `git tag` doesn't show any 5.63.0-rc* tag yet, so it hasn't been tagged, so 
this commit will be included.
  >
  > (I do that on the first Saturday of the month)
  
  
  Phew :) Glad that this one made it

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24389

To: hallas, dfaure, meven, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D24389: Fix klistopenfilesjob header file not being installed

2019-10-03 Thread David Hallas

hallas added a comment.


  In D24389#541539 , @ngraham wrote:
  
  > Please land this ASAP so it gets into Frameworks 5.63.
  
  
  Landed it now, is there anything else I need to do so that it makes the 5.63 
release?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24389

To: hallas, dfaure, meven, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D24389: Fix klistopenfilesjob header file not being installed

2019-10-03 Thread David Hallas

hallas closed this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24389

To: hallas, dfaure, meven, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D24389: Fix klistopenfilesjob header file not being installed

2019-10-03 Thread David Hallas

hallas created this revision.
hallas added reviewers: dfaure, meven.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
hallas requested review of this revision.

REVISION SUMMARY
  The header file KListOpenFilesJob and klistopenfilesjob.h was not being
  installed

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24389

AFFECTED FILES
  src/lib/CMakeLists.txt

To: hallas, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-10-01 Thread David Hallas

hallas added a comment.


  In D21235#536576 , @hallas wrote:
  
  > In D21235#535951 , @bruns wrote:
  >
  > > In D21235#535861 , @hallas 
wrote:
  > >
  > > > I have been resurrecting this patch again :) and have run into an issue 
I need some guidance on. To be able to parse the `~/.mtab.fuseiso` file I would 
like to use the `KMountPoint` class, but this class currently resides in KIO 
which Solid doesn't depend on. But, KIO actually depends on Solid so would it 
be an option to move this class from KIO to Solid?
  > >
  > >
  > > What do you gain by using KMountPoints? Solid already has code for 
parsing fstab style files.
  >
  >
  > Hi @bruns , no you are right that I should be able to do with the code that 
is in Solid already. It just needs to be updated to be able parse an arbitrary 
mtab file. But anyhow, the fstab/mtab code in Solid seems kind of duplicated 
with the code in KIO, so we might consolidate at some point.
  >
  > But let me look into the mtab code in Solid and see if I can make it work 
there :)
  
  
  Ok, I have been playing around with the code and using getmntent for parsing 
the `~/.mtab.fuseiso` file, but the getmntent function doesn't give me the 
first field, which is the path to the iso (this is the information I am looking 
for). So currently I am looking at just reading in the file, splitting the 
space separated lines and extracting the two first fields. If anyone has ideas 
to do this in a smarter way, please let me know ;) I will post an updated 
review once I have some working code.

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-09-23 Thread David Hallas

hallas added a comment.


  In D21235#535951 , @bruns wrote:
  
  > In D21235#535861 , @hallas wrote:
  >
  > > I have been resurrecting this patch again :) and have run into an issue I 
need some guidance on. To be able to parse the `~/.mtab.fuseiso` file I would 
like to use the `KMountPoint` class, but this class currently resides in KIO 
which Solid doesn't depend on. But, KIO actually depends on Solid so would it 
be an option to move this class from KIO to Solid?
  >
  >
  > What do you gain by using KMountPoints? Solid already has code for parsing 
fstab style files.
  
  
  Hi @bruns , no you are right that I should be able to do with the code that 
is in Solid already. It just needs to be updated to be able parse an arbitrary 
mtab file. But anyhow, the fstab/mtab code in Solid seems kind of duplicated 
with the code in KIO, so we might consolidate at some point.
  
  But let me look into the mtab code in Solid and see if I can make it work 
there :)

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-09-22 Thread David Hallas

hallas added a comment.


  I have been resurrecting this patch again :) and have run into an issue I 
need some guidance on. To be able to parse the `~/.mtab.fuseiso` file I would 
like to use the `KMountPoint` class, but this class currently resides in KIO 
which Solid doesn't depend on. But, KIO actually depends on Solid so would it 
be an option to move this class from KIO to Solid?

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-09-19 Thread David Hallas

hallas added a comment.


  @davidedmundson - ping :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23205

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed

2019-09-16 Thread David Hallas

hallas closed this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23884

To: hallas, dfaure
Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed

2019-09-12 Thread David Hallas

hallas marked 2 inline comments as done.
hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfilesjobtest_unix.cpp:67
> Please use QSKIP instead, otherwise the test output looks like the test 
> passed, rather than was skipped.

Good point, I didn't know that :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23884

To: hallas, dfaure
Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed

2019-09-12 Thread David Hallas

hallas updated this revision to Diff 65937.
hallas added a comment.


  Use QSKIP to skip tests

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23884?vs=65870=65937

BRANCH
  fix_unittest_if_lsof_is_not_installed

REVISION DETAIL
  https://phabricator.kde.org/D23884

AFFECTED FILES
  autotests/klistopenfilesjobtest_unix.cpp

To: hallas, dfaure
Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed

2019-09-11 Thread David Hallas

hallas updated this revision to Diff 65870.
hallas added a comment.


  Use QStandardPaths::findExecutable to locate lsof

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23884?vs=65869=65870

BRANCH
  fix_unittest_if_lsof_is_not_installed

REVISION DETAIL
  https://phabricator.kde.org/D23884

AFFECTED FILES
  autotests/klistopenfilesjobtest_unix.cpp

To: hallas, dfaure
Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed

2019-09-11 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> dhaumann wrote in klistopenfilesjobtest_unix.cpp:34
> Can't you simply use QStandardPaths::findExecutable()?
> 
> https://doc.qt.io/qt-5/qstandardpaths.html#findExecutable

Yes, this seems to do exactly what we need :)

> ngraham wrote in klistopenfilesjobtest_unix.cpp:37
> A prior career in build engineering tells me that this will start failing in 
> 10 years when `lsof` removes the `-v` argument and replaces it with 
> `--version`. For robustness' sake, I would additionally check for `--version` 
> (and possibly even `-version` too) if the initial `lsof -v` call fails.

I have changed the code to use QStandardPaths::findExecutable instead

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23884

To: hallas, dfaure
Cc: dhaumann, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23884: Fix KListOpenFilesJob unit test on Unix if lsof is not installed

2019-09-11 Thread David Hallas

hallas created this revision.
hallas added a reviewer: dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
hallas requested review of this revision.

REVISION SUMMARY
  The KListOpenFilesJob unit test on Unix expected lsof to be
  installed, but the KDE build infrastructure doesn't have it installed,
  so check if it is installed and skip the tests if it is not.

TEST PLAN
  Unit Test

REPOSITORY
  R244 KCoreAddons

BRANCH
  fix_unittest_if_lsof_is_not_installed

REVISION DETAIL
  https://phabricator.kde.org/D23884

AFFECTED FILES
  autotests/klistopenfilesjobtest_unix.cpp

To: hallas, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21760: Add KListOpenFilesJob

2019-09-11 Thread David Hallas

hallas closed this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-11 Thread David Hallas

hallas updated this revision to Diff 65856.
hallas added a comment.


  Updated @since

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65500=65856

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilesjobtest_unix.cpp
  autotests/klistopenfilesjobtest_unix.h
  autotests/klistopenfilesjobtest_win.cpp
  autotests/klistopenfilesjobtest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfilesjob.h
  src/lib/util/klistopenfilesjob_unix.cpp
  src/lib/util/klistopenfilesjob_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-07 Thread David Hallas

hallas added a comment.


  In D21760#526652 , @dfaure wrote:
  
  > This is good to go in :-)
  >
  > If you push it today it'll indeed be in 5.62, to be tagged tomorrow, 
otherwise we'll need to adjust the @since tag ;-)
  
  
  I think i prefer waiting to after the release so it has a chance to get some 
mileage before release :) I will make sure to update the @since tag.

REPOSITORY
  R244 KCoreAddons

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-06 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfilesjobtest_unix.cpp:34
> (minor) we never check that `new` succeeded, in Qt code.
> 
> The reasoning is that on desktop systems, with swap enabled, before the swap 
> is exhausted, the user will have given up and rebooted the machine anyway. I 
> know I do, many times a month :). So in practice, `new` can be considered to 
> always succeed.

Makes sense, this was a leftover from when the job was returned by a function 
instead of new-ing it directly :) Another reason for not checking is that new 
doesn't return nullptr on failure, instead it throws std::bad_alloc, to get 
nullptr on failure you need to use the nothrow version of new :)

> dfaure wrote in klistopenfilesjob.h:39
> [is it useful to repeat that sentence? I admit I'm no expert on Doxygen's 
> @brief command]

I don't think it is useful, so I have remove it

> dfaure wrote in klistopenfilesjob_win.cpp:26
> Won't be needed anymore once the getter returns by value :-)

Remove :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-06 Thread David Hallas

hallas edited the summary of this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-06 Thread David Hallas

hallas updated this revision to Diff 65500.
hallas marked 7 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65494=65500

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilesjobtest_unix.cpp
  autotests/klistopenfilesjobtest_unix.h
  autotests/klistopenfilesjobtest_win.cpp
  autotests/klistopenfilesjobtest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfilesjob.h
  src/lib/util/klistopenfilesjob_unix.cpp
  src/lib/util/klistopenfilesjob_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-05 Thread David Hallas

hallas updated this revision to Diff 65494.
hallas added a comment.


  Review comments, renamed the files to match the class name

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65330=65494

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilesjobtest_unix.cpp
  autotests/klistopenfilesjobtest_unix.h
  autotests/klistopenfilesjobtest_win.cpp
  autotests/klistopenfilesjobtest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfilesjob.h
  src/lib/util/klistopenfilesjob_unix.cpp
  src/lib/util/klistopenfilesjob_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-05 Thread David Hallas

hallas marked an inline comment as done.
hallas added a comment.


  In D21760#525842 , @dfaure wrote:
  
  > Yes, the filenames should match the classname, obviously :)
  >
  > I did a deeper review of the KJob usage and I have two more comments, sorry 
for not taking the time to do this earlier...
  
  
  No problem ;) I would rather have this change go in slow and correct than 
quick and dirty :)

INLINE COMMENTS

> dfaure wrote in klistopenfiles_unix.cpp:57
> Emitting `result` twice would be a very bad idea, a KJob can only emit 
> `result` once.
> 
> But QProcess can emit `errorOccurred` followed by `finished` (e.g. if the 
> subprocess crashes, from what I can see in qprocess.cpp).
> I think we want to only qWarning() in this method, and let `lsofFinished` 
> take care of finishing the job.

Good catch! Actually it turns out to be a tad more complicated ;) QProcess will 
only emit finished if the process actually started, so if you have the case 
where lsof is not installed you will only get the errorOccurred signal and not 
finished, but if it crashes you get both. I actually added a unit test to test 
the first case (where lsof is not found), and I ended up making a solution 
where we simply remember if we have emitted the result and only emit it if we 
haven't done so before.

> dfaure wrote in klistopenfiles_unix.cpp:73
> The class would be slightly easier to use if this was a getter instead of a 
> signal.
> 
> Because then, in a unittest, you can just do `job->exec()` and then 
> `job->processInfoList()`, no spy needed.
> In (GUI) application code you still need to connect to a signal, but then the 
> usual KJob::result signal is enough, instead of two signals 
> (`processInfoAvailable` in case of success, and `result` to handle failure).
> 
> See `KIO::StatJob::statResult()` for an example.
> 
> Signals emitted by jobs are useful if they happen during the job lifetime 
> (e.g. progress signals). If they happen at the same time as `result`, then 
> it's easier to use `result` as the notification signal.
> 
> The downside is one more member variable, but it'll be very short-lived so I 
> wouldn't worry about memory usage.

Agree, I have refactored the code to do this and it is **much** simpler :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-03 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> klistopenfiles.h:23
> +
> +#ifndef KLISTOPENFILES_H
> +#define KLISTOPENFILES_H

Should these files be renamed to klistopenfilesjob (along with tests etc.) now 
that is what the class is called?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-03 Thread David Hallas

hallas added a comment.


  In D21760#524860 , @dfaure wrote:
  
  > Given that the namespace doesn't contain anything else anymore, I would 
just get rid of it, and provide a single class, KListOpenFilesJob.
  
  
  Done :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-03 Thread David Hallas

hallas updated this revision to Diff 65330.
hallas added a comment.


  Removed KListOpenFiles namespace and renamed ListOpenFilesJob to 
KListOpenFilesJob

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65284=65330

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest_unix.cpp
  autotests/klistopenfilestest_unix.h
  autotests/klistopenfilestest_win.cpp
  autotests/klistopenfilestest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.h
  src/lib/util/klistopenfiles_unix.cpp
  src/lib/util/klistopenfiles_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

2019-09-03 Thread David Hallas

hallas retitled this revision from "Add KListOpenFiles::ListOpenFilesJob" to 
"Add KListOpenFilesJob".
hallas edited the summary of this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::ListOpenFilesJob

2019-09-02 Thread David Hallas

hallas retitled this revision from "Add 
KListOpenFiles::listProcessesWithOpenFiles" to "Add 
KListOpenFiles::ListOpenFilesJob".
hallas edited the summary of this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-09-02 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfilestest_unix.cpp:83
> I usually just use "/does/not/exist" as a path ;-)
> 
> This might actually be better because Windows has weird race conditions with 
> the filesystem stuff.

This test is unix only anyway so there shouldn't be any race conditions. But it 
might be a little more expressive to use "/does/not/exist" so I have changed it 
anyway ;)

> dfaure wrote in klistopenfiles.h:94
> KIO is designed like this for some reason, but I would just document the job 
> constructor and let people create the job with new, outside KIO.
> 
> That's e.g. what akonadi does.

Would it make sense to rename the ListOpenFilesJob class to simply Job? It is 
already inside a KListOpenFiles namespace so the current name seems a little 
long and duplicated? Do we have any general naming convention for these things?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-09-02 Thread David Hallas

hallas updated this revision to Diff 65284.
hallas marked 3 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65237=65284

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest_unix.cpp
  autotests/klistopenfilestest_unix.h
  autotests/klistopenfilestest_win.cpp
  autotests/klistopenfilestest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.h
  src/lib/util/klistopenfiles_unix.cpp
  src/lib/util/klistopenfiles_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-09-02 Thread David Hallas

hallas added a comment.


  I have added a minimal Windows implementation which always emits an error, 
along with a unit test. Please review it thoroughly and then I think it is 
ready to land :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-09-02 Thread David Hallas

hallas updated this revision to Diff 65237.
hallas marked 2 inline comments as done.
hallas added a comment.


  Review comments. Added minimal Windows implementation which basically always 
reports failure with the error code Unsupported.

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=64973=65237

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest_unix.cpp
  autotests/klistopenfilestest_unix.h
  autotests/klistopenfilestest_win.cpp
  autotests/klistopenfilestest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.h
  src/lib/util/klistopenfiles_unix.cpp
  src/lib/util/klistopenfiles_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-29 Thread David Hallas

hallas added a comment.


  One thing, when this is ready to land I will address the Windows support so 
that we do not get broken builds :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-29 Thread David Hallas

hallas updated this revision to Diff 64973.
hallas marked 2 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=64911=64973

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest.cpp
  autotests/klistopenfilestest.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.cpp
  src/lib/util/klistopenfiles.h

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-29 Thread David Hallas

hallas added a comment.


  @dfaure  - Overall, what do you think about the approach of subclassing KJob? 
Did it turn out like you had thought? And is this the solution we should go 
with, or was one of the other solutions better?

INLINE COMMENTS

> dfaure wrote in klistopenfiles.cpp:29
> I wonder if this actually needs to be a QObject, given that you use 
> connect-to-pointer-to-member-function?

The primary motivation was memory-management, but I could also just make the 
d-pointer in ListOpenFilesJob a QScopedPointer, would that be better?

> dfaure wrote in klistopenfiles.cpp:44
> KIO::ERR_DOES_NOT_EXIST would fit well here, but that's in kio...
> 
> I suggest adding enum { ERR_DOES_NOT_EXIST = KJob::UserDefinedError + 11 }
> in the header file for this job and using that here.
> 
> And yes, you should do something like
> 
>   setErrorText(i18n("Directory does not exist: %1", path));

I have added the error enum entries, but I am a little hesitant to add to error 
texts since KCoreAddons doesn't today depend on KDE::I18n and this would 
introduce that dependency, do we really want that? When I look at the KIO 
classes then they usually set som non-translated string as the error text, 
would that be an option?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-08-28 Thread David Hallas

hallas edited the test plan for this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23205

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-08-28 Thread David Hallas

hallas updated this revision to Diff 64914.
hallas added a comment.


  Add bug reference

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23205?vs=64913=64914

BRANCH
  optimize_kprocesslist_processinfo

REVISION DETAIL
  https://phabricator.kde.org/D23205

AFFECTED FILES
  src/lib/util/kprocesslist.cpp
  src/lib/util/kprocesslist_unix.cpp
  src/lib/util/kprocesslist_win.cpp

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-08-28 Thread David Hallas

hallas marked an inline comment as done.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23205

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-08-28 Thread David Hallas

hallas updated this revision to Diff 64913.
hallas marked an inline comment as done.
hallas added a comment.


  Fixed review comments, rebased.

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23205?vs=63873=64913

BRANCH
  optimize_kprocesslist_processinfo

REVISION DETAIL
  https://phabricator.kde.org/D23205

AFFECTED FILES
  src/lib/util/kprocesslist.cpp
  src/lib/util/kprocesslist_unix.cpp
  src/lib/util/kprocesslist_win.cpp

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-28 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> klistopenfiles.cpp:44
> +if (!path.exists()) {
> +job->setError(KJob::UserDefinedError);
> +job->emitResult();

Should I also set an error string here? And what about defining custom error 
codes, is that how we usually do?

> klistopenfiles.h:37
> +
> +class KCOREADDONS_EXPORT ListOpenFilesJob : public KJob
> +{

I am a little unsure if there is any other KJob functions that should be 
overwritten in this class? The current implementation is quite minimal, but 
maybe that is fine?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-28 Thread David Hallas

hallas added a comment.


  @meven , so I finally managed to rewrite this patch to use KJob instead. 
Please take a look at it again and see if this is better approach :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-28 Thread David Hallas

hallas updated this revision to Diff 64911.
hallas added a comment.


  Rewrote the code to use KJob

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=59636=64911

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest.cpp
  autotests/klistopenfilestest.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.cpp
  src/lib/util/klistopenfiles.h

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-17 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> meven wrote in klistopenfiles.cpp:39
> The qAsConst wrapping is missing it seems.

Yeah, I have only done this change locally ;)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23214: Add a BlockinApp util class to find process blockng access

2019-08-17 Thread David Hallas

hallas added a comment.


  In D23214#513408 , @meven wrote:
  
  > In D23214#513396 , @hallas wrote:
  >
  > > Hey @meven I have been working on the same thing D21760 
 - maybe we should consolidate our efforts? 
The code you have written looks very similar to what I have been doing :) As 
you can read in the review comments for D21760 
 the current suggestion is to look into 
doing a KJob subclass.
  >
  >
  > Great suggestion I missed this prior diff.
  >
  > Abandoned in favor of D21760  and 
adding a KJob for this use case
  
  
  I am currently working on re-working the code to derive from KJob, will post 
an updated patch soon :) Also, please take a look at the code and come with 
review comments/suggestions :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23214

To: meven, davidedmundson, #frameworks
Cc: hallas, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23214: Add a BlockinApp util class to find process blockng access

2019-08-17 Thread David Hallas

hallas added a comment.


  Hey @meven I have been working on the same thing D21760 
 - maybe we should consolidate our efforts? 
The code you have written looks very similar to what I have been doing :) As 
you can read in the review comments for D21760 
 the current suggestion is to look into 
doing a KJob subclass.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D23214

To: meven, davidedmundson, #frameworks
Cc: hallas, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

2019-08-16 Thread David Hallas

hallas created this revision.
hallas added reviewers: davidedmundson, broulik.
Herald added a project: Frameworks.
hallas requested review of this revision.

REVISION SUMMARY
  Optimize KProcessList::processInfo on unix so that it doesn't iterate over all
  processes and then filter the list to the requested process. Instead refactor
  the code that fetches process info from a single process and use that 
function.

TEST PLAN
  Unit Test

REPOSITORY
  R244 KCoreAddons

BRANCH
  optimize_kprocesslist_processinfo

REVISION DETAIL
  https://phabricator.kde.org/D23205

AFFECTED FILES
  src/lib/util/kprocesslist.cpp
  src/lib/util/kprocesslist_unix.cpp
  src/lib/util/kprocesslist_win.cpp

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-13 Thread David Hallas

hallas marked 3 inline comments as done.
hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfiles.cpp:49
> This reminds me of KCoreAddons' new KProcessInfo class, although that one 
> doesn't actually do this.
> 
> I see that FreeBSD and OSX have lsof (and the man page doesn't say those 
> options are missing there) so it should be fine, everywhere except Windows.
> 
> In general I don't really like starting executables like this, but indeed 
> writing a clone of lsof sounds like much work.

I don't like starting executables either, it is slow, brittle and an implicit 
dependency - but I also think in this case it is the only feasible option :/

But is it ok not to support Windows? Or maybe the Windows implementation would 
emit a debug warning and then always emit an empty list?

> dfaure wrote in klistopenfiles.h:40
> Reading the unittest, I definitely agree that a signal would be better. Then 
> you could use QSignalSpy::wait() without the need to use QEventLoop by hand 
> :-)
> 
> This is really a job. I would inherit KJob for this, actually. It's the way 
> we do such things everywhere else.

Thanks for the feedback! I will look into making a solution using a signal 
instead and push an updated commit.

Regarding implementing this as a KJob subclass, do you have a good example of 
this? Would the result be delivered in a special signal, or is there a standard 
mechanism for this in KJob?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-08-13 Thread David Hallas

hallas added a subscriber: cfeck.
hallas added a comment.


  In D21760#493817 , @cfeck wrote:
  
  > You wrote "ported from Device Notifier". I haven't check in detail yet, but 
do other copyright holders need to be mentioned?
  
  
  I have added the copyright holders from:
  plasma-workspace/dataengines/devicenotifications/ksolidnotify.cpp

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-06-27 Thread David Hallas

hallas added a comment.


  Ping :D

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-06-12 Thread David Hallas

hallas added a comment.


  First of, this is WIP, I just wanted to share this early to get some 
feedback. The reasoning for this is to generalize functionality for running 
`lsof`, this is currently in use by the Device Notifier applet and I would like 
to use it in Dolphin to fix bug #189302. Also see the discussion in D19989 
 for details.

INLINE COMMENTS

> klistopenfilestest.cpp:31
> +
> +void KListOpenFilesTest::testOpenFiles()
> +{

Most of these tests wont work on Windows, so I need to fix that

> klistopenfiles.cpp:49
> +});
> +p->start(QStringLiteral("lsof"), {QStringLiteral("-t"), 
> QStringLiteral("+d"), path.path()});
> +}

What should we do for Windows? Do we support any other platforms that doesn't 
have lsof?

> klistopenfiles.h:40
> + */
> +KCOREADDONS_EXPORT void listProcessesWithOpenFiles(QDir path, Callback 
> callback);
> +

I am very much in doubt about what a nice interface for this should be? I was 
thinking of an alternative approach where you would instantiate a class, 
connect to a signal on the class and then invoke a method to start the listing 
process, you would then receive the result on a slot instead. What do you guys 
think?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D21760

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

2019-06-12 Thread David Hallas

hallas created this revision.
hallas added reviewers: davidedmundson, broulik.
hallas added a project: Frameworks.
hallas requested review of this revision.

REVISION SUMMARY
  Add KListOpenFiles::listProcessesWithOpenFiles for retrieving the list
  of processes that has files open in the given path or a subdirectory of
  path. This code is ported from Device Notifier

TEST PLAN
  Unit Tests

REPOSITORY
  R244 KCoreAddons

BRANCH
  add_list_processes_with_open_files (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest.cpp
  autotests/klistopenfilestest.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.cpp
  src/lib/util/klistopenfiles.h

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-06-01 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabdevice.cpp:88
> By default fuseiso seems to maintain also a file "~/.mtab.fuseiso", there may 
> be more information

Yes it does :D After mounting an iso I have the following contents:

  /home/dha/test.iso /home/dha/mnt1 fuseiso defaults 0 0

So at least we should be able to dig out the iso filename.

So could a better description be something like:

  test.iso on /home/dha/mnt1

or

  test.iso (/home/dha/mnt1)

or maybe some mention that this is a fuseiso mount, i.e.

  test.iso fuseiso mounted on /home/dha/mnt1

I don't know if that becomes way too long

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-05-27 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabdevice.cpp:88
> @hallas:
> cat /proc/mounts

This is what I have in mounts:

  fuseiso on /home/dha/mnt1 type fuse.fuseiso 
(rw,nosuid,nodev,relatime,user_id=1000,group_id=1000)

so there just isn't a whole lot of information - we don't even have the iso 
filename :/

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-05-26 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> broulik wrote in fstabdevice.cpp:88
> Can we maybe give it a better description then? For loop devices we show the 
> name of the backing file as description.

Certainly :D I am also struggling a bit with how to best describe these FUSE 
mounted devices, @ngraham do you have any good suggestions?

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-05-15 Thread David Hallas

hallas added inline comments.

INLINE COMMENTS

> fstabdevice.cpp:99
> +} else if (m_storageType == StorageType::Iso) {
> +m_iconName = QLatin1String("media-optical-data");
>  } else {

I don't know if this is the best icon to use, but I think it should be 
something that the user would associate with a CD since an ISO is essentially a 
CD image.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21235: Add handling of fuseiso filesystem type

2019-05-15 Thread David Hallas

hallas created this revision.
hallas added reviewers: bruns, ngraham.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
hallas requested review of this revision.

REVISION SUMMARY
  Add handling of fuseiso filesystem type.
  When a fuseiso filesystem is mounted it will be shown as a optical data
  media.

TEST PLAN
  Mount an iso using fuseiso
  Start Dolphin and see the filesystem in the places panel

REPOSITORY
  R245 Solid

BRANCH
  add_handling_of_fuseiso (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21235

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp

To: hallas, bruns, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

2019-05-06 Thread David Hallas

hallas added a comment.


  Thanks for the review! Landing it now.

REPOSITORY
  R244 KCoreAddons

BRANCH
  adds_kprocesslist (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D20007

To: hallas, davidedmundson, broulik
Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

2019-05-06 Thread David Hallas

hallas closed this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D20007

To: hallas, davidedmundson, broulik
Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

2019-05-06 Thread David Hallas

hallas added a comment.


  @davidedmundson  - ping ?

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D20007

To: hallas, davidedmundson, broulik
Cc: vonreth, adridg, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D20938: Add Mounts Backend

2019-05-06 Thread David Hallas

hallas added a comment.


  This patch has been superseeded by the wotk @bruns has done in D20995 
 - so closing this one :D

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D20938

To: hallas, #frameworks, ngraham, elvisangelaccio, broulik, bruns
Cc: svuorela, nicolasfella, ivan, kde-frameworks-devel, michaelh, ngraham, bruns

D20938: Add Mounts Backend

2019-05-06 Thread David Hallas

hallas abandoned this revision.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D20938

To: hallas, #frameworks, ngraham, elvisangelaccio, broulik, bruns
Cc: svuorela, nicolasfella, ivan, kde-frameworks-devel, michaelh, ngraham, bruns

1 2 >

1 - 100 of 179 matches

Mail list logo