D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
This revision was automatically updated to reflect the committed changes. Closed by commit R126:484a7dc89a75: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior (authored by achauvel, committed by ngraham). REPOSITORY R126 KDE CLI Utilities CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15189?vs=50581=50664 REVISION DETAIL https://phabricator.kde.org/D15189 AFFECTED FILES kioclient/kioclient.cpp To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel added a comment. It’s perso at hack-libre dot org REPOSITORY R126 KDE CLI Utilities REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
ngraham added a comment. Can you provide your email address so we can land this patch for you with proper authorship information? Thanks! REPOSITORY R126 KDE CLI Utilities REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R126 KDE CLI Utilities REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel updated this revision to Diff 50581. REPOSITORY R126 KDE CLI Utilities CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15189?vs=50580=50581 REVISION DETAIL https://phabricator.kde.org/D15189 AFFECTED FILES kioclient/kioclient.cpp To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel updated this revision to Diff 50580. achauvel added a comment. Added an `ifdef` so that it could compile with older version of KIO. REPOSITORY R126 KDE CLI Utilities CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15189?vs=47987=50580 REVISION DETAIL https://phabricator.kde.org/D15189 AFFECTED FILES kioclient/kioclient.cpp To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
dfaure added a comment. #include #if KIO_VERSION >= QT_VERSION_CHECK(5,55,0) ... #endif REPOSITORY R126 KDE CLI Utilities REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel added a comment. @dfaure I don’t see how to do that, since there does not seem to have such `ifdef` in the actual codebase. However I can make `set(KF5_MIN_VERSION "5.55.0")` instead of 5.54.0 in the `CMakeLists.txt`. REPOSITORY R126 KDE CLI Utilities REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
dfaure added a comment. Please add a KIO version ifdef to avoid breaking compilation with older versions of KIO. REPOSITORY R126 KDE CLI Utilities REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel updated this revision to Diff 47987. achauvel added a comment. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Uses new function added in https://phabricator.kde.org/D17726 REPOSITORY R126 KDE CLI Utilities CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15189?vs=40897=47987 REVISION DETAIL https://phabricator.kde.org/D15189 AFFECTED FILES kioclient/kioclient.cpp To: achauvel, #frameworks, dfaure, cfeck Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel added a comment. Is anybody interested in helping me figure out how I can implement that properly? I would be glad to start working again on it, but I don’t know enough about KIO right now. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: anthonyfieroni, ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
dfaure added a comment. OK after reading the bug report, I think this is more about "KRun as used by kde-open5 vs KRun as used by other apps such as konqueror". We want e.g. konqueror to follow the redirection. On the other hand we don't want kde-open5 to follow the redirection. Then what we might need is a KRun::setFollowRedirections(bool). On by default, set to off by kde-open5. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: anthonyfieroni, ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. I'm really not convinced about this. What about all the cases where you do want to see the redirection happen? - Redirection from http to https - Redirection from some outdated URL to the new correct URL - Redirection from google.com to www.google.com I think this needs more research. Why do other browsers show the new URL in all of the above cases and not for the twitter testcase? Different HTTP error code maybe? (I vaguely remember something about permanent redirections vs normal redirections?) INLINE COMMENTS > anthonyfieroni wrote in krun.cpp:1319 > Also you can add Qt::CaseInsensitive as second parameter to startsWith for > scheme with capital letters. Not necessary, scheme() always returns the scheme lower-cased. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: anthonyfieroni, ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
ngraham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: anthonyfieroni, ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel updated this revision to Diff 40897. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15189?vs=40784=40897 REVISION DETAIL https://phabricator.kde.org/D15189 AFFECTED FILES src/widgets/krun.cpp To: achauvel, #frameworks, dfaure, cfeck Cc: anthonyfieroni, ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
anthonyfieroni added inline comments.
INLINE COMMENTS
> anthonyfieroni wrote in krun.cpp:1319
> You cannot remove it, since it can be a local file, remote file with scheme
> that differs http and should be redirect then. So you can make it like that
>
> if (!job->url().scheme().startsWith("http")) {
> setUrl(job->url());
> }
>
> Now it will not redirect only http and https.
Also you can add Qt::CaseInsensitive as second parameter to startsWith for
scheme with capital letters.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D15189
To: achauvel, #frameworks, dfaure, cfeck
Cc: anthonyfieroni, ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
anthonyfieroni added inline comments.
INLINE COMMENTS
> krun.cpp:1319
> -// Update our URL in case of a redirection
> -setUrl(job->url());
> -
You cannot remove it, since it can be a local file, remote file with scheme
that differs http and should be redirect then. So you can make it like that
if (!job->url().scheme().startsWith("http")) {
setUrl(job->url());
}
Now it will not redirect only http and https.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D15189
To: achauvel, #frameworks, dfaure, cfeck
Cc: anthonyfieroni, ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel updated this revision to Diff 40784. achauvel edited the test plan for this revision. achauvel added a comment. Previous was incorrect, this one works. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15189?vs=40782=40784 REVISION DETAIL https://phabricator.kde.org/D15189 AFFECTED FILES src/widgets/krun.cpp To: achauvel, #frameworks, dfaure, cfeck Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
ngraham added reviewers: Frameworks, dfaure, cfeck. ngraham added a comment. Can you change `Fix https://bugs.kde.org/show_bug.cgi?id=354246` to `BUG: 354246`? Thanks! See https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords It would also be nice if you could update the Test Plan section with an indication of how you tested this change. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15189 To: achauvel, #frameworks, dfaure, cfeck Cc: ngraham, kde-frameworks-devel, michaelh, bruns
D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
achauvel created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. achauvel requested review of this revision. REVISION SUMMARY Fix https://bugs.kde.org/show_bug.cgi?id=354246 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15189 AFFECTED FILES src/widgets/krun.cpp To: achauvel Cc: kde-frameworks-devel, michaelh, ngraham, bruns
