D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

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

D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[Méven Car]

meven accepted this revision.
meven added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> dfaure wrote in global.h:323
> There are bugs in the current kio_file implementation if StatBasic isn't set.
> 
>   mode_t type = 0;
>   if (details & KIO::StatBasic) {
>   ... code that sets type ...
>   }
>   if (details & KIO::StatAcl) {
>   appendACLAtoms(targetPath, entry, type); // oops type is 0
>   }
> 
> Hmm I thought I saw more, but now I don't see more (must have been fixed 
> meanwhile). If you fix the bug I'm happy to remove the comment, LOL.

I believe it was `StatResolveSymlink`
D28947  for the StatAcl fix

REPOSITORY
  R241 KIO

BRANCH
  2020_optimize_recursive_size

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

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

D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[David Faure]

dfaure updated this revision to Diff 80428.
dfaure added a comment.


  Remove comment change above StatBasic

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28901?vs=80335&id=80428

BRANCH
  2020_optimize_recursive_size

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

AFFECTED FILES
  src/core/global.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/kio_trash.h
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h

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

D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> meven wrote in trashimpl.cpp:1105
> Maybe, check for `details & KIO::StatTime`
> Down the rabbit hole...

Well, if StatRecursiveSize is set, we have the info, so we might as well 
provide it.

If it's not set, then this would mean we need to do the whole recursive thing 
also when StatTime is set, and it's part of StatDefaultDetails so I expect 
we'll do this often for nothing. I think we don't really want to do that, in 
practice we need both pieces of information, or none.

REPOSITORY
  R241 KIO

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

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

D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> meven wrote in global.h:323
> I don't think we should assume that, it defeats quite the purpose.
> For instance even if we the ioslave gets the filename or type for its needs 
> it may just not include it in the UDSEntry if StatBasic was not passed.

There are bugs in the current kio_file implementation if StatBasic isn't set.

  mode_t type = 0;
  if (details & KIO::StatBasic) {
  ... code that sets type ...
  }
  if (details & KIO::StatAcl) {
  appendACLAtoms(targetPath, entry, type); // oops type is 0
  }

Hmm I thought I saw more, but now I don't see more (must have been fixed 
meanwhile). If you fix the bug I'm happy to remove the comment, LOL.

REPOSITORY
  R241 KIO

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

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

D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[Méven Car]

meven requested changes to this revision.
meven added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> global.h:323
>  StatNoDetails = 0x0,
> -/// Filename, access, type, size, linkdest
> +/// Filename, access, type, size, linkdest -- necessary for all others 
> below
>  StatBasic = 0x1,

I don't think we should assume that, it defeats quite the purpose.
For instance even if we the ioslave gets the filename or type for its needs it 
may just not include it in the UDSEntry if StatBasic was not passed.

> meven wrote in trashimpl.cpp:1105
> Maybe, check for `details & KIO::StatTime`
> Down the rabbit hole...

(I don't expect it)

REPOSITORY
  R241 KIO

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

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

D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[Méven Car]

meven accepted this revision.
meven added a comment.
This revision is now accepted and ready to land.


  So we are gonna need a patch to baloo-widget stat, with 
`KIO::StatRecursiveSize`
  A test would nice for trash:/ with `KIO::StatRecursiveSize`

INLINE COMMENTS

> trashimpl.cpp:1105
> -
> -entry.fastInsert(KIO::UDSEntry::UDS_MODIFICATION_TIME, 
> latestModifiedDate / 1000);
> -// access date is unreliable for the trash folder, use the modified date 
> instead

Maybe, check for `details & KIO::StatTime`
Down the rabbit hole...

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

D28901: Add KIO::StatRecursiveSize detail value so kio_trash only does this on demand

[David Faure]

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

REVISION SUMMARY
  This indirectly fixes the testtrash unittest which noticed that the
  directory size cache was being updated when CopyJob stat's the
  destination trash:/.

TEST PLAN
  bin/testtrash

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/core/global.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/kio_trash.h
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure added a comment.


  Yep, that's it. Renaming trash:/A to trash:/B calls KIO::moveAs which stats 
trash:/ [useful to resolve desktop:/ to a local dir for instance], which now 
updates the on-disk cache... with 'A', before it gets renamed to 'B'.
  
  I think I know what the best solution is. Using StatDetails to skip the 
(expensive) size stuff when CopyJob calls stat(). Bugfix and performance 
improvement :)

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, elvisangelaccio, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> trashimpl.cpp:1092
> +TrashSizeCache trashSize(trashPath);
> +TrashSizeCache::SizeAndModTime res = 
> trashSize.calculateSizeAndLatestModDate();
> +size += res.size;

It's actually this call which triggers the unittest failure.

I wonder if maybe this happens at the wrong time

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, elvisangelaccio, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure added a comment.


  TestTrash::renameDirInTrash makes the dir cache invalid (it still lists 
trashDirFromHome). I think because of the optimization that we don't always 
care about the size... but this had the side effect of keeping the cache 
uptodate I wonder if the test is too strict and an old cache isn't better 
than keeping it coherent at extra cost... I'll dig further.

INLINE COMMENTS

> trashimpl.cpp:411
>  
> -const qulonglong pathSize = DiscSpaceUtil::sizeOfPath(origPath);
> -

This was done unconditionally...

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, elvisangelaccio, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Ahmad Samir]

ahmadsamir added a comment.


  Hello. This broke the testtrash unit test 
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.14/45/execution/node/36/log/

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, elvisangelaccio, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:570b48e656c7: kio_trash: Add size, modification, access 
and create date for trash:/ (authored by meven).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D24773?vs=80016&id=80061#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=80016&id=80061

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h
  src/ioslaves/trash/trashsizecache.cpp
  src/ioslaves/trash/trashsizecache.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24773_1

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven marked an inline comment as done.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 80016.
meven marked 2 inline comments as done.
meven added a comment.


  Fix, typo, pass max_mtime by ref to lambda

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=80008&id=80016

BRANCH
  arcpatch-D24773_1

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h
  src/ioslaves/trash/trashsizecache.cpp
  src/ioslaves/trash/trashsizecache.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dfaure wrote in trashsizecache.cpp:131
> Interesting. One benefit of lambdas is that they can work on local variable; 
> I would have captured max_mtime by reference and modified it inside the 
> lambda.
> Written this way (which I guess more "pure functional programming" because no 
> side effects), it could be a static helper function ;)
> 
> Wait, this doesn't work, does it? It makes a copy of mtime right now, while 
> it's still 0.
> 
> I'm pretty sure you want to capture by [&] instead
> (and then, unless you insist on pure functions, I'd suggest just modifying it 
> here, and returning void). It removes the "max_mtime = " duplication ;)

It probably didn't work, thanks

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added a dependent revision: D28794: Allow to display to use 
UDS_RECURSIVE_SIZE in status bar UDS_RECURSIVE_SIZE.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> trashsizecache.cpp:131
> +qint64 max_mtime = 0;
> +const auto checMaxTime = [max_mtime] (const qint64 lastModTime) -> 
> qint64 {
> +return lastModTime > max_mtime ? lastModTime : max_mtime;

Typo: checkMaxTime with a 'k'?

> trashsizecache.cpp:131
> +qint64 max_mtime = 0;
> +const auto checMaxTime = [max_mtime] (const qint64 lastModTime) -> 
> qint64 {
> +return lastModTime > max_mtime ? lastModTime : max_mtime;

Interesting. One benefit of lambdas is that they can work on local variable; I 
would have captured max_mtime by reference and modified it inside the lambda.
Written this way (which I guess more "pure functional programming" because no 
side effects), it could be a static helper function ;)

Wait, this doesn't work, does it? It makes a copy of mtime right now, while 
it's still 0.

I'm pretty sure you want to capture by [&] instead
(and then, unless you insist on pure functions, I'd suggest just modifying it 
here, and returning void). It removes the "max_mtime = " duplication ;)

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added a comment.


  > But I'm wondering how much effort we want to put into finding the latest 
mtime, this is starting to sound like a very expensive operation.
  
  I agree let's not got too deep down the rabbit hole.
  This is not a very common use case
  
  > When kio_trash takes care of the trashing, we should never end up in this 
code path (right?)
  
  AFAIK

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 80008.
meven marked 3 inline comments as done.
meven added a comment.


  Add lambdas to factorize code, comments update, move a couple of 
DiscSpaceUtil::sizeOfPath to where there are needed

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=79943&id=80008

BRANCH
  arcpatch-D24773_1

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h
  src/ioslaves/trash/trashsizecache.cpp
  src/ioslaves/trash/trashsizecache.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> trashimpl.cpp:1098
