D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 marked 2 inline comments as done.
brute4s99 added a comment.


  I'll not leave it like this from next time, that's for sure. :p

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[David Faure]

dfaure added a comment.


  Indeed. I assumed it wasn't in your actual commit log (bad idea, for this 
exact reason). Phabricator often shows outdated descriptions compared to the 
commit log (unless people use `arc diff --verbatim`) so I stopped complaining 
about the description Clearly I should have done it anyway :)

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:d6d724b9d4b9: WIP : Fix SFTP Plugin of KIO for Windows 
(authored by brute4s99).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=62254=62255

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  should've removed WIP before landing 

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  yeah I can push! I'm a GSoCer lol

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D22105

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Thanks for your persistence :-)
  
  Do you have commit access? Otherwise can I have your name and email, for the 
git author information?

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D22105

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 marked an inline comment as done.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 62254.
brute4s99 marked an inline comment as done.
brute4s99 added a comment.


  updated!

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=62237=62254

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_sftp.cpp:2028
>  }
> -else if (QT_STAT( QFile::encodeName(sPart),  ) == 0) { // 
> should a very small ".part" be deleted?
> +else if (partFile.exists()) { // should a very small ".part" be 
> deleted?
>  const int size = config()->readEntry("MinimumKeepSize", 
> DEFAULT_MINIMUM_KEEP_SIZE);

Better not do refresh on success, it slows things down for no purpose.

  else {
  partFile.refresh();
  const int size = ...;
  if (partFile.exists() && partFile.size() < size) {
  partFile.remove();
  }
  }

> kio_sftp.cpp:2045
> +}
> +else {
> +receivedFile.setFileTime(dt, 
> QFileDevice::FileModificationTime);

coding style: join with previous line

  } else {

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 edited the summary of this revision.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 62237.
brute4s99 marked 3 inline comments as done.
brute4s99 added a comment.


  updated

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=62187=62237

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> dfaure wrote in kio_sftp.cpp:2257
> This is marked as Done, but it's not Done. In fact it matches my own 
> suggestion above, so I agree with Albert ;-)

so sorry, it seems I updated a previous version of the diff.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Almost there :-)

INLINE COMMENTS

> kio_sftp.cpp:406
> +#ifdef Q_OS_WIN
> +// TODO Check if this works for other OSes too.
> +fileType = QT_STAT_LNK;

Yes, QT_STAT_LNK works on Unix too, we use it in many places in KIO
(and it has the value S_IFLNK)

So I'm pretty sure you can remove this ifdef.

You could also replace S_IFREG with QT_STAT_REG and S_IFDIR with QT_STAT_DIR, 
for consistency.

> kio_sftp.cpp:2032
>  }
> -else if (QT_STAT( QFile::encodeName(sPart),  ) == 0) { // 
> should a very small ".part" be deleted?
> +else if (partFile.exists()) { // should a very small ".part" be 
> deleted?
>  const int size = config()->readEntry("MinimumKeepSize", 
> DEFAULT_MINIMUM_KEEP_SIZE);

At this point we were doing another stat() in order to see how big the part 
file is *after* sftpGet.
You're reusing the old QFileInfo so that won't contain updated data.

You need to call QFileInfo::refresh() first.

> kio_sftp.cpp:2050
> +else {
> +
> receivedFile.setFileTime(partFile.fileTime(QFileDevice::FileAccessTime),
> +QFileDevice::FileAccessTime);

Sorry I gave wrong advice.
The old code was preserving the atime just because the utime() API forces us to 
specify an atime.
But what we want to do here is set the modification time, nothing else.
We don't care about the access time at all.
Please just remove this line.

> albertvaka wrote in kio_sftp.cpp:2257
> You can keep QT_STAT_LNK on every platform and remove the ifdef. That's the 
> point of the abstraction Qt provides.
> 
> For consistency, I would also change the other (S_IFDIR, etc.) to their Qt 
> counterparts.

This is marked as Done, but it's not Done. In fact it matches my own suggestion 
above, so I agree with Albert ;-)

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 62187.
brute4s99 added a comment.


  updated

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=62159=62187

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 marked 6 inline comments as done.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_sftp.cpp:266
>  
> -qCDebug(KIO_SFTP_LOG) << "name=" << name << " instruction=" << 
> instruction << " prompts=" << n;
> +qDebug() << "name=" << name << " instruction=" << instruction << " 
> prompts=" << n;
>  

Use `kdebugsettings` to enable sftp debug output instead of qCDebug->qDebug 
(which adds a lot of noise to the review)

> brute4s99 wrote in kio_sftp.cpp:2050-2051
> it uses buff.st_atime . Since I'm removing use of buff, I'm not sure how to 
> handle this. For now I've commented out setting the file access time 
> instruction for now.

Just use partFile.fileTime(QFileInfoFileAccessTime)

> kio_sftp.cpp:2052
> +// 
> QFileDevice::FileTime(QFileDevice::FileAccessTime));
> +receivedFile.setFileTime(dt, 
> QFileDevice::FileTime(QFileDevice::FileModificationTime));
> +}

