D11204: Support NTFS hidden files

ngraham added a comment. Mac build fixed with D12365: getxattr takes 6 parameters in macOS REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks, dfaure Cc: ngraham, vonreth, kossebau, dfaure,

D11204: Support NTFS hidden files

markg added a comment. In D11204#247248 , @vonreth wrote: > The commit broke the build on mac. > > 11:43:10

D11204: Support NTFS hidden files

vonreth added a comment. 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

D11204: Support NTFS hidden files

rominf added inline comments. INLINE COMMENTS > kossebau wrote in file_unix.cpp:40 > 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

D11204: Support NTFS hidden files

rominf marked 8 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks, dfaure Cc: kossebau, dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

kossebau added inline comments. INLINE COMMENTS > file_unix.cpp:40 > #include > +#include > #include 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

D11204: Support NTFS hidden files

rominf closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks, dfaure Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

rominf edited the summary of this revision. REPOSITORY R241 KIO BRANCH ntfs-hidden (branched from master) REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks, dfaure Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks,

D11204: Support NTFS hidden files

markg resigned from this revision. markg added a comment. This revision is now accepted and ready to land. In D11204#233997 , @dfaure wrote: > Thanks ! > > Do you have a contributor account for pushing the commit, or do you need someone else

D11204: Support NTFS hidden files

dfaure accepted this revision. dfaure added a comment. Thanks ! Do you have a contributor account for pushing the commit, or do you need someone else to do it? INLINE COMMENTS > markg wrote in file_unix.cpp:546-550 > I "think" it belongs in createUDSEntry, but i'm not sure either. It

D11204: Support NTFS hidden files

markg added inline comments. INLINE COMMENTS > rominf wrote in file_unix.cpp:546-550 > 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

D11204: Support NTFS hidden files

rominf updated this revision to Diff 30474. rominf added a comment. - Consider criticism - Split multiple statements to put one statement on a line REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11204?vs=30468=30474 BRANCH ntfs-hidden (branched from

D11204: Support NTFS hidden files

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > file_unix.cpp:418 > +constexpr auto attrName = "system.ntfs_attrib_be"; > +const auto filenameEncoded = QFile::encodeName(filename).data(); > +

D11204: Support NTFS hidden files

rominf added inline comments. INLINE COMMENTS > dfaure wrote in file_unix.cpp:422 > Doesn't this need to be static, for the next line to be standards compliant? > Not 100% sure. No, it's enough for array length to be `constexpr`. > dfaure wrote in file_unix.cpp:431 > QVarLengthArray might be

D11204: Support NTFS hidden files

rominf updated this revision to Diff 30468. rominf marked 4 inline comments as done. rominf added a comment. - Consider criticism REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11204?vs=29475=30468 BRANCH ntfs-hidden (branched from master) REVISION DETAIL

D11204: Support NTFS hidden files

dfaure requested changes to this revision. dfaure added inline comments. INLINE COMMENTS > file_unix.cpp:415 > +#ifdef Q_OS_LINUX > +bool isNtfsHidden(const QString ) > +{ static > file_unix.cpp:422 > +} > +constexpr size_t xattr_size = 1024; > +char strAttr[xattr_size]; Doesn't

D11204: Support NTFS hidden files

markg added a comment. Ping. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks, markg, dfaure Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

markg added a reviewer: dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks, markg, dfaure Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

rominf added inline comments. INLINE COMMENTS > markg wrote in file_unix.cpp:546-550 > Why here? > This should be done inside the createUDSEntry function (it's in file.cpp). The logic behind this is that `getxattr` exists only on Linux

D11204: Support NTFS hidden files

markg requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks, markg Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks,

D11204: Support NTFS hidden files

markg added a subscriber: dfaure. markg added a comment. @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

D11204: Support NTFS hidden files

rominf updated this revision to Diff 29475. rominf added a comment. - Cleanup REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11204?vs=29473=29475 BRANCH ntfs-hidden REVISION DETAIL https://phabricator.kde.org/D11204 AFFECTED FILES

D11204: Support NTFS hidden files

rominf updated this revision to Diff 29473. rominf added a comment. - Move isNtfsHidden to file ioslave REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11204?vs=29141=29473 BRANCH ntfs-hidden REVISION DETAIL https://phabricator.kde.org/D11204 AFFECTED

D11204: Support NTFS hidden files

markg requested changes to this revision. markg added a comment. This revision now requires changes to proceed. -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.

D11204: Support NTFS hidden files

broulik added a comment. > 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,

D11204: Support NTFS hidden files

elvisangelaccio added a comment. In D11204#222427 , @rominf wrote: > In D11204#222421 , @broulik wrote: > > > Shouldn't that rather go into the `file` KIO slave so it sets the `UDS_HIDDEN`

D11204: Support NTFS hidden files

ltoscano added a comment. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204

D11204: Support NTFS hidden files

rominf added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfileitem.cpp:1116-1136 > Indeed this should be done in different way, about me > > std::uint64_t attr; > length = getxattr(fileName, attrName, , sizeof attr); > if (length <= 0) { > return false; > } I wish

D11204: Support NTFS hidden files

rominf updated this revision to Diff 29141. rominf marked an inline comment as done. rominf added a comment. Use delete[] for array deletion REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11204?vs=29137=29141 BRANCH ntfs-hidden REVISION DETAIL

D11204: Support NTFS hidden files

anthonyfieroni added inline comments. INLINE COMMENTS > kfileitem.cpp:1116-1136 > +constexpr size_t xattr_size = 1024; > +char strAttr[xattr_size]; > +length = getxattr(fileName, attrName, strAttr, xattr_size); > +if (length <= 0) { > +return false; > +} > + Indeed

D11204: Support NTFS hidden files

rominf added a comment. In D11204#222421 , @broulik wrote: > Shouldn't that rather go into the `file` KIO slave so it sets the `UDS_HIDDEN` attribute? I think you are right, but I don't have an idea what "`file` KIO slave" means. Can

D11204: Support NTFS hidden files

broulik added a comment. Shouldn't that rather go into the `file` KIO slave so it sets the `UDS_HIDDEN` attribute? INLINE COMMENTS > kfileitem.h:282 > + */ > +bool isHiddenNtfs() const; > +#endif If we were to go this route (adding it to `KFileItem` which I think we shouldn't),

D11204: Support NTFS hidden files

rominf edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks Cc: #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

rominf edited the summary of this revision. rominf added a reviewer: Frameworks. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11204 To: rominf, #dolphin, #frameworks Cc: #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

rominf created this revision. rominf added a reviewer: Dolphin. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. rominf requested review of this revision. REVISION SUMMARY This patch implements function isHiddenNtfs which checks extended