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
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
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
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
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
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
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
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")) ?
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 '//'
