D13782: RFC: Ignore NTFS hidden flag for root volume

[Nathaniel Graham]

ngraham added a comment.


  @broulik? Is it ready to land?

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, yurikoles, bruns
Cc: ngraham, oysteins, wbauer, kde-frameworks-devel, michaelh, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Øystein S . -Alværvik]

oysteins added a comment.


  I cannot speak for the code, but this seems to work perfectly well when I 
tested it today. I used Kate and KBackup to open some files/directories on an 
external NTFS, and used KBackup to write a backup to the drive. I didn't have 
to check 'Show hidden folders' and the device shortcut for the NTFS drive in 
the Open dialog worked as expected.
  I hope this can be pushed soon.

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, yurikoles, bruns
Cc: ngraham, oysteins, wbauer, kde-frameworks-devel, michaelh, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Wolfgang Bauer]

wbauer added a comment.


  In D13782#297639 , @wbauer wrote:
  
  > Using QDir::canonicalPath() (instead of QDir::cleanPath() ) would fix all 3 
cases:
  >
  >   const QString fullFilePath = QDir(path + QLatin1Char('/') + 
filename).canonicalPath();
  >
  
  
  Or rather, as the current directory is set to `path` in line#507, this would 
actually suffice:
  
const QString fullFilePath = QDir(filename).canonicalPath();

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, yurikoles, bruns
Cc: ngraham, oysteins, wbauer, kde-frameworks-devel, michaelh, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Wolfgang Bauer]

wbauer added a comment.


  In D13782#297547 , @wbauer wrote:
  
  > With additional debug output I do see the '.' entry in the root dir still 
marked as hidden (as mentioned in the test plan), and also the '..' entry in 
(first-level) subdirs is hidden.
  
  
  I also found another problem though: symlinks to the mountpoint/NTFS root dir 
are (still) hidden as well (also if they are on different partitions).
  
  Using QDir::canonicalPath() (instead of QDir::cleanPath() ) would fix all 3 
cases:
  
const QString fullFilePath = QDir(path + QLatin1Char('/') + 
filename).canonicalPath();
  
  (that one isn't static...)

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, yurikoles, bruns
Cc: ngraham, oysteins, wbauer, kde-frameworks-devel, michaelh, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Wolfgang Bauer]

wbauer added a subscriber: ngraham.
wbauer added a comment.


  In D13782#296381 , @ngraham wrote:
  
  > @wbauer, does this work for you now?
  
  
  Yes, it does work as expected now here in all cases I tested, and I didn't 
notice problems.
  
  With additional debug output I do see the '.' entry in the root dir still 
marked as hidden (as mentioned in the test plan), and also the '..' entry in 
(first-level) subdirs is hidden.
  This could be fixed e.g. by using QDir::cleanPath(), i.e.:
  
const QString fullFilePath = QDir::cleanPath(path + QLatin1Char('/') + 
filename);
  
  But I'm not sure that is really necessary as those entries are not displayed 
at all anyway (even with hidden files enabled).

INLINE COMMENTS

> file_unix.cpp:575
> +if (ntfsHidden) {
> +entry.insert(KIO::UDSEntry::UDS_HIDDEN, 1);
> +}

This is entry.fastInsert(...) now as of KIO 5.48.0, see 
https://cgit.kde.org/kio.git/commit/?id=7048d259529fd37134e9bc1bc8d3edc3d4ac8ef4
Maybe the patch should be rebased...

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, yurikoles, bruns
Cc: ngraham, oysteins, wbauer, kde-frameworks-devel, michaelh, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Nathaniel Graham]

ngraham resigned from this revision.
ngraham added a comment.


  @wbauer, does this work for you now? Or @bruns? Still cannot test as I have 
no NTFS partitions.

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, yurikoles, bruns
Cc: oysteins, wbauer, kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Kai Uwe Broulik]

broulik added a comment.


  Ping

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, ngraham, yurikoles, bruns
Cc: oysteins, wbauer, kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Kai Uwe Broulik]

broulik updated this revision to Diff 37229.
broulik added a comment.


  Also check for `DT_UNKNOWN`

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13782?vs=36838&id=37229

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

AFFECTED FILES
  src/ioslaves/file/file_unix.cpp

To: broulik, dfaure, ngraham, yurikoles, bruns
Cc: wbauer, kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Jaime Torres Amate]

jtamate added a dependent revision: D13915: Use non deprecated fastInsert in 
file_unix.

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, ngraham, yurikoles, bruns
Cc: wbauer, kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Wolfgang Bauer]

wbauer added inline comments.

INLINE COMMENTS

> file_unix.cpp:566
> +// Bug 392913: NTFS root volume is always "hidden", 
> ignore this
> +if (ep->d_type == DT_DIR) {
> +const QString fullFilePath = path + QLatin1Char('/') 
> + filename;

Changing this to 'if (ep->d_type == DT_DIR || ep->d_type == DT_UNKNOWN) {' 
makes it work in both mentioned cases (internal harddisk mounted on boot via 
fstab, and external USB stick mounted via dolphin/solid) for me.

And hidden directories on the filesystem still are hidden.

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, ngraham, yurikoles, bruns
Cc: wbauer, kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Wolfgang Bauer]

wbauer added a comment.


  In D13782#287336 , @wbauer wrote:
  
  > I tried the patch and it doesn't make a difference here, the mountpoint of 
my NTFS partition is still hidden.
  >  (it's on an internal drive and mounted to /windows/C via fstab in case it 
matters)
  >
  > Additional debug output showed that ep->d_type is not DT_DIR but 0 (which 
would be DT_UNKNOWN IIANM?) in my case.
  
  
  It does seem to matter.
  I just created an NTFS partition on an USB stick, mounted it via dolphin (to 
/run/media/...) and in this case ep->d_type is indeed DT_DIR, the patch works 
as intended in this case.
  
  So maybe you should check for DT_UNKNOWN as well...

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, ngraham, yurikoles, bruns
Cc: wbauer, kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Wolfgang Bauer]

wbauer added a comment.


  I tried the patch and it doesn't make a difference here, the mountpoint of my 
NTFS partition is still hidden.
  (it's on an internal drive and mounted to /windows/C via fstab in case it 
matters)
  
  Additional debug output showed that ep->d_type is not DT_DIR but 0 (which 
would be DT_UNKNOWN IIANM?) in my case.

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, ngraham, yurikoles, bruns
Cc: wbauer, kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Kai Uwe Broulik]

broulik added a reviewer: bruns.

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, ngraham, yurikoles, bruns
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Kai Uwe Broulik]

broulik updated this revision to Diff 36838.
broulik added a comment.


  Only check for folders, a file cannot be a mount, saves querying mount points 
for hidden files

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13782?vs=36837&id=36838

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

AFFECTED FILES
  src/ioslaves/file/file_unix.cpp

To: broulik, dfaure, ngraham, yurikoles
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13782: RFC: Ignore NTFS hidden flag for root volume

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Those are always hidden as far as NTFS is concerned but leads to unwanted 
side-effects of NTFS volumes becoming inaccessible in the UI
  
  BUG: 392913

TEST PLAN
  My NTFS drive in `/media` is visible again
  Inside the folder I get `/media/foo/.` where the mapping fails again. Any 
`QUrl` or KIO resolve magic to fix that?
  No idea this patch is the right approach, just throwing it out for discussion

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/ioslaves/file/file_unix.cpp

To: broulik, dfaure, ngraham, yurikoles
Cc: kde-frameworks-devel, michaelh, ngraham, bruns