allow kio-extras to build with mingw on win32 and remove unnecessary includes
Needs RevisionPublic

Authored by brute4s99 on Jul 24 2019, 9:27 PM.

Details

Test Plan

should build on windows with mingw as compiler

should build on linux with gcc

Diff Detail

Repository
R320 KIO Extras
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14389
Build 14407: arc lint + arc unit
brute4s99 created this revision.Jul 24 2019, 9:27 PM
Restricted Application added projects: Dolphin, Frameworks. ยท View Herald TranscriptJul 24 2019, 9:27 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. ยท View Herald Transcript
brute4s99 requested review of this revision.Jul 24 2019, 9:27 PM
vonreth accepted this revision.Jul 24 2019, 9:49 PM
vonreth added a reviewer: dfaure.

Looks good.
Why not the first time ๐Ÿ˜Š

This revision is now accepted and ready to land.Jul 24 2019, 9:49 PM

Looks good.
Why not the first time ๐Ÿ˜Š

I was afraid of reverts ๐Ÿ˜ท

pino requested changes to this revision.Jul 25 2019, 4:33 AM
pino added a subscriber: pino.

other than building, please also check that it actually still works on Linux

sftp/kio_sftp.cpp
383

sounds like you need to use the *User enums, not the *Owner ones

1757

ditto

1964

ditto

2222

ditto

This revision now requires changes to proceed.Jul 25 2019, 4:33 AM
brute4s99 added inline comments.Jul 27 2019, 8:03 PM
sftp/kio_sftp.cpp
383

https://www.gnu.org/software/libc/manual/html_node/Permission-Bits.html

The link says these bits refer to the owner of the file. Should I still change these to *User ?

dfaure requested changes to this revision.Aug 1 2019, 10:49 PM

The Qt documentation says "Warning: Because of differences in the platforms supported by Qt, the semantics of ReadUser, WriteUser and ExeUser are platform-dependent: On Unix, the rights of the owner of the file are returned and on Windows the rights of the current user are returned. "

But, wait, this code is mixing "int permissions" (*) with the QFileDevice enum, that doesn't make any sense to me.

(*) this comes from KIO::put, which takes unix permissions on unix, not sure what it takes on Windows...

This doesn't match: unix permissions are octal (e.g. group read is 040 in octal), QFileDevice enum is hex (0x040).

The Qt documentation says "Warning: Because of differences in the platforms supported by Qt, the semantics of ReadUser, WriteUser and ExeUser are platform-dependent: On Unix, the rights of the owner of the file are returned and on Windows the rights of the current user are returned. "

okay, I believe you mean I should leave these changes as-are in the diff. Am I right?

But, wait, this code is mixing "int permissions" (*) with the QFileDevice enum, that doesn't make any sense to me.

(*) this comes from KIO::put, which takes unix permissions on unix, not sure what it takes on Windows...

This doesn't match: unix permissions are octal (e.g. group read is 040 in octal), QFileDevice enum is hex (0x040).

Okay. I'm still unable to understand where/ how sftp::put() is called, or I would change everywhere it is called, to Qt way of permission extraction.

But, wait, this code is mixing "int permissions" (*) with the QFileDevice enum, that doesn't make any sense to me.

(*) this comes from KIO::put, which takes unix permissions on unix, not sure what it takes on Windows...

This doesn't match: unix permissions are octal (e.g. group read is 040 in octal), QFileDevice enum is hex (0x040).

Okay. I'm still unable to understand where/ how sftp::put() is called, or I would change everywhere it is called, to Qt way of permission extraction.

I would recommend not making API or ABI changes. It's annoying that they're not compatible, but you can probably find some way to convert from Qt permissions to the expected values in order to not change users

The truth is stronger than "I would not recommend".
put() in SlaveBase-derived classes is called by the KIO library (TransferJob), so you CANNOT change the meaning of the arguments. It's part of the API/ABI for all slaves, and this cannot change until KF6.

The truth is stronger than "I would not recommend".
put() in SlaveBase-derived classes is called by the KIO library (TransferJob), so you CANNOT change the meaning of the arguments. It's part of the API/ABI for all slaves, and this cannot change until KF6.

+1

But it is worth making a note of this as something to change (or discuss changing) for KF6 since it seems like using the Qt constants would be a better design