D10993: Fix protocol selection in KUrlNavigator
This revision was automatically updated to reflect the committed changes. Closed by commit R241:37868c2c57f7: Fix protocol selection in KUrlNavigator (authored by aleksejshilin). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10993?vs=28571=28574 REVISION DETAIL https://phabricator.kde.org/D10993 AFFECTED FILES src/filewidgets/kurlnavigator.cpp To: aleksejshilin, #frameworks, dfaure Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
aleksejshilin added inline comments. INLINE COMMENTS > dfaure wrote in kurlnavigator.cpp:349 > Well, OK, it's not that it's *incorrect* for file:, it just seems unnecessary > to make that method call. So given that we have an if() already, why not only > do it in the else? > Well, OK, it's not that it's *incorrect* for file:, it just seems unnecessary > to make that method call. Ah, I misunderstood what you meant. > So given that we have an if() already, why not only do it in the else? Done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10993 To: aleksejshilin, #frameworks, dfaure Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH fix_kurlnavigator_protocol_selection REVISION DETAIL https://phabricator.kde.org/D10993 To: aleksejshilin, #frameworks, dfaure Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
aleksejshilin updated this revision to Diff 28571. aleksejshilin added a comment. - Move setting authority to the else branch REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10993?vs=28519=28571 BRANCH fix_kurlnavigator_protocol_selection REVISION DETAIL https://phabricator.kde.org/D10993 AFFECTED FILES src/filewidgets/kurlnavigator.cpp To: aleksejshilin, #frameworks, dfaure Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
dfaure added inline comments. INLINE COMMENTS > aleksejshilin wrote in kurlnavigator.cpp:349 > > Is this in order to get smb:// instead of smb: ? > > Yes. > > > It looks correct, but not for "file", > > Why not? If I read RFC 8089 correctly, for host rule it references RFC 3986 > which allows it to be empty, i.e. an empty authority is valid. > > > And I could add a comment like "we want smb:// or ftp://, not just smb: or > > ftp:". > > Makes sense, indeed. Will do shortly. Well, OK, it's not that it's *incorrect* for file:, it just seems unnecessary to make that method call. So given that we have an if() already, why not only do it in the else? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10993 To: aleksejshilin, #frameworks, dfaure Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
aleksejshilin updated this revision to Diff 28519. aleksejshilin added a comment. - Add comment explaining the empty authority REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10993?vs=28496=28519 BRANCH fix_kurlnavigator_protocol_selection REVISION DETAIL https://phabricator.kde.org/D10993 AFFECTED FILES src/filewidgets/kurlnavigator.cpp To: aleksejshilin, #frameworks, dfaure Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
aleksejshilin added inline comments.
INLINE COMMENTS
> dfaure wrote in kurlnavigator.cpp:349
> Is this in order to get smb:// instead of smb: ? It looks correct, but not
> for "file", how about moving that to an else{} branch of the if below? And I
> could add a comment like "we want smb:// or ftp://, not just smb: or ftp:".
> Is this in order to get smb:// instead of smb: ?
Yes.
> It looks correct, but not for "file",
Why not? If I read RFC 8089 correctly, for host rule it references RFC 3986
which allows it to be empty, i.e. an empty authority is valid.
> And I could add a comment like "we want smb:// or ftp://, not just smb: or
> ftp:".
Makes sense, indeed. Will do shortly.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10993
To: aleksejshilin, #frameworks, dfaure
Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Thanks for the fix. I have one small suggestion.
INLINE COMMENTS
> kurlnavigator.cpp:349
> url.setScheme(protocol);
> -url.setPath((protocol == QLatin1String("file")) ? QStringLiteral("/") :
> QStringLiteral("//"));
> +url.setAuthority(QStringLiteral(""));
> +if (protocol == QLatin1String("file")) {
Is this in order to get smb:// instead of smb: ? It looks correct, but not for
"file", how about moving that to an else{} branch of the if below? And I could
add a comment like "we want smb:// or ftp://, not just smb: or ftp:".
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10993
To: aleksejshilin, #frameworks, dfaure
Cc: michaelh
D10993: Fix protocol selection in KUrlNavigator
aleksejshilin created this revision. aleksejshilin added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. aleksejshilin requested review of this revision. REVISION SUMMARY QUrl no longer accepts an empty authority and a non-empty path that starts with '//' (Qt commit f62768d046528636789f901ac79e2cfa1843a7b7). As the result, protocol selection using the menu no longer worked for all but 'file' scheme. REPOSITORY R241 KIO BRANCH fix_kurlnavigator_protocol_selection REVISION DETAIL https://phabricator.kde.org/D10993 AFFECTED FILES src/filewidgets/kurlnavigator.cpp To: aleksejshilin, #frameworks, dfaure Cc: michaelh
