This patch implements function isHiddenNtfs which checks extended attribute HIDDEN of files from NTFS partitions and populates UDSEntry with KIO::UDSEntry::UDS_HIDDEN.
FEATURE: 171537
Details
Details
- Reviewers
dfaure markg - Group Reviewers
Dolphin Frameworks - Commits
- R241:d7cce9937d5e: Support NTFS hidden files
- Open Dolphin on Windows partition
- Check that "$Recycle.Bin", "MSOCache", "System Volume Information", etc. are hidden
- Show hidden files
- Check that Windows hidden files are displayed now
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- ntfs-hidden
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Shouldn't that rather go into the file KIO slave so it sets the UDS_HIDDEN attribute?
| src/core/kfileitem.h | ||
|---|---|---|
| 282 ↗ | (On Diff #29137) | If we were to go this route (adding it to KFileItem which I think we shouldn't), this should be in KFileItemPrivate and not a public method, let alone one whose availability depends on the platform |
Comment Actions
I think you are right, but I don't have an idea what "file KIO slave" means. Can you point me out to appropriate files?
| src/core/kfileitem.cpp | ||
|---|---|---|
| 1116–1136 ↗ | (On Diff #29137) | Indeed this should be done in different way, about me std::uint64_t attr;
length = getxattr(fileName, attrName, &attr, sizeof attr);
if (length <= 0) {
return false;
} |
| 1136 ↗ | (On Diff #29137) | It should be delete[] hexAttr; |
| src/core/kfileitem.cpp | ||
|---|---|---|
| 1116–1136 ↗ | (On Diff #29137) | I wish your code to work, but it doesn't. I took an inspiration from attr: http://git.savannah.nongnu.org/cgit/attr.git/tree/tools/getfattr.c#n235 and http://git.savannah.nongnu.org/cgit/attr.git/tree/tools/getfattr.c#n169. |
Comment Actions
I can't comment on the internal implementation, but: instead of checking for
#ifndef Q_OS_WIN
shouldn't you check for the availability of xattr? Think about other platforms too.
Comment Actions
Can you point me out to appropriate files?
Sure, I think you want to add your code to kio/src/ioslaves/file/file_unix.cpp (it already has a Unix-specific file) in the listDir method:
if (ntfshidden) {
entry.insert(KIO::UDSEntry::UDS_HIDDEN, true);
}(Note that this stuff runs out of process so you might need to kill the file.so executable rather than just Dolphin when you test it)
Comment Actions
-2 in my opinion.
KFileItem should know nothing about NTFS.
You already have isHidden in there which should handle it.
If it doesn't then you might have found a bug.
See how m_hidden is filled:
const int hiddenVal = m_entry.numberValue(KIO::UDSEntry::UDS_HIDDEN, -1); m_hidden = hiddenVal == 1 ? Hidden : (hiddenVal == 0 ? Shown : Auto);
If that gives the wrong result then the place that's filling UDS_HIDDEN (in the KIO slave that @broulik pointed you to likely has a bug that could use fixing.
Comment Actions
@dfaure I've been looking over the file.cpp and file_unix.cpp code a bit and i'm rather surprised that UDS_HIDDEN isn't being set at all here. Which makes me wonder, why is the hidden logic missing and how is it working now?
I don't know for the "why", i'm hoping you can share some insight on this?
I do know for the "how"; "KFileItem::isHidden()" is taking care of that. It checks the first character for a dot and returns true if it does (thus hidden for any app that uses KFileItem).
Would it be OK to move this logic from KFileItem::isHidden to the file.cpp side? Imho, that is the right place to check as operating systems apparently have a different way of showing files as hidden.
Note that this will cause regressions. IOSlaves that don't set UDS_HIDDEN will then show the hidden files. That imho is a bug for those respective IOSlaves not for KFileItem.
| src/ioslaves/file/file_unix.cpp | ||
|---|---|---|
| 546–550 ↗ | (On Diff #29475) | Why here? |
| src/ioslaves/file/file_unix.cpp | ||
|---|---|---|
| 546–550 ↗ | (On Diff #29475) | The logic behind this is that getxattr exists only on Linux (https://www.gnu.org/software/gnulib/manual/html_node/getxattr.html#getxattr) and file_unix.cpp is more Linux-y than file.cpp. If you think that I'm wrong, I can move it. |
| src/ioslaves/file/file_unix.cpp | ||
|---|---|---|
| 415 ↗ | (On Diff #29475) | static |
| 422 ↗ | (On Diff #29475) | Doesn't this need to be static, for the next line to be standards compliant? Not 100% sure. |
| 424 ↗ | (On Diff #29475) | but filename.toLocal8Bit() in a local QByteArray so you don't do this conversion twice. |
| 431 ↗ | (On Diff #29475) | QVarLengthArray might be useful here to avoid an allocation every time |
| src/ioslaves/file/file_unix.cpp | ||
|---|---|---|
| 418 ↗ | (On Diff #30468) | Ouch, you can't call data() here, that's keeping a char* pointing to a deleted (temporary) QByteArray. |
| 422 ↗ | (On Diff #29475) | Hmm, that makes a lot of sense, actually :-) |
Comment Actions
Thanks !
Do you have a contributor account for pushing the commit, or do you need someone else to do it?
| src/ioslaves/file/file_unix.cpp | ||
|---|---|---|
| 546–550 ↗ | (On Diff #29475) | Looks fine to me here, due to the file.cpp / file_unix.cpp split. |
| src/ioslaves/file/file_unix.cpp | ||
|---|---|---|
| 40 ↗ | (On Diff #30474) | This unconditional include sadly breaks the build at least on FreeBSD (see https://build.kde.org/view/Frameworks/job/Frameworks%20kio%20kf5-qt5%20FreeBSDQt5.9/164/console ) and other systems where this header is not part of the libc (?) headers. So this needs to get some condition checking before using it. Compare other code doing that: While the xattr.h is already checked for in KIO in the FindACL.cmake file, an explicite check like check_include_files(sys/xattr.h HAVE_SYS_XATTR_H) and passing HAVE_SYS_XATTR_H via config-kioslave-file.h.cmake into file_unix.cpp and checking the condition additionally to or instead of Q_OS_LINUX for the new code should solve this issue properly. |
Comment Actions
The commit broke the build on mac.
11:43:10 /Users/packaging/Craft/BinaryFactory/macos-64-clang/build/kde/frameworks/tier3/kio/work/kio-5.45.0/src/ioslaves/file/file_unix.cpp:421:19: error: no matching function for call to 'getxattr' 11:43:10 auto length = getxattr(filenameEncoded.data(), attrName, nullptr, 0); 11:43:10 ^~~~~~~~ 11:43:10 /usr/include/sys/xattr.h:61:9: note: candidate function not viable: requires 6 arguments, but 4 were provided 11:43:10 ssize_t getxattr(const char *path, const char *name, void *value, size_t size, u_int32_t position, int options); 11:43:10 ^ 11:43:10 /Users/packaging/Craft/BinaryFactory/macos-64-clang/build/kde/frameworks/tier3/kio/work/kio-5.45.0/src/ioslaves/file/file_unix.cpp:427:14: error: no matching function for call to 'getxattr' 11:43:10 length = getxattr(filenameEncoded.data(), attrName, strAttr, xattr_size); 11:43:10 ^~~~~~~~ 11:43:10 /usr/include/sys/xattr.h:61:9: note: candidate function not viable: requires 6 arguments, but 4 were provided 11:43:10 ssize_t getxattr(const char *path, const char *name, void *value, size_t size, u_int32_t position, int options); 11:43:10 ^ 11:43:10 2 errors generated.
Comment Actions
Ha, apparently linux and mac implement the same principle (xattr) but with different arguments (same function names).
Linux docs: http://man7.org/linux/man-pages/man2/getxattr.2.html
Mac docs: https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/getxattr.2.html
Note for the mac one, the link above is to their legacy stuff (found via google). I don't know if they have a "non-legacy" version or completely new alternative.