You can remove the FileTime() type conversion, just do setFileTime(dt, 
QFileDevice::FileModificationTime)

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> pino wrote in kio_sftp.cpp:2050-2051
> what is this commented code for?

it uses buff.st_atime . Since I'm removing use of buff, I'm not sure how to 
handle this. For now I've commented out setting the file access time 
instruction for now.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> kio_sftp.cpp:2047
> +QString error_msg = receivedFile.errorString();
> +qDebug() << QStringLiteral("Couldn't update modified 
> time : ") << error_msg;
> +}

no need for QStringLiteral here, sending a `const char *` to debug is perfectly 
fine; also, the qDebug must be categorized, just like all the other debug 
outputs

> kio_sftp.cpp:2050-2051
> +else {
> +// 
> receivedFile.setFileTime(QDateTime::fromTime_t(buff.st_atime),
> +// 
> QFileDevice::FileTime(QFileDevice::FileAccessTime));
> +receivedFile.setFileTime(dt, 
> QFileDevice::FileTime(QFileDevice::FileModificationTime));

what is this commented code for?

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: pino, andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Hannah von Reth]

vonreth added a comment.


  Please revert the changes to qCDebug

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, 
sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 62159.
brute4s99 marked 4 inline comments as done.
brute4s99 added a comment.


  udpated

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=61972=62159

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, 
sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_sftp.cpp:1946
>  // check if destination is ok ...
>  QT_STATBUF buff;
> +QFileInfo copyFile(sCopyFile);

Please remove this variable, since you're not calling QT_STAT anymore.

You'll notice that you're still using buff.st_size, which should become 
QFileInfo's size() method instead...

> kio_sftp.cpp:1973
>  }
>  bResume = canResume( buff.st_size );
>  }

Red alert! Red alert! Uninitialized data being used!

> kio_sftp.cpp:2067
>  // check if source is ok ...
>  QT_STATBUF buff;
> +QFileInfo copyFile(sCopyFile);

Same here

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, 
sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> dfaure wrote in kio_sftp.cpp:2037
> Why ReadWrite, if we know it doesn't exist?
> 
> Does setFileTime() even need open() first? I wouldn't have thought so.

`setFileTime()` needs the file to be open. 
Source : https://doc.qt.io/qt-5/qfiledevice.html#setFileTime

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, 
sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 61972.
brute4s99 marked 7 inline comments as done.
brute4s99 added a comment.


  updated wrt new comments. @dfaure please take another look! 

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=61419=61972

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, 
sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_sftp.cpp:1939
>  QT_STATBUF buff;
>  const bool bDestExists = (QT_STAT(QFile::encodeName(sCopyFile), ) 
> != -1);
> +QFileInfo info(sCopyFile);

We're now doing stat() twice, once here, and once in QFileInfo just below.

This could be fixed by using QFileInfo for everything:

  const bool bDestExists = info.exists();