> +
> +// Find lateest modification date
> +if (res.mtime > latestModifiedDate) {

typo: latest

> trashsizecache.cpp:144
> +if (trashFileInfo.exists() && 
> trashFileInfo.lastModified().toMSecsSinceEpoch() > max_mtime) {
> +max_mtime = 
> trashFileInfo.lastModified().toMSecsSinceEpoch();
> +}

This calls lastModified().toMSecsSinceEpoch() twice.

How about a lambda for (at least)

  if (lastModTime > max_mtime) {
   max_mtime = lastModTime;
  }

and then you can call that lambda with  lastModified().toMSecsSinceEpoch()?
I was thinking about suggesting such a lambda anyway since that logic exists 4 
times in this code.

In fact, checking that the trashinfo file exists is something that should be 
done in all code paths, no? So there's a lot more code that can be factored 
out, I would think.

> trashsizecache.cpp:168
>  if (!usableCache) {
> +// orphaned directories
>  const qulonglong size = 
> DiscSpaceUtil::sizeOfPath(file.absoluteFilePath());

orphaned sounds like "no .trashinfo file pointing to that dir".

But that's not the case here. It's just items that have been added to the trash 
without being added to the cache file (many implementations do that, the cache 
is optional).

So the directory is not listed in the cache, or is listed with a too old mtime 
i.e. it wasn't updated when adding another item into it.

> trashsizecache.cpp:171
>  sum += size;
> -add(fileId, size);
> +long mtime = 
> QFileInfo(file.absolutePath()).lastModified().toMSecsSinceEpoch() ;
> +if (mtime > max_mtime) {

Not technically correct, this is the mtime of the directory, while there could 
be much more recent items inside that directory.

But I'm wondering how much effort we want to put into finding the latest mtime, 
this is starting to sound like a very expensive operation.

So I'm actually find with this known bug, maybe with a comment about how this 
is a heuristic -- in a fallback for bad implementations.
When kio_trash takes care of the trashing, we should never end up in this code 
path (right?)

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 79943.
meven added a comment.


  Improve naming of struct CacheData to SizeAndModTime

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=79942&id=79943

BRANCH
  arcpatch-D24773_1

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h
  src/ioslaves/trash/trashsizecache.cpp
  src/ioslaves/trash/trashsizecache.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 79942.
meven added a comment.
This revision is now accepted and ready to land.


  Improve latest modification time of trash implementation, fix case when a 
symlink is in the trash returning incorrect trash size

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=79813&id=79942

BRANCH
  arcpatch-D24773_1

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h
  src/ioslaves/trash/trashsizecache.cpp
  src/ioslaves/trash/trashsizecache.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  The modification date is not what to expect currently, working on it.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D24773_1

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 79813.
meven marked 4 inline comments as done.
meven added a comment.


  Fix typo, avoid allocations, improve variable naming

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=79804&id=79813

BRANCH
  arcpatch-D24773_1

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kfileitem.h:349
> + *
> + * Initialy only implemented for trash:/
> + *

typo: Initially

> trashimpl.cpp:1099
> +
> +info = QFileInfo(trashPath);
> +modified = info.lastModified();

Reusing info is more expensive (and less readable) than creating it here 
everytime (price of assignment)

  const QFileInfo info(trashPath);

> trashimpl.cpp:1100
> +info = QFileInfo(trashPath);
> +modified = info.lastModified();
> +if (modDate.isNull() || modified > modDate) {

same here

> trashimpl.cpp:1107
> +KIO::UDSEntry entry;
> +entry.reserve(3);
> +entry.fastInsert(KIO::UDSEntry::UDS_RECURSIVE_SIZE, static_cast long>(size));

Why not move the whole code of createTopLevelDirEntry into this method?

It's weird to fill this in two separate steps.

OK this would require moving m_userName and m_groupName into this class...

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added a dependent revision: D28738: Use KFileItem::recursiveSize to 
Display recursize size as total size.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 79804.
meven marked 3 inline comments as done.
meven added a comment.


  Rebase, update @since, add precision to KFileItem comment

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=68829&id=79804

BRANCH
  arcpatch-D24773_1

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure added a comment.


  This looks good. I think I failed to notice the last update, sorry for the 
delay.
  
  Can you rebase (I suspect a few conflicts) and update version numbers to 
5.70? Then it'll be good to go.
  
  Ah, and the documentation for the KFileItem getter needs to say that this is 
generally NOT available and right now only implemented for trash:/.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 68829.
meven added a comment.


  Add KIO::UDSEntry::UDS_RECURSIVE_SIZE to store recursive size of folders

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=68402&id=68829

BRANCH
  arcpatch-D24773

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/core/udsentry.h
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added a comment.


  In D24773#554409 <https://phabricator.kde.org/D24773#554409>, @dfaure wrote:
  
  > You can do whatever you want in the GUI -- I'll happily step out of that 
part of the discussion --, as long as you don't abuse UDS_SIZE for what it was 
not meant for, thus creating an ambiguous meaning for it.
  >
  > My suggestion would be to add a UDS_RECURSIVE_SIZE instead.
  >  The problem with that, however, is that the kioslave can't know if the 
application actually wants the information. So in most cases we would be 
wasting a lot of time for nothing -- even here it's not fully for free.
  
  
  Some ioslave could provide it by default in some situation like for 
kio_trash, this make sense for /.
  
  > One solution would be to use the "details" metadata for this.
  > 
  > - 0 (as set by DeleteJob) means bare minimum (filename and type).
  > - 1 isn't used anymore, but it adds uid, gid, atime, mtime, btime
  > - 2 is the default, which is the above plus ACL data
  > - 3 requests in addition the device and inode number (from kio_file), so 
that DirectorySizeJob can check for hardlinks
  > - we could add that 4 means "I want UDS_RECURSIVE_SIZE too".
  
  Good idea, could be a base to fix https://bugs.kde.org/show_bug.cgi?id=158090 
later.
  
  This `KIO::stat` details parameter begs to be a bitmask...
  I am thinking about adding a KF6 TODO about this.
  
  > I don't know if this will be useful in any other kioslave, but at least 
here it would allow skipping this stuff when not needed.
  > 
  > Sample code, like in kio_file and kio_ftp:
  > 
  >   const QString sDetails = metaData(QLatin1String("details"));
  >   const int details = sDetails.isEmpty() ? 2 : sDetails.toInt();
  >
  
  All dependent of the implementations and the users.
  But for sure UDS_RECURSIVE_SIZE cannot be a default.
  
  About https://bugs.kde.org/show_bug.cgi?id=158090
  I would imagine something where the job recurse at most N levels, or M number 
of directory before stopping with N = 4 and M = 300.
  Returning incomplete results, but enough to give good information in most 
usercases.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure added a comment.


  You can do whatever you want in the GUI -- I'll happily step out of that part 
of the discussion --, as long as you don't abuse UDS_SIZE for what it was not 
meant for, thus creating an ambiguous meaning for it.
  
  My suggestion would be to add a UDS_RECURSIVE_SIZE instead.
  The problem with that, however, is that the kioslave can't know if the 
application actually wants the information. So in most cases we would be 
wasting a lot of time for nothing -- even here it's not fully for free.
  One solution would be to use the "details" metadata for this.
  
  - 0 (as set by DeleteJob) means bare minimum (filename and type).
  - 1 isn't used anymore, but it adds uid, gid, atime, mtime, btime
  - 2 is the default, which is the above plus ACL data
  - 3 requests in addition the device and inode number (from kio_file), so that 
DirectorySizeJob can check for hardlinks
  - we could add that 4 means "I want UDS_RECURSIVE_SIZE too".
  
  I don't know if this will be useful in any other kioslave, but at least here 
it would allow skipping this stuff when not needed.
  
  Sample code, like in kio_file and kio_ftp:
  
const QString sDetails = metaData(QLatin1String("details"));
const int details = sDetails.isEmpty() ? 2 : sDetails.toInt();

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added a comment.


  In D24773#551497 , @ngraham wrote:
  
  > In D24773#551245 , @meven wrote:
  >
  > > I would argue that "Disk space usage" and "Size" might be confusing to 
users. A folder size is the size of his content idiomatically.
  > >  That is our current usage of Size field for folders that is weird : My 
Image folder Size is "42 elements".
  > >  That is a paper cut that all new Plasma users have to learn "Size for 
folders is a content description" : not very intuitive.
  > >  We may want to in fact rename this field for folders to "Content" and 
keep Size for actual disk usage size (except in details view Size column of 
course).
  >
  >
  > Personally I would prefer it if we could actually calculate the total of a 
folder's contents: https://bugs.kde.org/show_bug.cgi?id=158090
  >
  > I tried to do this once and found that it was possible, but would need a 
lot of work to ensure adequate performance, and probably a lot of caching. I 
come from the macOS world where this is implemented and has no appreciable 
performance penalty whatsoever so I know it's possible.
  >
  > However this is probably unrelated to the current patch.
  >
  > Regarding this patch, I think the "Size" field for the trash should always 
show the total on-disk size of what's in the trash, regardless of the above, 
because the question that the user is looking to answer is, "How much space am 
I going to get back if I empty the trash right now?" For that, we need to 
always show the on-disk size.
  
  
  @dfaure any feedback ?
  Others maybe ?

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added a comment.


  In D24773#551497 , @ngraham wrote:
  
  > In D24773#551245 , @meven wrote:
  >
  > > I would argue that "Disk space usage" and "Size" might be confusing to 
users. A folder size is the size of his content idiomatically.
  > >  That is our current usage of Size field for folders that is weird : My 
Image folder Size is "42 elements".
  > >  That is a paper cut that all new Plasma users have to learn "Size for 
folders is a content description" : not very intuitive.
  > >  We may want to in fact rename this field for folders to "Content" and 
keep Size for actual disk usage size (except in details view Size column of 
course).
  >
  >
  > Personally I would prefer it if we could actually calculate the total of a 
folder's contents: https://bugs.kde.org/show_bug.cgi?id=158090
  >
  > I tried to do this once and found that it was possible, but would need a 
lot of work to ensure adequate performance, and probably a lot of caching. I 
come from the macOS world where this is implemented and has no appreciable 
performance penalty whatsoever so I know it's possible.
  >
  > However this is probably unrelated to the current patch.
  
  
  Agreed
  
  > Regarding this patch, I think the "Size" field for the trash should always 
show the total on-disk size of what's in the trash, regardless of the above, 
because the question that the user is looking to answer is, "How much space am 
I going to get back if I empty the trash right now?" For that, we need to 
always show the on-disk size.
  
  Agreed
  
  Your thought @dfaure ?

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Nathaniel Graham]

ngraham added a comment.


  In D24773#551245 , @meven wrote:
  
  > I would argue that "Disk space usage" and "Size" might be confusing to 
users. A folder size is the size of his content idiomatically.
  >  That is our current usage of Size field for folders that is weird : My 
Image folder Size is "42 elements".
  >  That is a paper cut that all new Plasma users have to learn "Size for 
folders is a content description" : not very intuitive.
  >  We may want to in fact rename this field for folders to "Content" and keep 
Size for actual disk usage size (except in details view Size column of course).
  
  
  Personally I would prefer it if we could actually calculate the total of a 
folder's contents: https://bugs.kde.org/show_bug.cgi?id=158090
  
  I tried to do this once and found that it was possible, but would need a lot 
of work to ensure adequate performance, and probably a lot of caching. I come 
from the macOS world where this is implemented and has no appreciable 
performance penalty whatsoever so I know it's possible.
  
  However this is probably unrelated to the current patch.
  
  Regarding this patch, I think the "Size" field for the trash should always 
show the total on-disk size of what's in the trash, regardless of the above, 
because the question that the user is looking to answer is, "How much space am 
I going to get back if I empty the trash right now?" For that, we need to 
always show the on-disk size.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven marked 2 inline comments as done.
meven added a comment.


  In D24773#551087 , @dfaure wrote:
  
  > I meant in the Dolphin view. RMB / Properties and calculating the size is 
available for trash:/. See 
http://www.davidfaure.fr/2019/dolphin_properties_dialog_for_trash.png
  
  
  This works from the folder view, but not from the places context menu where I 
tried it before.
  This is another issue.
  
  > I don't like abusing UDS_SIZE for this, because KIO is a virtual 
filesystem, trying to work like a normal filesystem. When you ::stat() a local 
directory, you get the size used by the directory entry, NOT the recursive size 
of all of its contents. There are ways to ask for that, by recursing, but 
explicitly, not as part of normal directory listing.
  >  I'm also concerned that now UDS_SIZE would have different meanings 
depending on the URL (sometimes the size of the directory entry, sometimes the 
full recursive size). This is how bad architecture starts, and unsolvable bugs 
- or ugly workarounds - tend to appear.
  
  Maybe we should add another fileld to UDSEntry like UDS_CONTENT_SIZE for this 
use case then to keep the two use cases clearly separate ?
  
  > There is indeed a fast way to get this particular one
  
  The way that exists necessitates two clicks and a load ; and it is available 
from only a particular place as mentioned up but that is not the issue here. 
But the value is in cache in fact for trash:/ we can shortcut the load.
  It is inconvenient and not what users would expect, some may just not find it.
  
  > , but I'm worried that this creates a precedent where the information panel 
starts expecting that all kioslaves fill UDS_SIZE with the full recursive size 
rather than just the size of the directory entry. And users expect to see the 
full recursive size there. I mean, after this goes in, the user goes to his 
~/Music folder, the information panel still says `Size: 4123 items`, they'll 
report a bug that it should instead show the full disk usage of that folder, 
right?
  
  Concerning recursive dir size, it is already a feature wish, that I would 
argue we should implement in some way sometimes (not default, not everywhere 
but still we would need something along this line). This is a legitimate 
feature in 2019 IMO :
  https://bugs.kde.org/show_bug.cgi?id=48434
  https://forum.kde.org/viewtopic.php?f=22&t=87452
  https://bugs.kde.org/show_bug.cgi?id=158090
  So I would argue that we need a way to surface directories content size to 
users.
  
  But here this does not concern size of dirs in general but only the size of 
trash:/ that will only be displayed in the information panel.
  So it might acceptable here.
  
  > To fix this issue, I would much rather that the information panel uses 
DirectorySizeJob (possibly on demand, like the Properties dialog); that class 
could have a fast path for trash: (e.g. making a special() call to the ioslave) 
Or if you don't want a button in that panel, special-case trash, call 
DirectorySizeJob only for trash, implement the fast path there - and change the 
label in the information panel to "Disk space usage" so it's not confused with 
the usual Size field.
  
  I would argue that "Disk space usage" and "Size" might be confusing to users. 
A folder size is the size of his content idiomatically.
  That is our current usage of Size field for folders that is weird : My Image 
folder Size is "42 elements".
  That is a paper cut that all new Plasma users have to learn "Size for folders 
is a content description" : not very intuitive.
  We may want to in fact rename this field for folders to "Content" and keep 
Size for actual disk usage size (except in details view Size column of course).
  
  The fast path in the information panel is completely viable here.
  
  I would appreciate some feedback from #vdg 
 as to what course of action is 
recommendable for a user experience perspective. @ngraham ?

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  I meant in the Dolphin view. RMB / Properties and calculating the size is 
available for trash:/. See 
http://www.davidfaure.fr/2019/dolphin_properties_dialog_for_trash.png
  
  I don't like abusing UDS_SIZE for this, because KIO is a virtual filesystem, 
trying to work like a normal filesystem. When you ::stat() a local directory, 
you get the size used by the directory entry, NOT the recursive size of all of 
its contents. There are ways to ask for that, by recursing, but explicitly, not 
as part of normal directory listing.
  
  There is indeed a fast way to get this particular one, but I'm worried that 
this creates a precedent where the information panel starts expecting that all 
kioslaves fill UDS_SIZE with the full recursive size rather than just the size 
of the directory entry. And users expect to see the full recursive size there. 
I mean, after this goes in, the user goes to his ~/Music folder, the 
information panel still says `Size: 4123 items`, they'll report a bug that it 
should instead show the full disk usage of that folder, right?
  
  I'm also concerned that now UDS_SIZE would have different meanings depending 
on the URL (sometimes the size of the directory entry, sometimes the full 
recursive size). This is how bad architecture starts, and unsolvable bugs - or 
ugly workarounds - tend to appear.
  
  To fix this issue, I would much rather that the information panel uses 
DirectorySizeJob (possibly on demand, like the Properties dialog); that class 
could have a fast path for trash: (e.g. making a special() call to the 
ioslave). Or if you don't want a button in that panel, special-case trash, call 
DirectorySizeJob only for trash, implement the fast path there - and change the 
label in the information panel to "Disk space usage" so it's not confused with 
the usual Size field.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

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

INLINE COMMENTS

> dfaure wrote in trashimpl.cpp:1088
> This method could be const, right?

No becauseof the call to `list()` that cannot be const.

> dfaure wrote in trashimpl.cpp:1104
> This looks very slow. We have a cache for the size usage. See 
> `TrashImpl::trashSpaceInfo`.
> 
> But anyway, we never return recursive directory size in the UDS_SIZE field, 
> in no ioslave.
> If someone wants to know the size of a directory, they can use the properties 
> dialog, which has a calculate button.

I missed the feature of trashSpaceInfo, thank you for pointing it out.

We need to add the UDS_SIZE field here : this is a missing feature for a 
special common use case : Finding out what amount of space is occupied by the 
trashed files ?
The trash size should be easy to find out for users. Plus we have a cache file 
to get this information cheaply.

UDS_SIZE may not be used that way currently but it is semantically correct 
nonetheless.
In most other cases directory size calculation is not used because it is too 
resource intensive but here we have the value pre-computed.

And by the way the trash:/ has not `Properties` entry in its context menu, so 
it is not even possible to get the trash size currently.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven updated this revision to Diff 68402.
meven added a comment.


  Use TrashSizeCache to file directory size, do not add creation date to the 
UDSEntry, better use UDSEntry reserve/clear

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24773?vs=68281&id=68402

BRANCH
  arcpatch-D24773

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

AFFECTED FILES
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Nathaniel Graham]

ngraham added a comment.


  Works great. Not sure creation date makes sense to show for the trash (if 
it's available) though. Can we suppress that?

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_trash.cpp:500
>  KIO::UDSEntry entry;
> +entry.clear();
>  createTopLevelDirEntry(entry);

why?

> trashimpl.cpp:1088
>  
> +KIO::UDSEntry TrashImpl::trashUDSEntry()
> +{

This method could be const, right?

> trashimpl.cpp:1104
> +// compute dir size if needed
> +KIO::DirectorySizeJob *job = 
> KIO::directorySize(QUrl::fromLocalFile(info.physicalPath));
> +connect(job, &KIO::DirectorySizeJob::result, this, [&size, job] 
> (){

This looks very slow. We have a cache for the size usage. See 
`TrashImpl::trashSpaceInfo`.

But anyway, we never return recursive directory size in the UDS_SIZE field, in 
no ioslave.
If someone wants to know the size of a directory, they can use the properties 
dialog, which has a calculate button.

> trashimpl.cpp:
> +}
> +entry.replace(KIO::UDSEntry::UDS_SIZE, static_cast(size));
> +

Why `replace`? This is inserting into a fresh new entry, it could use 
`fastInsert`

(same for the other calls below)

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven removed a dependent revision: D24774: Allow non-local directories to 
provide UDS_SIZE.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

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

INLINE COMMENTS

> elvisangelaccio wrote in trashimpl.cpp:1091
> Is this really needed? We just created `entry`.

It was done previously in void TrashProtocol::createTopLevelDirEntry to save 
some memory, so I just kept it here and line 500.
Although it is not very useful.

> elvisangelaccio wrote in trashimpl.cpp:1094
> coding style: local variables never start with an uppercase.

TrashedFileInfoList is the type of TrashedFileInfoList, no issue with codding 
style.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Elvis Angelaccio]

elvisangelaccio added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1

INLINE COMMENTS

> trashimpl.cpp:1091
> +KIO::UDSEntry entry;
> +entry.clear();
> +

Is this really needed? We just created `entry`.

> trashimpl.cpp:1094
> +// refresh list of trashes and get the list of files in them
> +const TrashedFileInfoList fileInfoList = list();
> +

coding style: local variables never start with an uppercase.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Nathaniel Graham]

ngraham added a dependent revision: D24774: Allow non-local directories to 
provide UDS_SIZE.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

meven added a reviewer: elvisangelaccio.

REPOSITORY
  R241 KIO

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Méven Car]

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

REVISION SUMMARY
  BUG: 413091

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/ioslaves/trash/kio_trash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/ioslaves/trash/trashimpl.h

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

D13918: Use non deprecated fastInsert in kio_trash

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:7b1210091c55: Use non deprecated fastInsert in kio_trash 
(authored by jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13918?vs=37232&id=37265

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

AFFECTED FILES
  src/ioslaves/trash/kio_trash.cpp

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13918: Use non deprecated fastInsert in kio_trash

[Jaime Torres Amate]

jtamate edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13918: Use non deprecated fastInsert in kio_trash

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  I remember there were good reasons for not setting UDS_LOCAL_PATH in 
kio_trash, but by now I forgot what they were exactly :-)
  
  I expect that some code will resolve to local paths before getting a chance 
to notice that it's in fact a trash url and trigger trash-specific code.
  But feel free to test that, provided the testing is complete enough :)
  (context menu, properties dialog, dnd for restoring, ...). That last one is a 
good one, we need to make sure kio_trash is involved otherwise the .trashinfo 
file won't get cleaned up.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13918: Use non deprecated fastInsert in kio_trash

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  This is the last one for kio
  The entry is cleared or new before calling createUDSEntry
  
  By the way, to fix https://bugs.kde.org/show_bug.cgi?id=368104
  Is it right to add KIO::UDSEntry::UDS_LOCAL_PATH in createUDSEntry?

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/ioslaves/trash/kio_trash.cpp

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

Re: Review Request 128910: [kio_trash] Fill in UDS_LOCAL_PATH in UDSEntry

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128910/#review99642
---



I'm very afraid of side effects of this.

For instance, deleting the file will do a direct deletion using the local path, 
not using kio_trash, and therefore leaving the .trashinfo file lying around 
(and not updating the total size usage of the trash).

And more such unwanted side effects.

Do we really need previews of folders in the trash? How about we disable that, 
rather?

- David Faure


On Sept. 14, 2016, 6:02 p.m., Wolfgang Bauer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128910/
> ---
> 
> (Updated Sept. 14, 2016, 6:02 p.m.)
> 
> 
> Review request for KDE Runtime, KDE Frameworks and David Faure.
> 
> 
> Bugs: 208625, 272249, 329155, and 368104
> https://bugs.kde.org/show_bug.cgi?id=208625
> https://bugs.kde.org/show_bug.cgi?id=272249
> https://bugs.kde.org/show_bug.cgi?id=329155
> https://bugs.kde.org/show_bug.cgi?id=368104
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Not doing so results in PreviewJob assuming the files/folders are remote and 
> copying them to /tmp when generating previews.
> This is especially annoying and dangerous if there are large folders in the 
> trash.
> 
> KDE4's kio_trash in kde-runtime has the same problem, as the code is 
> basically the same, the same patch fixes it too.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/kio_trash.cpp 3810941 
> 
> Diff: https://git.reviewboard.kde.org/r/128910/diff/
> 
> 
> Testing
> ---
> 
> Trash some files/folders, open dolphin, navigate to trash:/, and hover over 
> them.
> Previews are still generated (if enabled), and the files/folders are not 
> copied to /tmp any more.
> 
> Also tried with files on an USB stick, where a folder .Trash-XXX is used as 
> trash.
> 
> 
> Thanks,
> 
> Wolfgang Bauer
> 
>

Re: Review Request 128910: [kio_trash] Fill in UDS_LOCAL_PATH in UDSEntry

[Wolfgang Bauer]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128910/#review99637
---



Ping?

This can really cause bad problems and should be fixed ASAP IMHO, even if the 
problems exist since years.

- Wolfgang Bauer


On Sept. 14, 2016, 8:02 nachm., Wolfgang Bauer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128910/
> ---
> 
> (Updated Sept. 14, 2016, 8:02 nachm.)
> 
> 
> Review request for KDE Runtime, KDE Frameworks and David Faure.
> 
> 
> Bugs: 208625, 272249, 329155, and 368104
> https://bugs.kde.org/show_bug.cgi?id=208625
> https://bugs.kde.org/show_bug.cgi?id=272249
> https://bugs.kde.org/show_bug.cgi?id=329155
> https://bugs.kde.org/show_bug.cgi?id=368104
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Not doing so results in PreviewJob assuming the files/folders are remote and 
> copying them to /tmp when generating previews.
> This is especially annoying and dangerous if there are large folders in the 
> trash.
> 
> KDE4's kio_trash in kde-runtime has the same problem, as the code is 
> basically the same, the same patch fixes it too.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/kio_trash.cpp 3810941 
> 
> Diff: https://git.reviewboard.kde.org/r/128910/diff/
> 
> 
> Testing
> ---
> 
> Trash some files/folders, open dolphin, navigate to trash:/, and hover over 
> them.
> Previews are still generated (if enabled), and the files/folders are not 
> copied to /tmp any more.
> 
> Also tried with files on an USB stick, where a folder .Trash-XXX is used as 
> trash.
> 
> 
> Thanks,
> 
> Wolfgang Bauer
> 
>

Review Request 128910: [kio_trash] Fill in UDS_LOCAL_PATH in UDSEntry

[Wolfgang Bauer]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128910/
---

Review request for KDE Runtime, KDE Frameworks and David Faure.


Bugs: 208625, 272249, 329155, and 368104
https://bugs.kde.org/show_bug.cgi?id=208625
https://bugs.kde.org/show_bug.cgi?id=272249
https://bugs.kde.org/show_bug.cgi?id=329155
https://bugs.kde.org/show_bug.cgi?id=368104


Repository: kio


Description
---

Not doing so results in PreviewJob assuming the files/folders are remote and 
copying them to /tmp when generating previews.
This is especially annoying and dangerous if there are large folders in the 
trash.

KDE4's kio_trash in kde-runtime has the same problem, as the code is basically 
the same, the same patch fixes it too.


Diffs
-

  src/ioslaves/trash/kio_trash.cpp 3810941 

Diff: https://git.reviewboard.kde.org/r/128910/diff/


Testing
---

Trash some files/folders, open dolphin, navigate to trash:/, and hover over 
them.
Previews are still generated (if enabled), and the files/folders are not copied 
to /tmp any more.

Also tried with files on an USB stick, where a folder .Trash-XXX is used as 
trash.


Thanks,

Wolfgang Bauer

Re: Review Request 125290: Refresh Solid's device list before querying in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125290/
---

(Updated Sept. 28, 2015, 12:04 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit 9b82d31d66f3c7f831c158d7aa885469c76e967a by David 
Edmundson on behalf of Bartosz S?awianowski to branch master.


Repository: kio


Description
---

We need to process events in Qt event loop before querying Solid's device list. 
Solid caches devices and relies on signals to refresh them (at the very least 
that's how fstab backend works) which is a problem if kio_trash process is 
reused.

Currently if kio_trash process is reused and there were some device changes 
(for example new NFS share was mounted) kio_trash will fail to choose valid 
trash directory for "trashing" file - or fail to find all trash directories (in 
case of list action). Next reuse of the same process would actually succeed 
because TrashImpl::move (which is called later in the process of trashing file) 
enters the loop.


Diffs
-

  src/ioslaves/trash/trashimpl.cpp 5a2832a 
  src/ioslaves/trash/trashimpl.h 9881d8e 

Diff: https://git.reviewboard.kde.org/r/125290/diff/


Testing
---

Arch Linux


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125290: Refresh Solid's device list before querying in kio_trash

[Bartosz Sławianowski]

> On Sept. 26, 2015, 6:41 p.m., David Edmundson wrote:
> > do you have commit access?

Nope, would be nice if someone else could commit this


- Bartosz


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125290/#review85983
---


On Sept. 19, 2015, 7:14 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125290/
> ---
> 
> (Updated Sept. 19, 2015, 7:14 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> We need to process events in Qt event loop before querying Solid's device 
> list. Solid caches devices and relies on signals to refresh them (at the very 
> least that's how fstab backend works) which is a problem if kio_trash process 
> is reused.
> 
> Currently if kio_trash process is reused and there were some device changes 
> (for example new NFS share was mounted) kio_trash will fail to choose valid 
> trash directory for "trashing" file - or fail to find all trash directories 
> (in case of list action). Next reuse of the same process would actually 
> succeed because TrashImpl::move (which is called later in the process of 
> trashing file) enters the loop.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.cpp 5a2832a 
>   src/ioslaves/trash/trashimpl.h 9881d8e 
> 
> Diff: https://git.reviewboard.kde.org/r/125290/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125290: Refresh Solid's device list before querying in kio_trash

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125290/#review85983
---


do you have commit access?

- David Edmundson


On Sept. 19, 2015, 5:14 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125290/
> ---
> 
> (Updated Sept. 19, 2015, 5:14 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> We need to process events in Qt event loop before querying Solid's device 
> list. Solid caches devices and relies on signals to refresh them (at the very 
> least that's how fstab backend works) which is a problem if kio_trash process 
> is reused.
> 
> Currently if kio_trash process is reused and there were some device changes 
> (for example new NFS share was mounted) kio_trash will fail to choose valid 
> trash directory for "trashing" file - or fail to find all trash directories 
> (in case of list action). Next reuse of the same process would actually 
> succeed because TrashImpl::move (which is called later in the process of 
> trashing file) enters the loop.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.cpp 5a2832a 
>   src/ioslaves/trash/trashimpl.h 9881d8e 
> 
> Diff: https://git.reviewboard.kde.org/r/125290/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125290: Refresh Solid's device list before querying in kio_trash

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125290/#review85675
---

Ship it!


Ship It!

- David Faure


On Sept. 19, 2015, 5:14 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125290/
> ---
> 
> (Updated Sept. 19, 2015, 5:14 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> We need to process events in Qt event loop before querying Solid's device 
> list. Solid caches devices and relies on signals to refresh them (at the very 
> least that's how fstab backend works) which is a problem if kio_trash process 
> is reused.
> 
> Currently if kio_trash process is reused and there were some device changes 
> (for example new NFS share was mounted) kio_trash will fail to choose valid 
> trash directory for "trashing" file - or fail to find all trash directories 
> (in case of list action). Next reuse of the same process would actually 
> succeed because TrashImpl::move (which is called later in the process of 
> trashing file) enters the loop.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.cpp 5a2832a 
>   src/ioslaves/trash/trashimpl.h 9881d8e 
> 
> Diff: https://git.reviewboard.kde.org/r/125290/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125290: Refresh Solid's device list before querying in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125290/
---

(Updated Sept. 19, 2015, 7:14 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Added comments on why it's exactly needed and using qApp instead of QEventLoop


Repository: kio


Description
---

We need to process events in Qt event loop before querying Solid's device list. 
Solid caches devices and relies on signals to refresh them (at the very least 
that's how fstab backend works) which is a problem if kio_trash process is 
reused.

Currently if kio_trash process is reused and there were some device changes 
(for example new NFS share was mounted) kio_trash will fail to choose valid 
trash directory for "trashing" file - or fail to find all trash directories (in 
case of list action). Next reuse of the same process would actually succeed 
because TrashImpl::move (which is called later in the process of trashing file) 
enters the loop.


Diffs (updated)
-

  src/ioslaves/trash/trashimpl.cpp 5a2832a 
  src/ioslaves/trash/trashimpl.h 9881d8e 

Diff: https://git.reviewboard.kde.org/r/125290/diff/


Testing
---

Arch Linux


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 125290: Refresh Solid's device list before querying in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125290/
---

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

We need to process events in Qt event loop before querying Solid's device list. 
Solid caches devices and relies on signals to refresh them (at the very least 
that's how fstab backend works) which is a problem if kio_trash process is 
reused.

Currently if kio_trash process is reused and there were some device changes 
(for example new NFS share was mounted) kio_trash will fail to choose valid 
trash directory for "trashing" file - or fail to find all trash directories (in 
case of list action). Next reuse of the same process would actually succeed 
because TrashImpl::move (which is called later in the process of trashing file) 
enters the loop.


Diffs
-

  src/ioslaves/trash/trashimpl.h 9881d8e 
  src/ioslaves/trash/trashimpl.cpp 5a2832a 

Diff: https://git.reviewboard.kde.org/r/125290/diff/


Testing
---

Arch Linux


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/
---

(Updated Sept. 16, 2015, 10:17 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit a7871885046394d577eeea8fa8991d30a046545f by David Faure 
on behalf of Bartosz S?awianowski to branch master.


Bugs: 177023
https://bugs.kde.org/show_bug.cgi?id=177023


Repository: kio


Description
---

kio_trash doesn't support network shares, causing "trashed" files to be copied 
into user's home directory.

This patch adds support for them by assigning them persistent ID's (because 
there is no device minor/major ID or any other integer we can use) which are 
stored in trashrc config file - because we need to share them between possible 
multiple kio_trash processes.

Network shares get ID's starting with 600 to avoid possible conflicts with 
block devices.

NextID is also stored in trashrc file, I guess I could drop it by simply using 
group.keyList().count() or iterating all entries and finding max ID. Need 
opinions 

Thought about using different config file to avoid adding mutable modifier to 
m_config, need opinions here as well

Also using KInterProcessLock which already was there instead of adding 
dependency on KDbusAddons framework


Diffs
-

  src/ioslaves/trash/trashimpl.h cee9278 
  src/ioslaves/trash/trashimpl.cpp 300d320 

Diff: https://git.reviewboard.kde.org/r/125214/diff/


Testing
---

Arch Linux, few simple localhost NFS shares


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/#review85463
---

Ship it!


Ship It!

- David Faure


On Sept. 14, 2015, 10:31 a.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125214/
> ---
> 
> (Updated Sept. 14, 2015, 10:31 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 177023
> https://bugs.kde.org/show_bug.cgi?id=177023
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> kio_trash doesn't support network shares, causing "trashed" files to be 
> copied into user's home directory.
> 
> This patch adds support for them by assigning them persistent ID's (because 
> there is no device minor/major ID or any other integer we can use) which are 
> stored in trashrc config file - because we need to share them between 
> possible multiple kio_trash processes.
> 
> Network shares get ID's starting with 600 to avoid possible conflicts 
> with block devices.
> 
> NextID is also stored in trashrc file, I guess I could drop it by simply 
> using group.keyList().count() or iterating all entries and finding max ID. 
> Need opinions 
> 
> Thought about using different config file to avoid adding mutable modifier to 
> m_config, need opinions here as well
> 
> Also using KInterProcessLock which already was there instead of adding 
> dependency on KDbusAddons framework
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.h cee9278 
>   src/ioslaves/trash/trashimpl.cpp 300d320 
> 
> Diff: https://git.reviewboard.kde.org/r/125214/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux, few simple localhost NFS shares
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[Bartosz Sławianowski]

> On Sept. 14, 2015, 9:21 a.m., David Faure wrote:
> > src/ioslaves/trash/trashimpl.cpp, line 838
> > <https://git.reviewboard.kde.org/r/125214/diff/2/?file=403224#file403224line838>
> >
> > strange default value, 1 would work just as well, wouldn't it?
> > 
> > The first time we get here, we would use 1 and write NextID=2.
> > 
> > This looks like a mix of the two solutions you had in mind ;)

Right, forgot about that one


> On Sept. 14, 2015, 9:21 a.m., David Faure wrote:
> > src/ioslaves/trash/trashimpl.cpp, line 846
> > <https://git.reviewboard.kde.org/r/125214/diff/2/?file=403224#file403224line846>
> >
> > (not necessary, the dtor would do that anyway, but as you prefer)

Done it anyway, forgot about this when moving lock from class member to local 
variable


- Bartosz


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/#review85347
---


On Sept. 14, 2015, 12:31 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125214/
> ---
> 
> (Updated Sept. 14, 2015, 12:31 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 177023
> https://bugs.kde.org/show_bug.cgi?id=177023
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> kio_trash doesn't support network shares, causing "trashed" files to be 
> copied into user's home directory.
> 
> This patch adds support for them by assigning them persistent ID's (because 
> there is no device minor/major ID or any other integer we can use) which are 
> stored in trashrc config file - because we need to share them between 
> possible multiple kio_trash processes.
> 
> Network shares get ID's starting with 600 to avoid possible conflicts 
> with block devices.
> 
> NextID is also stored in trashrc file, I guess I could drop it by simply 
> using group.keyList().count() or iterating all entries and finding max ID. 
> Need opinions 
> 
> Thought about using different config file to avoid adding mutable modifier to 
> m_config, need opinions here as well
> 
> Also using KInterProcessLock which already was there instead of adding 
> dependency on KDbusAddons framework
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.h cee9278 
>   src/ioslaves/trash/trashimpl.cpp 300d320 
> 
> Diff: https://git.reviewboard.kde.org/r/125214/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux, few simple localhost NFS shares
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/
---

(Updated Sept. 14, 2015, 12:31 p.m.)


Review request for KDE Frameworks and David Faure.


Bugs: 177023
https://bugs.kde.org/show_bug.cgi?id=177023


Repository: kio


Description
---

kio_trash doesn't support network shares, causing "trashed" files to be copied 
into user's home directory.

This patch adds support for them by assigning them persistent ID's (because 
there is no device minor/major ID or any other integer we can use) which are 
stored in trashrc config file - because we need to share them between possible 
multiple kio_trash processes.

Network shares get ID's starting with 600 to avoid possible conflicts with 
block devices.

NextID is also stored in trashrc file, I guess I could drop it by simply using 
group.keyList().count() or iterating all entries and finding max ID. Need 
opinions 

Thought about using different config file to avoid adding mutable modifier to 
m_config, need opinions here as well

Also using KInterProcessLock which already was there instead of adding 
dependency on KDbusAddons framework


Diffs (updated)
-

  src/ioslaves/trash/trashimpl.h cee9278 
  src/ioslaves/trash/trashimpl.cpp 300d320 

Diff: https://git.reviewboard.kde.org/r/125214/diff/


Testing
---

Arch Linux, few simple localhost NFS shares


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/#review85347
---

Ship it!



src/ioslaves/trash/trashimpl.h (line 25)
<https://git.reviewboard.kde.org/r/125214/#comment58966>

namespace Solid { class Device; } would be enough here.



src/ioslaves/trash/trashimpl.cpp (line 831)
<https://git.reviewboard.kde.org/r/125214/#comment58963>

this returns a bool, so you're missing an if()

(it's unlikely to return false, e.g. no write access, return -1 if it does)



src/ioslaves/trash/trashimpl.cpp (line 838)
<https://git.reviewboard.kde.org/r/125214/#comment58965>

strange default value, 1 would work just as well, wouldn't it?

The first time we get here, we would use 1 and write NextID=2.

This looks like a mix of the two solutions you had in mind ;)



src/ioslaves/trash/trashimpl.cpp (line 846)
<https://git.reviewboard.kde.org/r/125214/#comment58964>

(not necessary, the dtor would do that anyway, but as you prefer)


- David Faure


On Sept. 13, 2015, 10:19 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125214/
> ---
> 
> (Updated Sept. 13, 2015, 10:19 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 177023
> https://bugs.kde.org/show_bug.cgi?id=177023
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> kio_trash doesn't support network shares, causing "trashed" files to be 
> copied into user's home directory.
> 
> This patch adds support for them by assigning them persistent ID's (because 
> there is no device minor/major ID or any other integer we can use) which are 
> stored in trashrc config file - because we need to share them between 
> possible multiple kio_trash processes.
> 
> Network shares get ID's starting with 600 to avoid possible conflicts 
> with block devices.
> 
> NextID is also stored in trashrc file, I guess I could drop it by simply 
> using group.keyList().count() or iterating all entries and finding max ID. 
> Need opinions 
> 
> Thought about using different config file to avoid adding mutable modifier to 
> m_config, need opinions here as well
> 
> Also using KInterProcessLock which already was there instead of adding 
> dependency on KDbusAddons framework
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.cpp 300d320 
>   src/ioslaves/trash/trashimpl.h cee9278 
> 
> Diff: https://git.reviewboard.kde.org/r/125214/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux, few simple localhost NFS shares
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/
---

(Updated Sept. 14, 2015, 12:19 a.m.)


Review request for KDE Frameworks and David Faure.


Bugs: 177023
https://bugs.kde.org/show_bug.cgi?id=177023


Repository: kio


Description
---

kio_trash doesn't support network shares, causing "trashed" files to be copied 
into user's home directory.

This patch adds support for them by assigning them persistent ID's (because 
there is no device minor/major ID or any other integer we can use) which are 
stored in trashrc config file - because we need to share them between possible 
multiple kio_trash processes.

Network shares get ID's starting with 600 to avoid possible conflicts with 
block devices.

NextID is also stored in trashrc file, I guess I could drop it by simply using 
group.keyList().count() or iterating all entries and finding max ID. Need 
opinions 

Thought about using different config file to avoid adding mutable modifier to 
m_config, need opinions here as well

Also using KInterProcessLock which already was there instead of adding 
dependency on KDbusAddons framework


Diffs (updated)
-

  src/ioslaves/trash/trashimpl.cpp 300d320 
  src/ioslaves/trash/trashimpl.h cee9278 

Diff: https://git.reviewboard.kde.org/r/125214/diff/


Testing
---

Arch Linux, few simple localhost NFS shares


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[David Faure]

> On Sept. 13, 2015, 8:48 p.m., David Faure wrote:
> > Thanks for the patch!
> > 
> > Protecting write access to a file is better done with QLockFile (which I 
> > added to Qt 5.0) than with a DBus based mechanism.
> > 
> > (btw I just noticed that KInterProcessLock is now unused in kio_trash, we 
> > should delete it, once your patch is in)
> 
> Bartosz Sławianowski wrote:
> What would be the best path for lock file, QStandardPaths::TempLocation 
> or QStandardPaths::CacheLocation (or something else)?

The best path is "right next to the file you're writing to".
So writableLocation(GenericConfigLocation)+"/trashrc.lock"
But wait, KConfig does this already, at a lower level. Your higher-level lock 
makes sense (read, increment, write). Just call it something slightly different 
then.

Something like writableLocation(GenericConfigLocation)+"/trashrc.nextid.lock".


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/#review85328
---


On Sept. 13, 2015, 7:59 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125214/
> ---
> 
> (Updated Sept. 13, 2015, 7:59 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 177023
> https://bugs.kde.org/show_bug.cgi?id=177023
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> kio_trash doesn't support network shares, causing "trashed" files to be 
> copied into user's home directory.
> 
> This patch adds support for them by assigning them persistent ID's (because 
> there is no device minor/major ID or any other integer we can use) which are 
> stored in trashrc config file - because we need to share them between 
> possible multiple kio_trash processes.
> 
> Network shares get ID's starting with 600 to avoid possible conflicts 
> with block devices.
> 
> NextID is also stored in trashrc file, I guess I could drop it by simply 
> using group.keyList().count() or iterating all entries and finding max ID. 
> Need opinions 
> 
> Thought about using different config file to avoid adding mutable modifier to 
> m_config, need opinions here as well
> 
> Also using KInterProcessLock which already was there instead of adding 
> dependency on KDbusAddons framework
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.h cee9278 
>   src/ioslaves/trash/trashimpl.cpp 300d320 
> 
> Diff: https://git.reviewboard.kde.org/r/125214/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux, few simple localhost NFS shares
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[Bartosz Sławianowski]

> On Sept. 13, 2015, 10:48 p.m., David Faure wrote:
> > Thanks for the patch!
> > 
> > Protecting write access to a file is better done with QLockFile (which I 
> > added to Qt 5.0) than with a DBus based mechanism.
> > 
> > (btw I just noticed that KInterProcessLock is now unused in kio_trash, we 
> > should delete it, once your patch is in)

What would be the best path for lock file, QStandardPaths::TempLocation or 
QStandardPaths::CacheLocation (or something else)?


- Bartosz


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/#review85328
---


On Sept. 13, 2015, 9:59 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125214/
> ---
> 
> (Updated Sept. 13, 2015, 9:59 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 177023
> https://bugs.kde.org/show_bug.cgi?id=177023
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> kio_trash doesn't support network shares, causing "trashed" files to be 
> copied into user's home directory.
> 
> This patch adds support for them by assigning them persistent ID's (because 
> there is no device minor/major ID or any other integer we can use) which are 
> stored in trashrc config file - because we need to share them between 
> possible multiple kio_trash processes.
> 
> Network shares get ID's starting with 600 to avoid possible conflicts 
> with block devices.
> 
> NextID is also stored in trashrc file, I guess I could drop it by simply 
> using group.keyList().count() or iterating all entries and finding max ID. 
> Need opinions 
> 
> Thought about using different config file to avoid adding mutable modifier to 
> m_config, need opinions here as well
> 
> Also using KInterProcessLock which already was there instead of adding 
> dependency on KDbusAddons framework
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.h cee9278 
>   src/ioslaves/trash/trashimpl.cpp 300d320 
> 
> Diff: https://git.reviewboard.kde.org/r/125214/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux, few simple localhost NFS shares
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/#review85328
---


Thanks for the patch!

Protecting write access to a file is better done with QLockFile (which I added 
to Qt 5.0) than with a DBus based mechanism.

(btw I just noticed that KInterProcessLock is now unused in kio_trash, we 
should delete it, once your patch is in)

- David Faure


On Sept. 13, 2015, 7:59 p.m., Bartosz Sławianowski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125214/
> ---
> 
> (Updated Sept. 13, 2015, 7:59 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 177023
> https://bugs.kde.org/show_bug.cgi?id=177023
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> kio_trash doesn't support network shares, causing "trashed" files to be 
> copied into user's home directory.
> 
> This patch adds support for them by assigning them persistent ID's (because 
> there is no device minor/major ID or any other integer we can use) which are 
> stored in trashrc config file - because we need to share them between 
> possible multiple kio_trash processes.
> 
> Network shares get ID's starting with 600 to avoid possible conflicts 
> with block devices.
> 
> NextID is also stored in trashrc file, I guess I could drop it by simply 
> using group.keyList().count() or iterating all entries and finding max ID. 
> Need opinions 
> 
> Thought about using different config file to avoid adding mutable modifier to 
> m_config, need opinions here as well
> 
> Also using KInterProcessLock which already was there instead of adding 
> dependency on KDbusAddons framework
> 
> 
> Diffs
> -
> 
>   src/ioslaves/trash/trashimpl.h cee9278 
>   src/ioslaves/trash/trashimpl.cpp 300d320 
> 
> Diff: https://git.reviewboard.kde.org/r/125214/diff/
> 
> 
> Testing
> ---
> 
> Arch Linux, few simple localhost NFS shares
> 
> 
> Thanks,
> 
> Bartosz Sławianowski
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125214: Add support for network shares in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/
---

(Updated Sept. 13, 2015, 9:59 p.m.)


Review request for KDE Frameworks and David Faure.


Bugs: 177023
https://bugs.kde.org/show_bug.cgi?id=177023


Repository: kio


Description (updated)
---

kio_trash doesn't support network shares, causing "trashed" files to be copied 
into user's home directory.

This patch adds support for them by assigning them persistent ID's (because 
there is no device minor/major ID or any other integer we can use) which are 
stored in trashrc config file - because we need to share them between possible 
multiple kio_trash processes.

Network shares get ID's starting with 600 to avoid possible conflicts with 
block devices.

NextID is also stored in trashrc file, I guess I could drop it by simply using 
group.keyList().count() or iterating all entries and finding max ID. Need 
opinions 

Thought about using different config file to avoid adding mutable modifier to 
m_config, need opinions here as well

Also using KInterProcessLock which already was there instead of adding 
dependency on KDbusAddons framework


Diffs
-

  src/ioslaves/trash/trashimpl.h cee9278 
  src/ioslaves/trash/trashimpl.cpp 300d320 

Diff: https://git.reviewboard.kde.org/r/125214/diff/


Testing
---

Arch Linux, few simple localhost NFS shares


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 125214: Add support for network shares in kio_trash

[Bartosz Sławianowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125214/
---

Review request for KDE Frameworks and David Faure.


Bugs: 177023
https://bugs.kde.org/show_bug.cgi?id=177023


Repository: kio


Description
---

kio_trash doesn't support network shares, causing "trashed" files to be copied 
into user's home directory.

This patch adds support for them by assigning them persistent ID's (because 
there is no device minor/major ID or any other integer we can use) which are 
stored in trashrc config file - because we need to share them between possible 
multiple kio_trash processes.

Network shares get ID's starting with 600 to avoid possible conflicts with 
block devices.

NextID is also stored in trashrc file, I guess I could drop it by simply using 
group.keyList().count() or iterating all entries and finding max ID. Need 
opinions 

Thought about using different config file to avoid adding mutable modified to 
m_config, need opinions here as well

Also using KInterProcessLock which already was there instead of adding 
dependency on KDbusAddons framework


Diffs
-

  src/ioslaves/trash/trashimpl.h cee9278 
  src/ioslaves/trash/trashimpl.cpp 300d320 

Diff: https://git.reviewboard.kde.org/r/125214/diff/


Testing
---

Arch Linux, few simple localhost NFS shares


Thanks,

Bartosz Sławianowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Alex Merry]

On 2014-09-22 21:26, David Faure wrote:

On Saturday 13 September 2014 20:29:18 Luigi Toscano wrote:

Luigi Toscano ha scritto:
> Nicolás Alvarez ha scritto:
>> 2014-09-13 15:13 GMT-03:00 Nicolás Alvarez :
>>> Done. The resulting history looks kind of weird though, because we
>>> have the full history of kio_trash but only partial history of kio, if
>>> you didn't graft kdelibs in.
>>
>> In fact, half the commits in kio are now from kio_trash. I didn't push
>> yet. I wonder if we should do the graft thing instead...


Strange, I didn't think there were that many commits to kio_trash.

Indeed, 618 commits - but just 388 after removing scripty.
Most of them qt3->qt4 and qt4->qt5 porting ;)


> It was already done for kdoctools (where you can see two parents). I would
> say it makes sense to import just the post-split commits, as you are
> grafting kdelibs anyway for kio history.

... but kio_trash comes from kde-runtime. Well, double grafting is 
possible

and maybe easier than full import anyway.


I'm fine with grafting, although I guess git-qt-grafts won't work since 
it
only looks at the very first commit in the repo? So I guess this will 
require
manual grafting - which in my case means "cd 
4/kde/kde-runtime/kioslave/trash"

will just be faster anyway.
I still have some kde3 svn modules checked out, so I'll be keeping kde4 
git

modules around for a very long time I'm sure :-)


git-qt-grafts works just fine with multiple graft points. Remember that 
in git, there can be multiple "very first commits".


Alex
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Nicolás Alvarez]

2014-10-01 4:49 GMT-03:00 David Faure :
> On Monday 22 September 2014 22:26:06 David Faure wrote:
>> On Saturday 13 September 2014 20:29:18 Luigi Toscano wrote:
>> > Luigi Toscano ha scritto:
>> > > Nicolás Alvarez ha scritto:
>> > >> 2014-09-13 15:13 GMT-03:00 Nicolás Alvarez :
>> > >>> Done. The resulting history looks kind of weird though, because we
>> > >>> have the full history of kio_trash but only partial history of kio, if
>> > >>> you didn't graft kdelibs in.
>> > >>
>> > >> In fact, half the commits in kio are now from kio_trash. I didn't push
>> > >> yet. I wonder if we should do the graft thing instead...
>>
>> Strange, I didn't think there were that many commits to kio_trash.
>>
>> Indeed, 618 commits - but just 388 after removing scripty.
>> Most of them qt3->qt4 and qt4->qt5 porting ;)
>>
>> > > It was already done for kdoctools (where you can see two parents). I
>> > > would
>> > > say it makes sense to import just the post-split commits, as you are
>> > > grafting kdelibs anyway for kio history.
>> >
>> > ... but kio_trash comes from kde-runtime. Well, double grafting is
>> > possible
>> > and maybe easier than full import anyway.
>>
>> I'm fine with grafting, although I guess git-qt-grafts won't work since it
>> only looks at the very first commit in the repo? So I guess this will
>> require manual grafting - which in my case means "cd
>> 4/kde/kde-runtime/kioslave/trash" will just be faster anyway.
>> I still have some kde3 svn modules checked out, so I'll be keeping kde4 git
>> modules around for a very long time I'm sure :-)
>
> Ping? Can this be done before saturday? Anything I can do to help?

Done. I only merged the history, someone else has to take care of the
build system etc :)

-- 
Nicolás
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[David Faure]

On Monday 22 September 2014 22:26:06 David Faure wrote:
> On Saturday 13 September 2014 20:29:18 Luigi Toscano wrote:
> > Luigi Toscano ha scritto:
> > > Nicolás Alvarez ha scritto:
> > >> 2014-09-13 15:13 GMT-03:00 Nicolás Alvarez :
> > >>> Done. The resulting history looks kind of weird though, because we
> > >>> have the full history of kio_trash but only partial history of kio, if
> > >>> you didn't graft kdelibs in.
> > >> 
> > >> In fact, half the commits in kio are now from kio_trash. I didn't push
> > >> yet. I wonder if we should do the graft thing instead...
> 
> Strange, I didn't think there were that many commits to kio_trash.
> 
> Indeed, 618 commits - but just 388 after removing scripty.
> Most of them qt3->qt4 and qt4->qt5 porting ;)
> 
> > > It was already done for kdoctools (where you can see two parents). I
> > > would
> > > say it makes sense to import just the post-split commits, as you are
> > > grafting kdelibs anyway for kio history.
> > 
> > ... but kio_trash comes from kde-runtime. Well, double grafting is
> > possible
> > and maybe easier than full import anyway.
> 
> I'm fine with grafting, although I guess git-qt-grafts won't work since it
> only looks at the very first commit in the repo? So I guess this will
> require manual grafting - which in my case means "cd
> 4/kde/kde-runtime/kioslave/trash" will just be faster anyway.
> I still have some kde3 svn modules checked out, so I'll be keeping kde4 git
> modules around for a very long time I'm sure :-)

Ping? Can this be done before saturday? Anything I can do to help?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[David Faure]

On Saturday 13 September 2014 20:29:18 Luigi Toscano wrote:
> Luigi Toscano ha scritto:
> > Nicolás Alvarez ha scritto:
> >> 2014-09-13 15:13 GMT-03:00 Nicolás Alvarez :
> >>> Done. The resulting history looks kind of weird though, because we
> >>> have the full history of kio_trash but only partial history of kio, if
> >>> you didn't graft kdelibs in.
> >> 
> >> In fact, half the commits in kio are now from kio_trash. I didn't push
> >> yet. I wonder if we should do the graft thing instead...

Strange, I didn't think there were that many commits to kio_trash.

Indeed, 618 commits - but just 388 after removing scripty.
Most of them qt3->qt4 and qt4->qt5 porting ;)

> > It was already done for kdoctools (where you can see two parents). I would
> > say it makes sense to import just the post-split commits, as you are
> > grafting kdelibs anyway for kio history.
> 
> ... but kio_trash comes from kde-runtime. Well, double grafting is possible
> and maybe easier than full import anyway.

I'm fine with grafting, although I guess git-qt-grafts won't work since it 
only looks at the very first commit in the repo? So I guess this will require 
manual grafting - which in my case means "cd 4/kde/kde-runtime/kioslave/trash" 
will just be faster anyway.
I still have some kde3 svn modules checked out, so I'll be keeping kde4 git 
modules around for a very long time I'm sure :-)

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Luigi Toscano]

