Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Jan. 13, 2015, 5:44 p.m.) Status -- This change has been marked as submitted. Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp fe2e152 kioslave/trash/trashimpl.h bc68723 kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/CMakeLists.txt 3604089 Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On gen. 9, 2015, 12:27 a.m., David Faure wrote: Sorry, this one got drowned in the ML noise and I missed it After renaming idForDevice to idForMountPoint, please push to kde-runtime once the Applications 14.12 freeze is over. Then you'll have to forward-port this commit to the kio repository, where the kio_trash code has moved (and port your code to Qt5/KF5, although hopefully it's not a lot of work). René J.V. Bertin wrote: When will that freeze be over? Also, think I introduced a (minor?) regression when I heeded a number of comments on Oct. 17th, and wrote _Trash infrastructure is now created only when really needed (please check if I forgot any cases), which does NOT include scanning the trash nor doing something with a trashed file (knowing a trashed file by name should mean the infrastruct. exists)._ The regression being that when you empty the OS X trash and then navigate to the wastebin in Dolphin, any KDE content that was in the trash is still shown; even the properties are still available. I haven't checked exactly why this is yet; should I, or could I use it as a check to see if anyone actually uses the feature enough to be bothered? ;) The packages for 14.12.1 have been created, you can commit now. About the regression, is it MacOS only or for everyone? - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review73542 --- On nov. 27, 2014, 11:04 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated nov. 27, 2014, 11:04 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp fe2e152 kioslave/trash/trashimpl.h bc68723 kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/CMakeLists.txt 3604089 Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Jan. 9, 2015, 1:27 a.m., David Faure wrote: Sorry, this one got drowned in the ML noise and I missed it After renaming idForDevice to idForMountPoint, please push to kde-runtime once the Applications 14.12 freeze is over. Then you'll have to forward-port this commit to the kio repository, where the kio_trash code has moved (and port your code to Qt5/KF5, although hopefully it's not a lot of work). René J.V. Bertin wrote: When will that freeze be over? Also, think I introduced a (minor?) regression when I heeded a number of comments on Oct. 17th, and wrote _Trash infrastructure is now created only when really needed (please check if I forgot any cases), which does NOT include scanning the trash nor doing something with a trashed file (knowing a trashed file by name should mean the infrastruct. exists)._ The regression being that when you empty the OS X trash and then navigate to the wastebin in Dolphin, any KDE content that was in the trash is still shown; even the properties are still available. I haven't checked exactly why this is yet; should I, or could I use it as a check to see if anyone actually uses the feature enough to be bothered? ;) Albert Astals Cid wrote: The packages for 14.12.1 have been created, you can commit now. About the regression, is it MacOS only or for everyone? How could it be for everyone? It's only on OS X that the KDE wastebin is hosted in the Finder trash ;) - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review73542 --- On Nov. 28, 2014, 12:04 a.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Nov. 28, 2014, 12:04 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp fe2e152 kioslave/trash/trashimpl.h bc68723 kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/CMakeLists.txt 3604089 Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Jan. 9, 2015, 1:27 a.m., David Faure wrote: Sorry, this one got drowned in the ML noise and I missed it After renaming idForDevice to idForMountPoint, please push to kde-runtime once the Applications 14.12 freeze is over. Then you'll have to forward-port this commit to the kio repository, where the kio_trash code has moved (and port your code to Qt5/KF5, although hopefully it's not a lot of work). When will that freeze be over? Also, think I introduced a (minor?) regression when I heeded a number of comments on Oct. 17th, and wrote _Trash infrastructure is now created only when really needed (please check if I forgot any cases), which does NOT include scanning the trash nor doing something with a trashed file (knowing a trashed file by name should mean the infrastruct. exists)._ The regression being that when you empty the OS X trash and then navigate to the wastebin in Dolphin, any KDE content that was in the trash is still shown; even the properties are still available. I haven't checked exactly why this is yet; should I, or could I use it as a check to see if anyone actually uses the feature enough to be bothered? ;) - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review73542 --- On Nov. 28, 2014, 12:04 a.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Nov. 28, 2014, 12:04 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp fe2e152 kioslave/trash/trashimpl.h bc68723 kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/CMakeLists.txt 3604089 Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 19, 2014, 8:25 a.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 870 https://git.reviewboard.kde.org/r/120573/diff/9/?file=320645#file320645line870 Should be const QString mountPoint, you're not modifying it. Also, rename this method to idForMountPoint, it doesn't take a device string as input, but a mountpoint string. This confused me greatly when looking at the calling code :-) This issue wasn't fixed - please rename idForDevice to idForMountPoint, since that's what it does - and check all callers indeed pass a mountpoint, not a device. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68687 --- On Nov. 27, 2014, 11:04 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Nov. 27, 2014, 11:04 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp fe2e152 kioslave/trash/trashimpl.h bc68723 kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/CMakeLists.txt 3604089 Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review73542 --- Ship it! Sorry, this one got drowned in the ML noise and I missed it After renaming idForDevice to idForMountPoint, please push to kde-runtime once the Applications 14.12 freeze is over. Then you'll have to forward-port this commit to the kio repository, where the kio_trash code has moved (and port your code to Qt5/KF5, although hopefully it's not a lot of work). - David Faure On Nov. 27, 2014, 11:04 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Nov. 27, 2014, 11:04 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp fe2e152 kioslave/trash/trashimpl.h bc68723 kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/CMakeLists.txt 3604089 Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Nov. 28, 2014, 12:04 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/trashimpl.cpp fe2e152 kioslave/trash/trashimpl.h bc68723 kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/CMakeLists.txt 3604089 Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/#review68687
---
kioslave/trash/CMakeLists.txt
https://git.reviewboard.kde.org/r/120573/#comment47875
ok with the variable to avoid duplication, but now kio+solid are duplicated
(and changed completely in KF5)
This would be better:
set (kio_trash_LIBS ...)
if (APPLE)
set(kio_trash_LIBS ${kio_trash_LIBS} -framework DiskArbitration)
endif()
kioslave/trash/kcmtrash.cpp
https://git.reviewboard.kde.org/r/120573/#comment47869
My point was that you forgot to change this one back :-)
kioslave/trash/tests/CMakeLists.txt
https://git.reviewboard.kde.org/r/120573/#comment47874
Don't duplicate the list of libs.
Just add a
if (APPLE)
target_link_libraries(testtrash -framework DiskArbitration)
endif()
target_link_libraries is cumulative, it doesn't have to be called only once.
kioslave/trash/trashimpl.h
https://git.reviewboard.kde.org/r/120573/#comment47870
spaces around '='.
And no, don't worry about the overhead. The QString() constructor does
very little (it uses an internal null-string instance).
No need for premature optimization at the expense of readability.
kioslave/trash/trashimpl.cpp
https://git.reviewboard.kde.org/r/120573/#comment47871
!path.isEmpty() is more usual in Qt code than path.size().
kioslave/trash/trashimpl.cpp
https://git.reviewboard.kde.org/r/120573/#comment47873
Should be const QString mountPoint, you're not modifying it.
Also, rename this method to idForMountPoint, it doesn't take a device
string as input, but a mountpoint string. This confused me greatly when looking
at the calling code :-)
kioslave/trash/trashimpl.cpp
https://git.reviewboard.kde.org/r/120573/#comment47872
You can't do that. I'm surprised it doesn't crash for you, actually.
encodeName returns a QByteArray. If you only store a char*, the bytearray goes
out of scope and you're looking at deleted memory.
Turn mp into a QByteArray, and remove the if(mp).
- David Faure
On Oct. 18, 2014, 6:08 p.m., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/
---
(Updated Oct. 18, 2014, 6:08 p.m.)
Review request for KDE Software on Mac OS X, KDE Runtime and David Faure.
Repository: kde-runtime
Description
---
KDE on OS X does not handle the desktop session (no Plasma) nor can it rely
on XDG to obtain the proper paths to use for something like the trash. As a
result, all applications that propose to move things they manage to the
wastebin (Dolphin, but also digiKam) will store those items in a place that
has no particular meaning on OS X, and that will thus tend to fill up.
OS X stores trash in one of several locations. Files trashed from the boot
volume (and/or the volume containing $HOME, I don't actually know that) end
up in `~/.Trash`. Files deleted from other volumes end up in
`/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless
whether it's an external or a remote drive; only mounted NFS shares are
handled differently) and uid the numerical user id. Permissions on `.Trashes`
are the same as those expected by KDE.
The kio_trash kioslave appears to support several actual trash directory
locations, just like OS X. `TrashImpl::init()` creates a standard trash in
`~/.local/share/Trash` (at least under OS X) but also
`TrashImpl::trashForMountPoint()` that is used in cases I have not yet
encountered.
On OS X, my modified `TrashImpl::init()` sets the standard trash directory to
`~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as
required, because they will of course be deleted when the user empties the OS
X trash. `TrashImpl::fileRemoved()` has been modified to call a new function,
`deleteEmptyTrashInfraStructure` to delete the KDE trash's internal
infrastructure when the wastebin is empty so that OS X also sees the trash as
emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature
actually works, as expected as far as I can tell).
Remains to be done:
- determine in what cases `trashForMountPoint()` is used, and finish the
modifications for it to use `/.Trashes/uid/KDE.trash`
Diffs
-
kioslave/trash/CMakeLists.txt 3604089
kioslave/trash/kcmtrash.cpp f4811fd
kioslave/trash/tests/CMakeLists.txt 9161fdf
kioslave/trash/trashimpl.h bc68723
kioslave/trash/trashimpl.cpp 30ee05b
Diff: https://git.reviewboard.kde.org/r/120573/diff/
Testing
---
On OS X 10.6.8 with
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 18, 2014, 11:28 a.m., David Faure wrote: Almost there :-) Re your earlier comments: - trashimpl.h is completely internal to the kioslave, adding methods there is no problem - trashForMountPoint is called when trashing a file that is on a different partition than HOME. I can see it being called when I do this, on Linux: cd /tmp touch foo export KDE_FORK_SLAVES=1 kde-cp foo trash:/ The debug output from kio_trash (which appears in the same terminal due to the use of KDE_FORK_SLAVES=1) says TrashImpl::findTrashDirectory: mountPoint= / trashDir= which is one line below the call to trashForMountPoint, and prints out its result. So it's called for sure :-) Indeed it's called, but then the/my code doesn't use whatever is returned, using ~/.Trash/KDE.trash instead. Hmmm. Wonder why I never saw it called when using Dolphin, though. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68648 --- On Oct. 17, 2014, 12:35 a.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 17, 2014, 12:35 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On oct. 18, 2014, 9:28 matin, David Faure wrote: Almost there :-) Re your earlier comments: - trashimpl.h is completely internal to the kioslave, adding methods there is no problem - trashForMountPoint is called when trashing a file that is on a different partition than HOME. I can see it being called when I do this, on Linux: cd /tmp touch foo export KDE_FORK_SLAVES=1 kde-cp foo trash:/ The debug output from kio_trash (which appears in the same terminal due to the use of KDE_FORK_SLAVES=1) says TrashImpl::findTrashDirectory: mountPoint= / trashDir= which is one line below the call to trashForMountPoint, and prints out its result. So it's called for sure :-) René J.V. Bertin wrote: Indeed it's called, but then the/my code doesn't use whatever is returned, using ~/.Trash/KDE.trash instead. Hmmm. Wonder why I never saw it called when using Dolphin, though. probably because the trash dir at the toplevel of the partition doesn't exist and can't be created by the user. See the security checks in the spec code. Try creating a trash dir in the mount point with the right permissions and ownership. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68648 --- On oct. 16, 2014, 10:35 après-midi, René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated oct. 16, 2014, 10:35 après-midi) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 18, 2014, 8:08 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Changes --- Includes today's fixed issues as well as an OS X-specific `idForDevice` function that is independent of the Solid library for now. With these latest changes, the KDE trash is hosted in the volume-specific OS X trash directory (KDE.trash in /Volumes/name/.Trashes/uid) or in the trash directory under $HOME. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/CMakeLists.txt 3604089 kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 1043 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. René J.V. Bertin wrote: Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ... René J.V. Bertin wrote: I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of. Unless it's a remnant from the past that's no longer being used? David Faure wrote: What would be a remnant from the past? trashForMountPoint?? It's called by TrashImpl::findTrashDirectory and by TrashImpl::scanTrashDirectories. It's the way we find the trash dirs on other partitions. Surely that is used. You can e.g. try opening trash:/ in a kde file manager, it will scan for all trash dirs. René J.V. Bertin wrote: Well, I've done that; opening the trash in Dolphin is part of my regular testing routine for this. And NEVER have I yet seen debug output from trashForMountPoint... René J.V. Bertin wrote: Even for the NFS mount mounted under /Volumes (comparable to /media) I just checked: the file I moved ended up in the ~/.Trash directory. Note to self: This appears to work now, remember to remove the debug statements or generalise them. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 18, 2014, 8:08 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 18, 2014, 8:08 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/CMakeLists.txt 3604089 kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/tests/CMakeLists.txt 9161fdf kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 1043 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. René J.V. Bertin wrote: Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ... René J.V. Bertin wrote: I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of. Unless it's a remnant from the past that's no longer being used? David Faure wrote: What would be a remnant from the past? trashForMountPoint?? It's called by TrashImpl::findTrashDirectory and by TrashImpl::scanTrashDirectories. It's the way we find the trash dirs on other partitions. Surely that is used. You can e.g. try opening trash:/ in a kde file manager, it will scan for all trash dirs. Well, I've done that; opening the trash in Dolphin is part of my regular testing routine for this. And NEVER have I yet seen debug output from trashForMountPoint... - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 1043 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. René J.V. Bertin wrote: Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ... René J.V. Bertin wrote: I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of. Unless it's a remnant from the past that's no longer being used? David Faure wrote: What would be a remnant from the past? trashForMountPoint?? It's called by TrashImpl::findTrashDirectory and by TrashImpl::scanTrashDirectories. It's the way we find the trash dirs on other partitions. Surely that is used. You can e.g. try opening trash:/ in a kde file manager, it will scan for all trash dirs. René J.V. Bertin wrote: Well, I've done that; opening the trash in Dolphin is part of my regular testing routine for this. And NEVER have I yet seen debug output from trashForMountPoint... Even for the NFS mount mounted under /Volumes (comparable to /media) I just checked: the file I moved ended up in the ~/.Trash directory. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Okt. 14, 2014, 9:13 nachm., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. René J.V. Bertin wrote: Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/classTrashImpl.html On the $TRASH/KDE.trash issue and in general: OS_X stores trashed files directly in ~/.Trash, I take? In that case i foresee a general issue (maybe academic, but possible) with the approach to store the KDE trash stuff there (which OSX will take as trashed files). What happens if you delete a file named info or files or KDE.trash? In either case you'd likely run into a conflict? Ie. either OS_X cannot trash the new file (for there's a directory of that name) or OS_X overrides/invalidates the entire KDE trash or OS_X creates such file and you can't add the KDE stuff there. Yesno? In case: how does the OS_X trash handle hidden (.*) files? Assuming it doesn't put hidden files dotted into the trash, how does it react when adding a hidden file there (whether .info, .files or .KDE.trash)? - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Okt. 15, 2014, 4:26 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Okt. 15, 2014, 4:26 nachm.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. René J.V. Bertin wrote: Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? Thomas Lübking wrote: http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/classTrashImpl.html On the $TRASH/KDE.trash issue and in general: OS_X stores trashed files directly in ~/.Trash, I take? In that case i foresee a general issue (maybe academic, but possible) with the approach to store the KDE trash stuff there (which OSX will take as trashed files). What happens if you delete a file named info or files or KDE.trash? In either case you'd likely run into a conflict? Ie. either OS_X cannot trash the new file (for there's a directory of that name) or OS_X overrides/invalidates the entire KDE trash or OS_X creates such file and you can't add the KDE stuff there. Yesno? In case: how does the OS_X trash handle hidden (.*) files? Assuming it doesn't put hidden files dotted into the trash, how does it react when adding a hidden file there (whether .info, .files or .KDE.trash)? In fact, it works exactly the way the KDE trash works, except for the restore info which (I presume) is stored in the file metadata. So: - deleting a file that has the same name as a file already in the trash will 1) rename that file to something like name 1 and 2) move that file into the trash. (I've been able to see that happen when the system is very busy). In short, it's only if someone already put a KDE.trash in the trash that we'd into trouble, unless it's a directory. - Why wouldn't it put hidden files there? Again, the Finder works like Dolphin or Nautilus: hidden files are nothing special, they're just not shown in the Finder (nor in file selection dialogs). I'm not sure if the trash would show as non-empty if we store everything under ~/.Trash/.KDE.trash but if it does we have the problem that the user won't understand why when s/he opens the Trash in the Finder. And if it doesn't we're more or less back where we started. I think it's important that KDE waste becomes visible in the OS X Trash, preferably in a subdirectory. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.)
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... René J.V. Bertin wrote: Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. David Faure wrote: mkdir will fail with errno==EEXIST if a dir (or file) already exists with the same name, anyway (no matter what the permissions and ownership are). I just don't like that this code can use either trashPath or trashPath+/KDE.trash, and has to handle both cases everywhere, including hacks like if endsWith(/KDE.trash). We should pick one way and stick to it, anything else increases complexity and therefore the risk of bugs. remove the subdirectory we couldn't create and use the standard KDE infrastructure is weird if you have decided that the KDE infrastructure on OS X *is* trashdir+/KDE.trash. René J.V. Bertin wrote: Point taken. Now the question is, how am I going to implement a function that (re)creates the infrastructure if it has been deleted? Is trashimpl.h part of the external API, i.e. can I add a function to TrashImpl - and would that have to be a function available everywhere (but a stub on all but OS X), or a function that only exists on OS X? Thomas Lübking wrote: http://api.kde.org/frameworks-api/frameworks5-apidocs/kio/html/classTrashImpl.html On the $TRASH/KDE.trash issue and in general: OS_X stores trashed files directly in ~/.Trash, I take? In that case i foresee a general issue (maybe academic, but possible) with the approach to store the KDE trash stuff there (which OSX will take as trashed files). What happens if you delete a file named info or files or KDE.trash? In either case you'd likely run into a conflict? Ie. either OS_X cannot trash the new file (for there's a directory of that name) or OS_X overrides/invalidates the entire KDE trash or OS_X creates such file and you can't add the KDE stuff there. Yesno? In case: how does the OS_X trash handle hidden (.*) files? Assuming it doesn't put hidden files dotted into the trash, how does it react when adding a hidden file there (whether .info, .files or .KDE.trash)? René J.V. Bertin wrote: In fact, it works exactly the way the KDE trash works, except for the restore info which (I presume) is stored in the file metadata. So: - deleting a file that has the same name as a file already in the trash will 1) rename that file to something like name 1 and 2) move that file into the trash. (I've been able to see that happen when the system is very busy). In short, it's only if someone already put a KDE.trash in the trash that we'd into trouble, unless it's a directory. - Why wouldn't it put hidden files there? Again, the Finder works like Dolphin or Nautilus: hidden files are nothing special, they're just not shown in the Finder (nor in file selection dialogs). I'm not sure if the trash would show as non-empty if we store everything under ~/.Trash/.KDE.trash but if it does we have the problem that the user won't understand why when s/he opens the Trash in the Finder. And if it doesn't we're more or less back where we started. I think it's important that KDE waste becomes visible in the OS X Trash, preferably in a subdirectory. Thomas Lübking wrote: Why wouldn't it put hidden files there? Counterfeit - there's only limited reason to hide a file in the trash (the kioslave simply shows them), so I could have assumed it's renamed .foo - _.foo or so. Does OS_X consider an empty directory in ~/.Trash a non-empty trash (notably if it has no meta/restorage data)? The ideal solution to me seems to ensure there's always OUR (we can hardly re-use a directory the user deleted there
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 854 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line854 deleteEmptyTrashInfraStructure is implemented on all OSes, but only called on Mac, which seems a bit inconsistent. I looked at the trash spec again, and given the special permissions required on trash dirs in other partitions (DIR/.Trash or DIR/.Trash-$uid), I would feel safer if we didn't delete trash infrastructure. So I would make the entire method OSX only. BTW you should use Q_OS_OSX instead of Q_OS_MAC. iOS for sure doesn't work this way. René J.V. Bertin wrote: KDE on iOS, seriously? Even if so, I'd hope Qt don't use Q_OS_MAC for that, because 1 MAC as in Macintosh refers to a line of desktop and laptop computers, not the iDevices 2 iOS is in fact an embedded form of OS X René J.V. Bertin wrote: Re: iOS: here's how Qt5 defines the platform tokens: ```C++ #if defined(Q_OS_DARWIN) # define Q_OS_MAC # if defined(Q_OS_DARWIN64) # define Q_OS_MAC64 # elif defined(Q_OS_DARWIN32) # define Q_OS_MAC32 # endif # include TargetConditionals.h # if defined(TARGET_OS_IPHONE) TARGET_OS_IPHONE # define Q_OS_IOS # elif defined(TARGET_OS_MAC) TARGET_OS_MAC # define Q_OS_OSX # define Q_OS_MACX // compatibility synonym # endif #endif ``` David Faure wrote: Yes, so Q_OS_MAC means OSX and iOS, while this code is OSX only and won't work on iOS, hence my suggestion to use Q_OS_OSX. In fact, Q_OS_OSX was introduced by Jake Petroules in Qt 5.2, so doesn't exist in Qt 4.8 ... - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 15, 2014, 6:26 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 17, 2014, 12:35 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Changes --- New version taking into account the various suggestions. Trash infrastructure is now created only when really needed (please check if I forgot any cases), which does NOT include scanning the trash nor doing something with a trashed file (knowing a trashed file by name should mean the infrastruct. exists). Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 9:35 p.m., Thomas Lübking wrote: kioslave/trash/kcmtrash.cpp, line 220 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318518#file318518line220 don't know about the exotic OS policy on this, but changes to i18n strings need to happen for new minors only (unless you get an exception), ie. for 4.15 René J.V. Bertin wrote: Would it be better if I made this a regular string, with something like a // TODO: internationalise ? David Faure wrote: No that's worse, it doesn't even leave a chance for the translators to translate it. Since this is mac only I wouldn't worry too much about it though. Understood. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68412 --- On Oct. 14, 2014, 1:59 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 1:59 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 9:35 p.m., Thomas Lübking wrote:
kioslave/trash/trashimpl.cpp, line 362
https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line362
This can only make sense when
a) init() created $TRASH/KDE.trash
b) deleteEmptyTrashInfraStructure() kicked it
c) at some later point $TRASH/KDE.trash is no more writable
- add it to ::copyToTrash() and ::moveToTrash() or ::adaptTrashSize() ?
Also, you might want to try returning trashPath to $TRASH/KDE.trash
when the trash was so far empty?
I think it does make sense. The KDE.trash/{files,info} infrastructure can be
deleted not only when the KDE wastebin is emptied one way or another, but also
when the OS X Trash is emptied through the Finder.
I think the best way to test for its presence and recreate it if necessary is
in the functions that return the name of the infrastructural subdir.
I don't understand your Also remark, please explain?
- René J.V.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/#review68412
---
On Oct. 14, 2014, 1:59 p.m., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/
---
(Updated Oct. 14, 2014, 1:59 p.m.)
Review request for KDE Software on Mac OS X, KDE Runtime and David Faure.
Repository: kde-runtime
Description
---
KDE on OS X does not handle the desktop session (no Plasma) nor can it rely
on XDG to obtain the proper paths to use for something like the trash. As a
result, all applications that propose to move things they manage to the
wastebin (Dolphin, but also digiKam) will store those items in a place that
has no particular meaning on OS X, and that will thus tend to fill up.
OS X stores trash in one of several locations. Files trashed from the boot
volume (and/or the volume containing $HOME, I don't actually know that) end
up in `~/.Trash`. Files deleted from other volumes end up in
`/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless
whether it's an external or a remote drive; only mounted NFS shares are
handled differently) and uid the numerical user id. Permissions on `.Trashes`
are the same as those expected by KDE.
The kio_trash kioslave appears to support several actual trash directory
locations, just like OS X. `TrashImpl::init()` creates a standard trash in
`~/.local/share/Trash` (at least under OS X) but also
`TrashImpl::trashForMountPoint()` that is used in cases I have not yet
encountered.
On OS X, my modified `TrashImpl::init()` sets the standard trash directory to
`~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as
required, because they will of course be deleted when the user empties the OS
X trash. `TrashImpl::fileRemoved()` has been modified to call a new function,
`deleteEmptyTrashInfraStructure` to delete the KDE trash's internal
infrastructure when the wastebin is empty so that OS X also sees the trash as
emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature
actually works, as expected as far as I can tell).
Remains to be done:
- determine in what cases `trashForMountPoint()` is used, and finish the
modifications for it to use `/.Trashes/uid/KDE.trash`
Diffs
-
kioslave/trash/kcmtrash.cpp f4811fd
kioslave/trash/trashimpl.h bc68723
kioslave/trash/trashimpl.cpp 30ee05b
Diff: https://git.reviewboard.kde.org/r/120573/diff/
Testing
---
On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested
actions are
- move items to wastebin from $HOME and a directory on a different volume
- restore items to both places
- empty wastebin through Dolphin
- empty OS X trashcan
Thanks,
René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 854 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line854 deleteEmptyTrashInfraStructure is implemented on all OSes, but only called on Mac, which seems a bit inconsistent. I looked at the trash spec again, and given the special permissions required on trash dirs in other partitions (DIR/.Trash or DIR/.Trash-$uid), I would feel safer if we didn't delete trash infrastructure. So I would make the entire method OSX only. BTW you should use Q_OS_OSX instead of Q_OS_MAC. iOS for sure doesn't work this way. KDE on iOS, seriously? Even if so, I'd hope Qt don't use Q_OS_MAC for that, because 1 MAC as in Macintosh refers to a line of desktop and laptop computers, not the iDevices 2 iOS is in fact an embedded form of OS X - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 14, 2014, 1:59 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 1:59 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 984 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line984 Damn, if only we had known, we could have made the XDG spec closer to the OSX implementation just by adding es :-) Heh, yes, I think there are a few other similar points where parallel evolution would have been beneficial (rather than convergent evolution) ... ;) - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 14, 2014, 1:59 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 1:59 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 1043 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ... - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 14, 2014, 1:59 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 1:59 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 170 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line170 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... Modified as suggested. I agree that the error shouldn't occur. Normally it *cannot* occur for the reason you indicate unless another user wrote an entry in this user's Trash explicitly and by hand. However I'm not sure how KDE_mkdir handles a situation in which a (read-only) _file_ of the same name is already present, owned by the same user. While that is unlikely it's not entirely impossible either. On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 351 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line351 this comment doesn't match the code in the macro, which merely concatenates two strings, it doesn't check anything nor create anything. On that note, macros expanding to code are not great... functions are much better. Or in this case, it's a two-liner used twice, I would just inline (duplicate) the code. Oh yes, the macro checks something, and creates it if it doesn't exist - or should I say that the `testDir` function called by the macro does that? Understood that duplicating the code is acceptable here. On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 382 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line382 same as above, why do we want this fallback? What else would we want to do? I haven't seen provisions to move only the file and not create the `.trashinfo` entry that contains the relevant meta-data. I'm inclined to try as hard/much as possible to achieve normal behaviour, and have to tell the user that we couldn't move an item to the wastebin. On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 854 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line854 deleteEmptyTrashInfraStructure is implemented on all OSes, but only called on Mac, which seems a bit inconsistent. I looked at the trash spec again, and given the special permissions required on trash dirs in other partitions (DIR/.Trash or DIR/.Trash-$uid), I would feel safer if we didn't delete trash infrastructure. So I would make the entire method OSX only. BTW you should use Q_OS_OSX instead of Q_OS_MAC. iOS for sure doesn't work this way. René J.V. Bertin wrote: KDE on iOS, seriously? Even if so, I'd hope Qt don't use Q_OS_MAC for that, because 1 MAC as in Macintosh refers to a line of desktop and laptop computers, not the iDevices 2 iOS is in fact an embedded form of OS X Re: iOS: here's how Qt5 defines the platform tokens: ```C++ #if defined(Q_OS_DARWIN) # define Q_OS_MAC # if defined(Q_OS_DARWIN64) # define Q_OS_MAC64 # elif defined(Q_OS_DARWIN32) # define Q_OS_MAC32 # endif # include TargetConditionals.h # if defined(TARGET_OS_IPHONE) TARGET_OS_IPHONE # define Q_OS_IOS # elif defined(TARGET_OS_MAC) TARGET_OS_MAC # define Q_OS_OSX # define Q_OS_MACX // compatibility synonym # endif #endif ``` On Oct. 14, 2014, 11:13 p.m., David Faure wrote: kioslave/trash/trashimpl.cpp, line 1043 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line1043 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. René J.V. Bertin wrote: Point(s) taken. For now I still don't know in what circumstances trashForMountPoint is called/used. Once that figured out the new debug statements can go altogether ... I'm keeping the Q_OS_MAC except for the last debug statement, as a reminder to remove them when `trashForMountPoint` has been taken care of. Unless it's a remnant from the past that's no longer being used? - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- On Oct. 14, 2014, 1:59 p.m., René J.V. Bertin wrote:
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 15, 2014, 6:26 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Okt. 14, 2014, 7:35 nachm., Thomas Lübking wrote:
kioslave/trash/trashimpl.cpp, line 362
https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line362
This can only make sense when
a) init() created $TRASH/KDE.trash
b) deleteEmptyTrashInfraStructure() kicked it
c) at some later point $TRASH/KDE.trash is no more writable
- add it to ::copyToTrash() and ::moveToTrash() or ::adaptTrashSize() ?
Also, you might want to try returning trashPath to $TRASH/KDE.trash
when the trash was so far empty?
René J.V. Bertin wrote:
I think it does make sense. The KDE.trash/{files,info} infrastructure can
be deleted not only when the KDE wastebin is emptied one way or another, but
also when the OS X Trash is emptied through the Finder.
I think the best way to test for its presence and recreate it if
necessary is in the functions that return the name of the infrastructural
subdir.
I don't understand your Also remark, please explain?
I was rather pointing out when such test and recreation would be actually
required and that should be when you attempt to add files to a (so far) empty
(KDE) trash (why I suggested to move the test/creation there)
By the Also sentence, I reffered to the fact that you're conditionally
chopping the trashpath from $TRASH/KDE.trash to $TRASH if, for whatever
reason, $TRASH/KDE.trash originally could be created but no longer can be.
I assume it therefore would also make sense to attempt to try to create
trashDir + /KDE.trash (and set trashDir to trashDir + KDE.trash in case
of success) if the trash is empty (you're actually attempting to create a
trashDir) and trashDir is currently $TRASH and not $TRASH/KDE.trash
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/#review68412
---
On Okt. 15, 2014, 4:26 nachm., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/
---
(Updated Okt. 15, 2014, 4:26 nachm.)
Review request for KDE Software on Mac OS X, KDE Runtime and David Faure.
Repository: kde-runtime
Description
---
KDE on OS X does not handle the desktop session (no Plasma) nor can it rely
on XDG to obtain the proper paths to use for something like the trash. As a
result, all applications that propose to move things they manage to the
wastebin (Dolphin, but also digiKam) will store those items in a place that
has no particular meaning on OS X, and that will thus tend to fill up.
OS X stores trash in one of several locations. Files trashed from the boot
volume (and/or the volume containing $HOME, I don't actually know that) end
up in `~/.Trash`. Files deleted from other volumes end up in
`/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless
whether it's an external or a remote drive; only mounted NFS shares are
handled differently) and uid the numerical user id. Permissions on `.Trashes`
are the same as those expected by KDE.
The kio_trash kioslave appears to support several actual trash directory
locations, just like OS X. `TrashImpl::init()` creates a standard trash in
`~/.local/share/Trash` (at least under OS X) but also
`TrashImpl::trashForMountPoint()` that is used in cases I have not yet
encountered.
On OS X, my modified `TrashImpl::init()` sets the standard trash directory to
`~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as
required, because they will of course be deleted when the user empties the OS
X trash. `TrashImpl::fileRemoved()` has been modified to call a new function,
`deleteEmptyTrashInfraStructure` to delete the KDE trash's internal
infrastructure when the wastebin is empty so that OS X also sees the trash as
emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature
actually works, as expected as far as I can tell).
Remains to be done:
- determine in what cases `trashForMountPoint()` is used, and finish the
modifications for it to use `/.Trashes/uid/KDE.trash`
Diffs
-
kioslave/trash/kcmtrash.cpp f4811fd
kioslave/trash/trashimpl.h bc68723
kioslave/trash/trashimpl.cpp 30ee05b
Diff: https://git.reviewboard.kde.org/r/120573/diff/
Testing
---
On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested
actions are
- move items to wastebin from $HOME and a directory on a different volume
- restore items to both places
- empty wastebin through Dolphin
- empty OS X trashcan
Thanks,
René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 10:38 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 1:59 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Changes --- Adds an indication that the wastebin uses an OS X backend to kcm_trash Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 9:35 p.m., Thomas Lübking wrote: kioslave/trash/kcmtrash.cpp, line 220 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318518#file318518line220 don't know about the exotic OS policy on this, but changes to i18n strings need to happen for new minors only (unless you get an exception), ie. for 4.15 Would it be better if I made this a regular string, with something like a // TODO: internationalise ? On Oct. 14, 2014, 9:35 p.m., Thomas Lübking wrote: kioslave/trash/trashimpl.cpp, line 816 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318520#file318520line816 I don't understand why this cleanup is required on osx, but not other systems? On other systems, there's only a single trash at a time, and it's rendered to the user via kde-runtime. Here we're putting that KDE wastebin inside another one - think of it as an ashtray inside a larger garbage can. KDE only knows the ashtray, but OS X sees the whole garbage ... so in order to play nice KDE has to get rid of the ashtray too, each time it empties it. I noticed that there's a win32 backend which seems to use a native API interfaced to TrashImpl. I presume that something similar could be done on OS X (haven't checked) and it's almost certain that I could replace the `files + info` infrastructure by something that uses extended file attributes (resource forks no longer exist...) but TBH I'd rather use the simpler route implemented here. Besides, suppose a user does go into the Finder's Trash to restore a binned file, any trash info stored as a file attribute will remain attached, which isn't very proper. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68412 --- On Oct. 14, 2014, 1:59 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 1:59 p.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68418 --- Nice! Just some comments. kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47678 Shouldn't this return false like the other blocks? And then I would swap the if and else blocks, removing the '!' in the condition... so that all if() blocks follow the same pattern. I see that the code below tries to cope with the case where we couldn't create KDE.trash ... but then we shouldn't set any error code, if we fallback to another solution. However I'm not sure I understand why this could happen though. Why wouldn't we be able to create KDE.trash but we would be able to create info? Well, this would be the case if KDE.trash existed already and was owned by another user, but then the same could happen with info... kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47679 this comment doesn't match the code in the macro, which merely concatenates two strings, it doesn't check anything nor create anything. On that note, macros expanding to code are not great... functions are much better. Or in this case, it's a two-liner used twice, I would just inline (duplicate) the code. kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47680 same as above, why do we want this fallback? kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47681 Minor: I'd put it.value() into a const QString trashDir local variable, to avoid repeating it 4 times, and to improve readability. kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47682 deleteEmptyTrashInfraStructure is implemented on all OSes, but only called on Mac, which seems a bit inconsistent. I looked at the trash spec again, and given the special permissions required on trash dirs in other partitions (DIR/.Trash or DIR/.Trash-$uid), I would feel safer if we didn't delete trash infrastructure. So I would make the entire method OSX only. BTW you should use Q_OS_OSX instead of Q_OS_MAC. iOS for sure doesn't work this way. kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47686 Damn, if only we had known, we could have made the XDG spec closer to the OSX implementation just by adding es :-) kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47683 Not much reason to have an ifdef mac around a kDebug. Remove it all, or enable the kDebug everywhere. kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47684 same kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47685 such a debug statement is more useful if it prints out the input to the method, i.e. topdir. - David Faure On Oct. 14, 2014, 11:59 a.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 11:59 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 14, 2014, 7:35 p.m., Thomas Lübking wrote: kioslave/trash/kcmtrash.cpp, line 220 https://git.reviewboard.kde.org/r/120573/diff/6/?file=318518#file318518line220 don't know about the exotic OS policy on this, but changes to i18n strings need to happen for new minors only (unless you get an exception), ie. for 4.15 René J.V. Bertin wrote: Would it be better if I made this a regular string, with something like a // TODO: internationalise ? No that's worse, it doesn't even leave a chance for the translators to translate it. Since this is mac only I wouldn't worry too much about it though. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68412 --- On Oct. 14, 2014, 11:59 a.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 14, 2014, 11:59 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and David Faure. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/kcmtrash.cpp f4811fd kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 7:32 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Changes --- last-minute changes inspired by writing up the RR description. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68315 --- kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47629 QFile::remove() does not work with directories. You should use QDir().rmdir() instead, which only works as long as the directory is empty though. - Lamarque Souza On Oct. 13, 2014, 5:32 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 5:32 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 13, 2014, 6:03 p.m., Lamarque Souza wrote: kioslave/trash/trashimpl.cpp, line 815 https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line815 QFile::remove() does not work with directories. You should use QDir().rmdir() instead, which only works as long as the directory is empty though. in Qt5, there is a QDir::removeRecursively though. In Qt4/KDE4, you'll need to use e.g. KIO::del or similar. - Milian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68315 --- On Oct. 13, 2014, 5:32 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 5:32 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 13, 2014, 8:03 p.m., Lamarque Souza wrote: kioslave/trash/trashimpl.cpp, line 815 https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line815 QFile::remove() does not work with directories. You should use QDir().rmdir() instead, which only works as long as the directory is empty though. Milian Wolff wrote: in Qt5, there is a QDir::removeRecursively though. In Qt4/KDE4, you'll need to use e.g. KIO::del or similar. Duly noted - does that mean that the corresponding (and existing) statements in `createInfo` and `deleteInfo` (lines 311 and 392) are in error too?! Sadly this doesn't explain everything: it's as if `isEmpty` is not called, in fact: the kDebug() statement on line 811 is never printed. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68315 --- On Oct. 13, 2014, 7:32 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 7:32 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68318 --- kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47632 why do you try to *delete* something in the check that should figure out if something is empty? that sounds very wrong to me. - Milian Wolff On Oct. 13, 2014, 5:32 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 5:32 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 13, 2014, 8:16 p.m., Milian Wolff wrote: kioslave/trash/trashimpl.cpp, line 812 https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line812 why do you try to *delete* something in the check that should figure out if something is empty? that sounds very wrong to me. The goal is explained in the comment: delete an empty directory infrastructure that no longer has a need to exist because the wastebin is empty, in order to empty the hosting trashcan. Yes, it gives a side-effect to the function, but one very much related to its original function - one could say that for isEmpty to return true, said infrastructure should have been deleted. I have no objection to adding an additional function that handles this task, but it would have to be called from several locations (after isEmpty returned true), and also loop over all known trashes. Adding a few lines to isEmpty adds less overhead, and with proper comment doesn't increase maintainance cost, IMHO. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68318 --- On Oct. 13, 2014, 7:32 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 7:32 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68322 --- kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47637 != 0, bonus points if there's sth. like != Success kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47638 why dI assume you want to remove a *trailing* /KDE.trash? I would not call my login like this, but you never know ;-) Also: you exit on ::init() if this path cannot be created - so why are the alternative paths not attempted there and is handling the failed path anywhere else actually reasonable (ie. does the kioslave actually work under this condition?) - Thomas Lübking On Okt. 13, 2014, 5:32 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Okt. 13, 2014, 5:32 nachm.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Okt. 13, 2014, 6:16 nachm., Milian Wolff wrote: kioslave/trash/trashimpl.cpp, line 812 https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line812 why do you try to *delete* something in the check that should figure out if something is empty? that sounds very wrong to me. René J.V. Bertin wrote: The goal is explained in the comment: delete an empty directory infrastructure that no longer has a need to exist because the wastebin is empty, in order to empty the hosting trashcan. Yes, it gives a side-effect to the function, but one very much related to its original function - one could say that for isEmpty to return true, said infrastructure should have been deleted. I have no objection to adding an additional function that handles this task, but it would have to be called from several locations (after isEmpty returned true), and also loop over all known trashes. Adding a few lines to isEmpty adds less overhead, and with proper comment doesn't increase maintainance cost, IMHO. You want to add this to ::fileRemoved() i'd say (cause this is when the state will change), eventually on ::init() - in case that's required to catch trash emptying via safari. Random side effects on a state queries are really bad style, so please don't do such (unless required, ie. you're hacking into other libs ;-) - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68318 --- On Okt. 13, 2014, 5:32 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Okt. 13, 2014, 5:32 nachm.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 13, 2014, 8:16 p.m., Milian Wolff wrote:
kioslave/trash/trashimpl.cpp, line 812
https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line812
why do you try to *delete* something in the check that should figure
out if something is empty? that sounds very wrong to me.
René J.V. Bertin wrote:
The goal is explained in the comment: delete an empty directory
infrastructure that no longer has a need to exist because the wastebin is
empty, in order to empty the hosting trashcan.
Yes, it gives a side-effect to the function, but one very much related to
its original function - one could say that for isEmpty to return true, said
infrastructure should have been deleted.
I have no objection to adding an additional function that handles this
task, but it would have to be called from several locations (after isEmpty
returned true), and also loop over all known trashes. Adding a few lines to
isEmpty adds less overhead, and with proper comment doesn't increase
maintainance cost, IMHO.
Thomas Lübking wrote:
You want to add this to ::fileRemoved() i'd say (cause this is when the
state will change), eventually on ::init() - in case that's required to catch
trash emptying via safari.
Random side effects on a state queries are really bad style, so please
don't do such (unless required, ie. you're hacking into other libs ;-)
And there I was under the impression that I *am* hacking into (an)other('s) lib
O:-)
- René J.V.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/#review68318
---
On Oct. 13, 2014, 7:32 p.m., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/
---
(Updated Oct. 13, 2014, 7:32 p.m.)
Review request for KDE Software on Mac OS X and KDE Runtime.
Repository: kde-runtime
Description
---
KDE on OS X does not handle the desktop session (no Plasma) nor can it rely
on XDG to obtain the proper paths to use for something like the trash. As a
result, all applications that propose to move things they manage to the
wastebin (Dolphin, but also digiKam) will store those items in a place that
has no particular meaning on OS X, and that will thus tend to fill up.
OS X stores trash in one of several locations. Files trashed from the boot
volume (and/or the volume containing $HOME, I don't actually know that) end
up in `~/.Trash`. Files deleted from other volumes end up in
`/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless
whether it's an external or a remote drive; only mounted NFS shares are
handled differently) and uid the numerical user id. Permissions on `.Trashes`
are the same as those expected by KDE.
The kio_trash kioslave appears to support several actual trash directory
locations, just like OS X. `TrashImpl::init()` creates a standard trash in
`~/.local/share/Trash` (at least under OS X) but also
`TrashImpl::trashForMountPoint()` that is used in cases I have not yet
encountered.
On OS X, my modified `TrashImpl::init()` sets the standard trash directory to
`~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as
required, because they will of course be deleted when the user empties the OS
X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's
internal infrastructure when the wastebin is empty so that OS X also see the
trash as emptied, but that doesn't work yet.
Remains to be done:
- figure out why `isEmpty()` doesn't completely clean out the trash as
expected which causes OS X to show the trash as full even after emptying the
wastebin through KDE.
- determine in what cases `trashForMountPoint()` is used, and finish the
modifications for it to use `/.Trashes/uid/KDE.trash`
Diffs
-
kioslave/trash/trashimpl.cpp 30ee05b
Diff: https://git.reviewboard.kde.org/r/120573/diff/
Testing
---
On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested
actions are
- move items to wastebin from $HOME and a directory on a different volume
- restore items to both places
- empty wastebin through Dolphin
- empty OS X trashcan
Thanks,
René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Okt. 13, 2014, 6:16 nachm., Milian Wolff wrote:
kioslave/trash/trashimpl.cpp, line 812
https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line812
why do you try to *delete* something in the check that should figure
out if something is empty? that sounds very wrong to me.
René J.V. Bertin wrote:
The goal is explained in the comment: delete an empty directory
infrastructure that no longer has a need to exist because the wastebin is
empty, in order to empty the hosting trashcan.
Yes, it gives a side-effect to the function, but one very much related to
its original function - one could say that for isEmpty to return true, said
infrastructure should have been deleted.
I have no objection to adding an additional function that handles this
task, but it would have to be called from several locations (after isEmpty
returned true), and also loop over all known trashes. Adding a few lines to
isEmpty adds less overhead, and with proper comment doesn't increase
maintainance cost, IMHO.
Thomas Lübking wrote:
You want to add this to ::fileRemoved() i'd say (cause this is when the
state will change), eventually on ::init() - in case that's required to catch
trash emptying via safari.
Random side effects on a state queries are really bad style, so please
don't do such (unless required, ie. you're hacking into other libs ;-)
René J.V. Bertin wrote:
And there I was under the impression that I *am* hacking into
(an)other('s) lib O:-)
Nah... that's not *my* definition of hacking :-P
You'd do so when eg. bringing a plugin and want to sneak your way into the
process by abusing a function that will be called for sure to cause sth.
entirely different (because there's no other way to impact the binary behavior)
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/#review68318
---
On Okt. 13, 2014, 5:32 nachm., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120573/
---
(Updated Okt. 13, 2014, 5:32 nachm.)
Review request for KDE Software on Mac OS X and KDE Runtime.
Repository: kde-runtime
Description
---
KDE on OS X does not handle the desktop session (no Plasma) nor can it rely
on XDG to obtain the proper paths to use for something like the trash. As a
result, all applications that propose to move things they manage to the
wastebin (Dolphin, but also digiKam) will store those items in a place that
has no particular meaning on OS X, and that will thus tend to fill up.
OS X stores trash in one of several locations. Files trashed from the boot
volume (and/or the volume containing $HOME, I don't actually know that) end
up in `~/.Trash`. Files deleted from other volumes end up in
`/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless
whether it's an external or a remote drive; only mounted NFS shares are
handled differently) and uid the numerical user id. Permissions on `.Trashes`
are the same as those expected by KDE.
The kio_trash kioslave appears to support several actual trash directory
locations, just like OS X. `TrashImpl::init()` creates a standard trash in
`~/.local/share/Trash` (at least under OS X) but also
`TrashImpl::trashForMountPoint()` that is used in cases I have not yet
encountered.
On OS X, my modified `TrashImpl::init()` sets the standard trash directory to
`~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as
required, because they will of course be deleted when the user empties the OS
X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's
internal infrastructure when the wastebin is empty so that OS X also see the
trash as emptied, but that doesn't work yet.
Remains to be done:
- figure out why `isEmpty()` doesn't completely clean out the trash as
expected which causes OS X to show the trash as full even after emptying the
wastebin through KDE.
- determine in what cases `trashForMountPoint()` is used, and finish the
modifications for it to use `/.Trashes/uid/KDE.trash`
Diffs
-
kioslave/trash/trashimpl.cpp 30ee05b
Diff: https://git.reviewboard.kde.org/r/120573/diff/
Testing
---
On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested
actions are
- move items to wastebin from $HOME and a directory on a different volume
- restore items to both places
- empty wastebin through Dolphin
- empty OS X trashcan
Thanks,
René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 13, 2014, 9:31 p.m., Thomas Lübking wrote: kioslave/trash/trashimpl.cpp, line 363 https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line363 why dI assume you want to remove a *trailing* /KDE.trash? I would not call my login like this, but you never know ;-) Also: you exit on ::init() if this path cannot be created - so why are the alternative paths not attempted there and is handling the failed path anywhere else actually reasonable (ie. does the kioslave actually work under this condition?) You're right about the ::init() exit, that was a copy/paste oversight. Is there something like QString -= extension or do I have to make something up myself with lastIndexOf and whatsmore? - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68322 --- On Oct. 13, 2014, 7:32 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 7:32 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Okt. 13, 2014, 7:31 nachm., Thomas Lübking wrote: kioslave/trash/trashimpl.cpp, line 363 https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line363 why dI assume you want to remove a *trailing* /KDE.trash? I would not call my login like this, but you never know ;-) Also: you exit on ::init() if this path cannot be created - so why are the alternative paths not attempted there and is handling the failed path anywhere else actually reasonable (ie. does the kioslave actually work under this condition?) René J.V. Bertin wrote: You're right about the ::init() exit, that was a copy/paste oversight. Is there something like QString -= extension or do I have to make something up myself with lastIndexOf and whatsmore? if (trashPath.endsWith(QLatin1String(/KDE.trash))) trashPath.chop(10); - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68322 --- On Okt. 13, 2014, 5:32 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Okt. 13, 2014, 5:32 nachm.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 10:42 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Changes --- Takes the feedback until now into account (issues marked as closed). Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 13, 2014, 8:03 p.m., Lamarque Souza wrote: kioslave/trash/trashimpl.cpp, line 815 https://git.reviewboard.kde.org/r/120573/diff/2/?file=318230#file318230line815 QFile::remove() does not work with directories. You should use QDir().rmdir() instead, which only works as long as the directory is empty though. Milian Wolff wrote: in Qt5, there is a QDir::removeRecursively though. In Qt4/KDE4, you'll need to use e.g. KIO::del or similar. René J.V. Bertin wrote: Duly noted - does that mean that the corresponding (and existing) statements in `createInfo` and `deleteInfo` (lines 311 and 392) are in error too?! Sadly this doesn't explain everything: it's as if `isEmpty` is not called, in fact: the kDebug() statement on line 811 is never printed. I saw that the class already had a `synchronousDel` function. This function could receive an overload to accept a `QStringList` argument that will allow to schedule the 2 or 3 subdirectories to be deleted with a single call to synchronousDel instead of with 3 consecutive calls. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68315 --- On Oct. 13, 2014, 10:42 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 10:42 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68331 --- kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47650 nothing actuallly changed here - if this failed, the following testDir( trashDir + QString::fromLatin1(/info) ) will fail and init returns false. From the other code, I assume you'd want to chop trashDir here and then won't require any code in infoPath/filePath ? kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47649 *cough* kioslave/trash/trashimpl.cpp https://git.reviewboard.kde.org/r/120573/#comment47651 comment seems wrong now. - Thomas Lübking On Okt. 13, 2014, 8:42 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Okt. 13, 2014, 8:42 nachm.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::isEmpty()` has been modified to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also see the trash as emptied, but that doesn't work yet. Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 11:02 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Changes --- Updated the description to take the new situation into account Repository: kde-runtime Description (updated) --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - figure out why `isEmpty()` doesn't completely clean out the trash as expected which causes OS X to show the trash as full even after emptying the wastebin through KDE. - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 11:09 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Changes --- Can someone please comment on the what/why/when of `trashForMountPoint`? Repository: kde-runtime Description (updated) --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
On Oct. 13, 2014, 10:59 p.m., Thomas Lübking wrote: kioslave/trash/trashimpl.cpp, line 168 https://git.reviewboard.kde.org/r/120573/diff/3/?file=318241#file318241line168 nothing actuallly changed here - if this failed, the following testDir( trashDir + QString::fromLatin1(/info) ) will fail and init returns false. From the other code, I assume you'd want to chop trashDir here and then won't require any code in infoPath/filePath ? oops, hadn't seen this comment. Your assumption is right, though I prefer to `testdir(trashDir+/KDE.trash)` and then do the `+=` when `!err` (or an `error()` otherwise). I think it's better to skip only the KDE.trash path component (even if it's likely that subsequent subdir creation will fail too) : skipping the `KDE.trash` component has no effect on KDE wastebin functionality, skipping the other components does. `KDE.trash` is only there so that the user knows where those `files` and `info` directories and their contents come from, should s/he open the Trash for whatever reason. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/#review68331 --- On Oct. 13, 2014, 11:09 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 11:09 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
Re: Review Request 120573: [OS X] make KDE's trash use the OS X trash
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120573/ --- (Updated Oct. 13, 2014, 11:20 p.m.) Review request for KDE Software on Mac OS X and KDE Runtime. Repository: kde-runtime Description --- KDE on OS X does not handle the desktop session (no Plasma) nor can it rely on XDG to obtain the proper paths to use for something like the trash. As a result, all applications that propose to move things they manage to the wastebin (Dolphin, but also digiKam) will store those items in a place that has no particular meaning on OS X, and that will thus tend to fill up. OS X stores trash in one of several locations. Files trashed from the boot volume (and/or the volume containing $HOME, I don't actually know that) end up in `~/.Trash`. Files deleted from other volumes end up in `/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless whether it's an external or a remote drive; only mounted NFS shares are handled differently) and uid the numerical user id. Permissions on `.Trashes` are the same as those expected by KDE. The kio_trash kioslave appears to support several actual trash directory locations, just like OS X. `TrashImpl::init()` creates a standard trash in `~/.local/share/Trash` (at least under OS X) but also `TrashImpl::trashForMountPoint()` that is used in cases I have not yet encountered. On OS X, my modified `TrashImpl::init()` sets the standard trash directory to `~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as required, because they will of course be deleted when the user empties the OS X trash. `TrashImpl::fileRemoved()` has been modified to call a new function, `deleteEmptyTrashInfraStructure` to delete the KDE trash's internal infrastructure when the wastebin is empty so that OS X also sees the trash as emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature actually works, as expected as far as I can tell). Remains to be done: - determine in what cases `trashForMountPoint()` is used, and finish the modifications for it to use `/.Trashes/uid/KDE.trash` Diffs (updated) - kioslave/trash/trashimpl.h bc68723 kioslave/trash/trashimpl.cpp 30ee05b Diff: https://git.reviewboard.kde.org/r/120573/diff/ Testing --- On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested actions are - move items to wastebin from $HOME and a directory on a different volume - restore items to both places - empty wastebin through Dolphin - empty OS X trashcan Thanks, René J.V. Bertin
