Differential D21463

KIMAP: Option to use system proxy settings (Core part)
ClosedPublic
Actions

Authored by marten on May 28 2019, 3:39 PM.

Details

Reviewers
mlaurent
dvratil
Group Reviewers
KDE PIM
Commits
R177:544bae2f88f3: Provide an option wheether to use the system proxy settings
Summary

This is to provide an explicit option to not use the system proxy setting for IMAP connections, as described in T7430.

If the system proxy settings are set to use the environment variables http_proxy etc (for compatibility with non-KDE applications), then they are also used for IMAP connections. This is rarely required and will fail unless the proxy is transparent enough. See https://bugs.kde.org/show_bug.cgi?id=407685 and related bugs.

This change adds a IMAP account option to use the proxy for connection (the default is off). This is the core part of the change; the visible GUI part will follow in another diff.

Test Plan

Built kimap with this change, verified that by default the IMAP connection is not made via the proxy and that mail fetching works as expected. Verified that, if the option is set, the IMAP connection is made via the proxy.

Diff Detail

Repository
R177 PIM: KIMAP
Lint
Lint Skipped
Unit
Unit Tests Skipped
marten created this revision.May 28 2019, 3:39 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptMay 28 2019, 3:39 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
marten requested review of this revision.May 28 2019, 3:39 PM
mlaurent requested changes to this revision.May 28 2019, 5:22 PM
mlaurent added a subscriber: mlaurent.

You missed to increase kimap version (and missing to increase dependancy in kdepim-runtime)

This revision now requires changes to proceed.May 28 2019, 5:22 PM
marten added a comment.May 28 2019, 7:02 PM

Laurent, do you mean the PIM_VERSION in kimap/CMakeLists.txt (which then sets KIMAP_LIB_VERSION), currently "5.11.40"? I was under the impression that this was managed by the release scripts and all PIM components had to have the same version.
I assume that you don't mean the SOVERSION of the kimap library (which doesn't have a minor version).

mlaurent added a comment.May 28 2019, 8:39 PM

"PIM_VERSION in kimap/CMakeLists.txt (which then sets KIMAP_LIB_VERSION), currently "5.11.40"" yep. Increase to 5.11.41
it's not the release script which changes it.

marten updated this revision to Diff 58821.May 29 2019, 8:24 AM

Version number updated

dvratil requested changes to this revision.May 31 2019, 11:56 AM
dvratil added a subscriber: dvratil.
dvratil added inline comments.
src/sessionthread.cpp
67

Either make m_useProxy an std::atomic_bool or make sure m_useProxy is accessed from the SessionThread's thread (QMetaObject::invokeMethod(this, [this, useProxy]() { m_useProxy = useProxy; }, Qt::QueuedConnection))

68

Unless the socket is disconnected, you should probably force reconnection here to apply the new configuration...

175

Provide more context: "Connecting to IMAP server without a proxy"

180

More context: "Connecting to IMAP server using default system proxy"

Shouldn't you also explicitly set the QNetworkProxy here as well? The socket doesn't get recreated on reconnect, so if the option is changed, it wouldn't get reset.

This revision now requires changes to proceed.May 31 2019, 11:56 AM
marten updated this revision to Diff 58957.May 31 2019, 3:27 PM
marten marked 3 inline comments as done.

Diff updated in accordance with review comments

marten added inline comments.May 31 2019, 3:33 PM
src/sessionthread.cpp
68

Is this needed? I've traced the agent and the execution of the SessionThread, and if the account settings are changed in KMail the agent is set offline then online. This means that threadQuit() and threadInit() are called which recreates the socket and reconnects to the server with the new settings.

180

Set here to make sure, but as above the socket and connection is recreated when the option is changed by the user anyway.

dvratil added inline comments.Jun 1 2019, 6:28 PM
src/sessionthread.cpp
68

KIMAP is a generic IMAP library, you shouldn't think about it only in the context of how it's used by KMail/Akonadi.

marten updated this revision to Diff 59077.Jun 3 2019, 4:05 PM

New function setUseProxyInternal() to set proxy state and, if the connection is active, disconnect and reconnect it with the new proxy setting.

dvratil accepted this revision.Jun 8 2019, 11:49 AM
marten added a comment.Jun 15 2019, 7:36 PM

@mlaurent, are you also happy to accept this revision?

mlaurent accepted this revision.Jun 16 2019, 5:54 PM
This revision is now accepted and ready to land.Jun 16 2019, 5:54 PM
Closed by commit R177:544bae2f88f3: Provide an option wheether to use the system proxy settings (authored by marten). · Explain WhyJun 19 2019, 6:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
MCMakeLists.txt (2 lines)
Msrc/session.h (9 lines)
Msrc/session.cpp (5 lines)
Msrc/sessionthread.cpp (23 lines)
Msrc/sessionthread_p.h (3 lines)

Diff 58957

View Options

CMakeLists.txt

Loading...
View Options

src/session.h

Loading...
View Options

src/session.cpp

Loading...
View Options

src/sessionthread.cpp

Loading...
View Options

src/sessionthread_p.h

Loading...