Luigi Toscano ha scritto:
> Nicolás Alvarez ha scritto:
>> 2014-09-13 15:13 GMT-03:00 Nicolás Alvarez :

>>> Done. The resulting history looks kind of weird though, because we
>>> have the full history of kio_trash but only partial history of kio, if
>>> you didn't graft kdelibs in.
>>
>> In fact, half the commits in kio are now from kio_trash. I didn't push
>> yet. I wonder if we should do the graft thing instead...
> 
> It was already done for kdoctools (where you can see two parents). I would say
> it makes sense to import just the post-split commits, as you are grafting
> kdelibs anyway for kio history.

... but kio_trash comes from kde-runtime. Well, double grafting is possible
and maybe easier than full import anyway.

Ciao
-- 
Luigi

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Luigi Toscano]

Nicolás Alvarez ha scritto:
> 2014-09-13 15:13 GMT-03:00 Nicolás Alvarez :
>> 2014-09-09 12:22 GMT-03:00 David Faure :
>>> On Sunday 17 August 2014 00:03:41 David Faure wrote:
>>>> kio_trash is currently in kde-workspace/kio-extras, but KIO actually offers
>>>> API that depends on kio_trash (KIO::trash(), JobUiDelegateExtension::Trash,
>>>> support for trash in FileUndoManager, and I'm about to add a
>>>> KIO::restoreFromTrash job).
>>>> And KIO::trash is called from the file dialog (kdiroperator.cpp).
>>>> So this is a missing runtime dependency if kde-workspace/kio-extras isn't
>>>> installed.
>>>>
>>>> How about we move kio_trash to the kio framework?
>>>>
>>>> (If we agree, can someone do it? I still have no experience with doing this
>>>> in a way that preserves the git history)
>>>
>>> Aleix or Nicolas, can you move it?
>>
>> Done. The resulting history looks kind of weird though, because we
>> have the full history of kio_trash but only partial history of kio, if
>> you didn't graft kdelibs in.
> 
> In fact, half the commits in kio are now from kio_trash. I didn't push
> yet. I wonder if we should do the graft thing instead...