> kio_sftp.cpp:1960
>  if (bMarkPartial && bPartExists && buff.st_size > 0) {
> -if (S_ISDIR(buff.st_mode)) {
> +if(info.isDir()) {
>  errorCode = ERR_DIR_ALREADY_EXIST;

WRONG. Here we were testing the result of stat() on the .part file, see old 
line 1942.

Granted, reusing "buff" didn't make the code very readable

Create a different QFileInfo instance for the .part file, and use it for 
bPartExists and for isDir() here.

> kio_sftp.cpp:2037
> +if (!receivedFile.exists()) {
> +if (!receivedFile.open(QIODevice::ReadWrite | 
> QIODevice::Text)) {
> +QString error_msg = receivedFile.errorString();

Why ReadWrite, if we know it doesn't exist?

Does setFileTime() even need open() first? I wouldn't have thought so.

> kio_sftp.cpp:2042
> +else {
> +receivedFile.open(QIODevice::ReadWrite | 
> QIODevice::Text);
> +
> receivedFile.setFileTime(QDateTime::fromTime_t(buff.st_atime),

Again??

> kio_sftp.cpp:2060
>  QT_STATBUF buff;
>  bool bSrcExists = (QT_STAT(QFile::encodeName(sCopyFile), ) != -1);
> +QFileInfo info(sCopyFile);

= info.exists()

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Hannah von Reth]

vonreth accepted this revision.
vonreth added a comment.
This revision is now accepted and ready to land.


  looks good

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D22105

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Hannah von Reth]

vonreth added a comment.


  I think you need to ```set(CMAKE_CXX_STANDARD 17)``` to make this compile 
with mingw

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> kio_sftp.cpp:29
>  #include 
> -#include 
>  

this builds without `utime.h` on my system (Arch Linux with latest Plasma, Qt 
and other packages), . Please inform if someone else has issues without this 
header.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 61419.
brute4s99 added a comment.


  Updated with `QT_` pre-procs for S_IFDIR, S_IFLNK and others.

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=61225=61419

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> brute4s99 wrote in kio_sftp.cpp:2257
> yes! thanks Hannah! 

You can keep QT_STAT_LNK on every platform and remove the ifdef. That's the 
point of the abstraction Qt provides.

For consistency, I would also change the other (S_IFDIR, etc.) to their Qt 
counterparts.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> vonreth wrote in kio_sftp.cpp:2257
> Does 
> https://code.woboq.org/qt5/qtbase/mkspecs/common/posix/qplatformdefs.h.html#111
>  work?

yes! thanks Hannah! 

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 marked 9 inline comments as done.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 61225.
brute4s99 added a comment.


  added handling for WIndows. I'll try it on linux and revert on this patch!

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=61035=61225

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> brute4s99 wrote in kio_sftp.cpp:2257
> `S_IFLNK` is not defined on WIndows

Does 
https://code.woboq.org/qt5/qtbase/mkspecs/common/posix/qplatformdefs.h.html#111 
work?

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> albertvaka wrote in kio_sftp.cpp:2257
> I think what they mean is that if you remove the ifdef it should just work.

`S_IFLNK` is not defined on WIndows

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> albertvaka wrote in kio_sftp.cpp:2257
> I think what they mean is that if you remove the ifdef it should just work.

ah, gotcha!

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> andriusr wrote in kio_sftp.cpp:2257
> I'm not sure what is intended to achieve here, the remote file _can_ be a 
> symlink, and IIRC when we had sftp on dolphin in KDE4 those symlinks were 
> shown as such.

I think what they mean is that if you remove the ifdef it should just work.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  In D22105#489954 , @vonreth wrote:
  
  > Any reason why you skip the symlinks?
  >  I mean displaying thm should be fine, dolphin probably can also follow 
them.
  >  Creating them on windows is a bit more problematic.
  
  
  For my use case, I could not find any possibility of symlinks, (traversing 
Android filesystem) so I left it blank in here for any future dev to fix it.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Hannah von Reth]

vonreth added a comment.


  Any reason why you skip the symlinks?
  I mean displaying thm should be fine, dolphin probably can also follow them.
  Creating them on windows is a bit more problematic.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 marked an inline comment as done.
brute4s99 added inline comments.

INLINE COMMENTS

> vonreth wrote in kio_sftp.cpp:402
> What about the QFileInfo?

For my use case, I could not find any possibility of symlinks, (traversing 
Android filesystem) so I left it blank in here for any future dev to fix it.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> vonreth wrote in CMakeLists.txt:12
> What requires this bump?

ah, I thought QFileDevice needs it. Reverting this.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 61035.
brute4s99 marked 6 inline comments as done.

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22105?vs=60701=61035

BRANCH
  arcpatch-D22105

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

AFFECTED FILES
  CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Hannah von Reth]

vonreth added reviewers: sitter, dfaure.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Andrius da Costa Ribas]

andriusr added inline comments.

INLINE COMMENTS

> kio_sftp.cpp:2257
> +#ifdef Q_OS_WIN
> +// TODO ADD CODE FOR HANDLING FILETYPE - SYMLINK IN WIN
> +#else

I'm not sure what is intended to achieve here, the remote file _can_ be a 
symlink, and IIRC when we had sftp on dolphin in KDE4 those symlinks were shown 
as such.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Hannah von Reth]

vonreth requested changes to this revision.
vonreth added a comment.
This revision now requires changes to proceed.


  We also should add a maintainer for review

INLINE COMMENTS

> CMakeLists.txt:12
>  
> -set(QT_MIN_VERSION "5.8.0")
> +set(QT_MIN_VERSION "5.10.0")
>  set(KF5_MIN_VERSION "5.48.0")

What requires this bump?

> kio_sftp.cpp:387
> +#ifdef Q_OS_WIN
> +access = (mode_t)perms::owner_all | (mode_t)perms::group_all | 
> (mode_t)perms::others_all;
> +#else

static_cast instead of c cast, a single cast should do.
Are the types compatible?

> kio_sftp.cpp:402
> +#ifdef Q_OS_WIN
> +// TODO ADD CODE FOR HANDLING FILETYPE - SYMLINK IN WIN
> +#else

What about the QFileInfo?

> kio_sftp.cpp:1767
> +#ifdef Q_OS_WIN
> +initialMode = permissions | 
> (mode_t)perms::owner_write | (mode_t)perms::owner_read ;
> +#else

single static cast as above

> kio_sftp.cpp:1978
> +#ifdef Q_OS_WIN
> +initialMode = permissions | (mode_t)perms::owner_write;
> +#else

cast

> kio_sftp.cpp:2041
> +if (!receivedFile.open(QIODevice::ReadWrite | 
> QIODevice::Text)) {
> +QString error_msg = receivedFile.errorString();
> +}

well do something with the error string?

> kio_sftp.cpp:2242
> +#ifdef Q_OS_WIN
> +access = (mode_t)perms::owner_all | (mode_t)perms::group_all | 
> (mode_t)perms::others_all;
> +#else

cast

> kio_sftp.cpp:2257
> +#ifdef Q_OS_WIN
> +// TODO ADD CODE FOR HANDLING FILETYPE - SYMLINK IN WIN
> +#else

QFileInfo

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman
Cc: kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, alexde, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 edited the summary of this revision.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman
Cc: kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, alexde, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Piyush Aggarwal]

