D11204: Support NTFS hidden files

[Nathaniel Graham]

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, markg, elvisangelaccio, ltoscano, 
anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, bruns

D11204: Support NTFS hidden files

[Mark Gaiser]

markg added a comment.


  In D11204#247248 , @vonreth wrote:
  
  > 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.
  
  
  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.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, dfaure
Cc: vonreth, kossebau, dfaure, markg, elvisangelaccio, ltoscano, 
anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, ngraham, bruns

D11204: Support NTFS hidden files

[Hannah von Reth]

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 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.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, dfaure
Cc: vonreth, kossebau, dfaure, markg, elvisangelaccio, ltoscano, 
anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, ngraham, bruns

D11204: Support NTFS hidden files

[Roman Inflianskas]

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 header is not part of the libc (?) headers.
> 
> So this needs to get some condition checking before using it. Compare other 
> code doing that:
> https://lxr.kde.org/search?_filestring=&_string=xattr.h&_casesensitive=1
> 
> 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.

I'm sorry for breaking the build. Here is the fix: D11716 


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

[Roman Inflianskas]

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

[Friedrich W . H . Kossebau]

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 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:
https://lxr.kde.org/search?_filestring=&_string=xattr.h&_casesensitive=1

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.

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

[Roman Inflianskas]

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

[Roman Inflianskas]

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, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

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 to do it?
  
  
  He can commit, he has done a bunch in Dolphin already :)

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, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[David Faure]

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 would 
> keep this else nice and clean. But i do see why you'd want to put it here... 
> What's your take on this, @dfaure?

Looks fine to me here, due to the file.cpp / file_unix.cpp split.

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

[Mark Gaiser]

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 think that I'm 
> wrong, I can move it.

I "think" it belongs in createUDSEntry, but i'm not sure either. It would keep 
this else nice and clean. But i do see why you'd want to put it here... What's 
your take on this, @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

[Roman Inflianskas]

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 master)

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

AFFECTED FILES
  src/ioslaves/file/file_unix.cpp

To: rominf, #dolphin, #frameworks, markg, dfaure
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[David Faure]

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();
> +auto length = getxattr(filenameEncoded, attrName, nullptr, 0);

Ouch, you can't call data() here, that's keeping a char* pointing to a deleted 
(temporary) QByteArray.
The .data() calls need to be inside the getxattr calls themselves (so that 
filenameEncoded is a QByteArray, which controls the lifetime of the 8bit 
string).

> rominf wrote in file_unix.cpp:422
> No, it's enough for array length to be `constexpr`.

Hmm, that makes a lot of sense, actually :-)

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

[Roman Inflianskas]

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 useful here to avoid an allocation every time

TIL about `QVarLengthArray`. Thank you.

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

[Roman Inflianskas]

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
  https://phabricator.kde.org/D11204

AFFECTED FILES
  src/ioslaves/file/file_unix.cpp

To: rominf, #dolphin, #frameworks, markg, dfaure
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[David Faure]

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 this need to be static, for the next line to be standards compliant? 
Not 100% sure.

> file_unix.cpp:424
> +char strAttr[xattr_size];
> +length = getxattr(filename.toLocal8Bit().data(), attrName, strAttr, 
> xattr_size);
> +if (length <= 0) {

but filename.toLocal8Bit() in a local QByteArray so you don't do this 
conversion twice.
And in fact this should use QFile::encodeName() instead of toLocal8Bit().

> file_unix.cpp:431
> +static const auto digits = "0123456789abcdef";
> +auto hexAttr = new char[length * 2 + 4];
> +char *c = strAttr, *e = hexAttr;

QVarLengthArray might be useful here to avoid an allocation every time

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

[Mark Gaiser]

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

[Mark Gaiser]

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

[Roman Inflianskas]

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 
(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.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

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, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

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 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.

INLINE COMMENTS

> file_unix.cpp:546-550
> +#ifdef Q_OS_LINUX
> +if (isNtfsHidden(filename)) {
> +entry.insert(KIO::UDSEntry::UDS_HIDDEN, 1);
> +}
> +#endif

Why here?
This should be done inside the createUDSEntry function (it's in file.cpp).

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Roman Inflianskas]

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
  src/ioslaves/file/file_unix.cpp

To: rominf, #dolphin, #frameworks, markg
Cc: markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, 
#dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Roman Inflianskas]

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 FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/ioslaves/file/file_unix.cpp

To: rominf, #dolphin, #frameworks, markg
Cc: markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, 
#dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

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.
  
  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 point you to.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg
Cc: markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, 
#dolphin, michaelh

D11204: Support NTFS hidden files

[Kai Uwe Broulik]

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, 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)

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks
Cc: elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, #dolphin, 
michaelh

D11204: Support NTFS hidden files

[Elvis Angelaccio]

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` attribute?
  >
  >
  > 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?
  
  
  You can find it in `kio/src/ioslaves/file`.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks
Cc: elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, #dolphin, 
michaelh

D11204: Support NTFS hidden files

[Luigi Toscano]

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

To: rominf, #dolphin, #frameworks
Cc: ltoscano, anthonyfieroni, broulik, #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

[Roman Inflianskas]

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 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.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks
Cc: anthonyfieroni, broulik, #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

[Roman Inflianskas]

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
  https://phabricator.kde.org/D11204

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h

To: rominf, #dolphin, #frameworks
Cc: anthonyfieroni, broulik, #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

[Anthony Fieroni]

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 this should be done in different way, about me

  std::uint64_t attr;
  length = getxattr(fileName, attrName, , sizeof attr);
  if (length <= 0) {
  return false;
  }

> kfileitem.cpp:1136
> +auto intAttr = static_cast(strtol(hexAttr, nullptr, 16));
> +delete hexAttr;
> +

It should be

  delete[] hexAttr;

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks
Cc: anthonyfieroni, broulik, #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

[Roman Inflianskas]

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 you point me out to appropriate files?

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks
Cc: broulik, #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

[Kai Uwe Broulik]

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), this should be in `KFileItemPrivate` and not a public method, let 
alone one whose availability depends on the platform

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks
Cc: broulik, #frameworks, #dolphin, michaelh

D11204: Support NTFS hidden files

[Roman Inflianskas]

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

[Roman Inflianskas]

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

[Roman Inflianskas]

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 attribute 
HIDDEN of files from NTFS partitions and complements isHidden function.
  FEATURE: 171537

TEST PLAN
  1. Open Dolphin on Windows partition
  2. Check that "$Recycle.Bin", "MSOCache", "System Volume Information", etc. 
are hidden
  3. Show hidden files
  4. Check that Windows hidden files are displayed now

REPOSITORY
  R241 KIO

BRANCH
  ntfs-hidden

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/kfileitem.h

To: rominf, #dolphin
Cc: #frameworks, #dolphin, michaelh