It was already done for kdoctools (where you can see two parents). I would say
it makes sense to import just the post-split commits, as you are grafting
kdelibs anyway for kio history.

Ciao
-- 
Luigi

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Nicolás Alvarez]

2014-09-13 15:13 GMT-03:00 Nicolás Alvarez :
> 2014-09-09 12:22 GMT-03:00 David Faure :
>> On Sunday 17 August 2014 00:03:41 David Faure wrote:
>>> kio_trash is currently in kde-workspace/kio-extras, but KIO actually offers
>>> API that depends on kio_trash (KIO::trash(), JobUiDelegateExtension::Trash,
>>> support for trash in FileUndoManager, and I'm about to add a
>>> KIO::restoreFromTrash job).
>>> And KIO::trash is called from the file dialog (kdiroperator.cpp).
>>> So this is a missing runtime dependency if kde-workspace/kio-extras isn't
>>> installed.
>>>
>>> How about we move kio_trash to the kio framework?
>>>
>>> (If we agree, can someone do it? I still have no experience with doing this
>>> in a way that preserves the git history)
>>
>> Aleix or Nicolas, can you move it?
>
> Done. The resulting history looks kind of weird though, because we
> have the full history of kio_trash but only partial history of kio, if
> you didn't graft kdelibs in.

In fact, half the commits in kio are now from kio_trash. I didn't push
yet. I wonder if we should do the graft thing instead...