brute4s99 created this revision.
brute4s99 added reviewers: albertvaka, vonreth, sredman.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
brute4s99 requested review of this revision.

REVISION SUMMARY
  CAUTION : Still WIP. The main functionality works, but it's not a pleasant 
for the end user for now.
  
  The fixed plugin works on Windows. Right now, KDE Connect uses it to 
establish an initial connection over SFTP, which can then be offloaded to any 
sftp:// handling application.
  
  Currently, this can be achieved seamlessly by using WinSCP 
 which is a free and open source SFTP Client.

TEST PLAN
  0. Install WinSCP : https://winscp.net/eng/download.php.
  
  1. set Craft to use `master` for `kio-extras`
  
craft --set version=master kio-extras
  
  
  
  2. install `kio-extras` and all deps
  
craft -i kio-extras
  
  
  
  3. apply this patch
  
  4. re-build `kio-extras`
  
craft --compile --install --qmerge kio-extras
  
  
  
  5. checkout `milestone2` branch from 
invent.kde.org/piyushaggarwal/kdeconnect-kde
  
  6. build `kdeconnect-kde` again
  
craft --compile --install --qmerge kdeconnect-kde
  
  
  
  7. Run `kdeconnect-indicator.exe` from within `CraftRoot/bin/`
  
  8. Right  Click on the dark icon in sys tray, go to your phone and click on 
**Browse Device**.
  
  9. Press YES, followed by an OK on password prompt (the correct password 
comes pre-filled).
  
  10. Expect WinSCP to take point from there on.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, albertvaka, vonreth, sredman
Cc: kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, alexde, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov