I haven't built it locally yet (I need to update the whole kdepim* stack to build master, working on it).

pop3 is a critical resource.
We need to be sure that all is ok before to commit it.

I CC volker here

The change as such is correct once the TCPSlaveBase port has landed.
The thing I'm wondering about is the various combinations in which this can be deployed though, considering the recent webdav breakage. So I think we will need to have both cases (KTcpSocket and QSslSocket) in 19.08, and for 19.12 we need that too unless we can depend on a recent enough KIO. Does that make sense @dfaure?

This looks like it's based on a change in KF5, so the right solution is #if KIO_VERSION >= KIO_VERSION_CHECK( ... )

This is easier than the webdav case where it was the other way around, KF5 behavior had to change depending on kdepim version, which it can't check for.

In D24929#554396, @dfaure wrote:

This looks like it's based on a change in KF5, so the right solution is #if KIO_VERSION >= KIO_VERSION_CHECK( ... )

This is easier than the webdav case where it was the other way around, KF5 behavior had to change depending on kdepim version, which it can't check for.

Is that really enough? Considering a KF5 update on an existing 19.08 installation for example, the existing installation will assume it gets a KTcpSocket here, while the KF5 update then gives it a QSslSocket.

Ah, I didn't think of the case of upgrading without recompiling.

Yeah then the easiest solution is to do two qobject_casts.
But the KTcpSocket code could still be inside a "if KIO version is old" check, so that we remember to remove this later.

19.08.3 is expected to be tagged on 2019-11-04 and released on 2019-11-07
5.64 is expected to be tagged on 2019-11-02 and released on 2019-11-09

If kio-5.64 is pushed to distro repos before 19.08.3 is there, pop3 kioslave will break for already installed kdepim-runtime, right?

So my, tentative, proposal is to change kdepim-runtime to require frameworks 5.64 (it currently requires 5.63), this way we are sure packagers will wait and push 19.08.3 and 5.64 in the same update batch/together. What do you think?

Edit: and of course, inform the packagers to wait and push 5.64 with 19.08.3.

OR, push this, with two qobject_cast's as dfaure said (checking for KIO_VERSION 5.65), for 19.08.3 and delay the TcpSlaveBase change to 5.65...

In D24929#556487, @ahmadsamir wrote:

OR, push this, with two qobject_cast's as dfaure said (checking for KIO_VERSION 5.65), for 19.08.3 and delay the TcpSlaveBase change to 5.65...

I'd go for this. Given the invasive nature of the TcpSlaveBase change I'd like @dfaure's review of that first anyway, as well as ideally a full KF release cycle for pre-release testing. With 5.64 being prepared within the next days anyway, getting this into 5.65 seems more realistic and safer therefore. The POP3 change with both variants can then go in right away.

If this is OK then please accept, the 19.08.3 release is almost knocking on the door.

I think we cannot ifdef the two cases in this way, we need to assume that the 19.08.3 release is not rebuild by distros when updating to the 5.65 KF release.
That is, if the KF version is higher than 5.65 we can drop the KTcpSocket case, but not the other way around. In that case we need to check which socket type we get at runtime (ie. cast to both types and check which one succeeds).

In D24929#557643, @vkrause wrote:

I think we cannot ifdef the two cases in this way, we need to assume that the 19.08.3 release is not rebuild by distros when updating to the 5.65 KF release.
That is, if the KF version is higher than 5.65 we can drop the KTcpSocket case, but not the other way around. In that case we need to check which socket type we get at runtime (ie. cast to both types and check which one succeeds).

Right, that makes sense, KIO_VERSION is checked at build time not run time. I'll update the diff.

Thanks, that looks like the logic I had in mind :) The KTcpSocket branch we can then drop in master once we bump the KF5 dependency requirement, but that isn't urgent.
This can maybe be written a bit more elegant with slightly less nesting, but I'll leave the style aspect to Laurent, as far as I'm concerned this can go into 19.08 branch as soon as possible.

Yep, I agree, it is ugly; I spent some time trying to use several methods (ternaries and such) but came back empty handed; I'm genuinely interested in how this can be made smarter :)

In D24929#557692, @ahmadsamir wrote:

Yep, I agree, it is ugly; I spent some time trying to use several methods (ternaries and such) but came back empty handed; I'm genuinely interested in how this can be made smarter :)

Maybe something like this:

QNetworkProxy proxy;
proxy.setType(QNetworkProxy::NoProxy);
if (QSslSocket *sock = qobject_cast<...>(...)) {
    sock->setProxy(proxy);
} else if (KTCpSocket *sock = qobject_cast<...>(...)) {
    sock->setProxy(proxy);
} else {
   qWarning()
}

Limits nesting depth to 1 on the ifs, avoids the nested boolean condition in the final if altogether, reduces scope of the variables and avoids the duplicated proxy code, and makes the eventual removal of the KTcpSocket branch fairly simple.