-- 
Nicolás
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Nicolás Alvarez]

2014-09-09 12:22 GMT-03:00 David Faure :
> On Sunday 17 August 2014 00:03:41 David Faure wrote:
>> kio_trash is currently in kde-workspace/kio-extras, but KIO actually offers
>> API that depends on kio_trash (KIO::trash(), JobUiDelegateExtension::Trash,
>> support for trash in FileUndoManager, and I'm about to add a
>> KIO::restoreFromTrash job).
>> And KIO::trash is called from the file dialog (kdiroperator.cpp).
>> So this is a missing runtime dependency if kde-workspace/kio-extras isn't
>> installed.
>>
>> How about we move kio_trash to the kio framework?
>>
>> (If we agree, can someone do it? I still have no experience with doing this
>> in a way that preserves the git history)
>
> Aleix or Nicolas, can you move it?

Done. The resulting history looks kind of weird though, because we
have the full history of kio_trash but only partial history of kio, if
you didn't graft kdelibs in.

-- 
Nicolás
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[David Faure]

On Sunday 17 August 2014 00:03:41 David Faure wrote:
> kio_trash is currently in kde-workspace/kio-extras, but KIO actually offers
> API that depends on kio_trash (KIO::trash(), JobUiDelegateExtension::Trash,
> support for trash in FileUndoManager, and I'm about to add a
> KIO::restoreFromTrash job).
> And KIO::trash is called from the file dialog (kdiroperator.cpp).
> So this is a missing runtime dependency if kde-workspace/kio-extras isn't
> installed.
> 
> How about we move kio_trash to the kio framework?
> 
> (If we agree, can someone do it? I still have no experience with doing this
> in a way that preserves the git history)

Aleix or Nicolas, can you move it?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Kevin Funk]

On Sunday 17 August 2014 00:03:41 David Faure wrote:
> kio_trash is currently in kde-workspace/kio-extras, but KIO actually offers
> API that depends on kio_trash (KIO::trash(), JobUiDelegateExtension::Trash,
> support for trash in FileUndoManager, and I'm about to add a
> KIO::restoreFromTrash job).
> And KIO::trash is called from the file dialog (kdiroperator.cpp).
> So this is a missing runtime dependency if kde-workspace/kio-extras isn't
> installed.
> 
> How about we move kio_trash to the kio framework?
> 
> (If we agree, can someone do it? I still have no experience with doing this
> in a way that preserves the git history)

I guess that'd also fix my CI-related issue here [1], right:

QWARN  : GitInitTest::testRemoveEmptyFolder() couldn't create slave: "Unable 
to create io-slave:
klauncher said: Unknown protocol 'trash'.
"

(We're using KIO::trash, but the CI doesn't provide this particular slave 
because it's part of different module)

Greets

[1] 
http://build.kde.org/view/kdevelop/job/kdevplatform_frameworks_qt5/154/testReport/(root)/TestSuite/test_kdevgit/

-- 
Kevin Funk | kf...@kde.org | http://kfunk.org
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: kio_trash

[Alex Merry]

On Sunday 17 August 2014 00:03:41 David Faure wrote:
> kio_trash is currently in kde-workspace/kio-extras, but KIO actually offers
> API that depends on kio_trash (KIO::trash(), JobUiDelegateExtension::Trash,
> support for trash in FileUndoManager, and I'm about to add a
> KIO::restoreFromTrash job).
> And KIO::trash is called from the file dialog (kdiroperator.cpp).
> So this is a missing runtime dependency if kde-workspace/kio-extras isn't
> installed.
> 
> How about we move kio_trash to the kio framework?
> 
> (If we agree, can someone do it? I still have no experience with doing this
> in a way that preserves the git history)

That sounds sensible. I would volunteer to do it, but after today I have no 
free time for over a week (which will be very close to the next release).

If someone else wants to have a go, 
https://community.kde.org/Sysadmin/GitKdeOrgManual#Advanced_Git is the write-
up I did of how to do this sort of thing.

Alex
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

kio_trash

[David Faure]

kio_trash is currently in kde-workspace/kio-extras, but KIO actually offers 
API that depends on kio_trash (KIO::trash(), JobUiDelegateExtension::Trash,
support for trash in FileUndoManager, and I'm about to add a 
KIO::restoreFromTrash job).
And KIO::trash is called from the file dialog (kdiroperator.cpp).
So this is a missing runtime dependency if kde-workspace/kio-extras isn't 
installed.

How about we move kio_trash to the kio framework?

(If we agree, can someone do it? I still have no experience with doing this in 
a way that preserves the git history)

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel