Differential D10993

Fix protocol selection in KUrlNavigator
ClosedPublic
Actions

Authored by aleksejshilin on Mar 3 2018, 3:39 PM.

Details

Reviewers

dfaure

Group Reviewers

Frameworks

Commits

R241:37868c2c57f7: Fix protocol selection in KUrlNavigator

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.

Diff Detail

Repository

R241 KIO

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

aleksejshilin created this revision.Mar 3 2018, 3:39 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptMar 3 2018, 3:39 PM

aleksejshilin requested review of this revision.Mar 3 2018, 3:39 PM

Thanks for the fix. I have one small suggestion.

src/filewidgets/kurlnavigator.cpp
349–355	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:".

This revision now requires changes to proceed.Mar 3 2018, 7:00 PM

aleksejshilin added inline comments.Mar 3 2018, 8:34 PM

src/filewidgets/kurlnavigator.cpp
349–355	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.

Add comment explaining the empty authority

dfaure added inline comments.Mar 4 2018, 9:07 AM

src/filewidgets/kurlnavigator.cpp
349–355	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?

Move setting authority to the else branch

src/filewidgets/kurlnavigator.cpp
349–355	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.

dfaure accepted this revision.Mar 4 2018, 10:18 AM

This revision is now accepted and ready to land.Mar 4 2018, 10:18 AM

Closed by commit R241:37868c2c57f7: Fix protocol selection in KUrlNavigator (authored by aleksejshilin). · Explain WhyMar 4 2018, 10:41 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			src/filewidgets/kurlnavigator.cpp (8 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	28496	6e60885		Mar 3 2018, 3:39 PM	★	★
Diff 2	28519	6e60885	- Add comment explaining the empty authority	Mar 3 2018, 9:08 PM	★	★
Diff 3	28571	6e60885	- Move setting authority to the else branch	Mar 4 2018, 10:17 AM	★	★
Diff 4	28574	6e60885	R241:37868c2c57f7eff0ac4c7c401d271a3b7ea86be0	Mar 4 2018, 10:40 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
de27cd693fab	761c6e5bff4d	fcdb0193d1f5	Алексей Шилин	Move setting authority to the else branch	Mar 4 2018, 9:43 AM
fcdb0193d1f5	9b821b45ad22	dced0a6f8b02	Алексей Шилин	Add comment explaining the empty authority	Mar 3 2018, 9:06 PM
dced0a6f8b02	8e67bc95e428	6e608854469f	Алексей Шилин	Fix protocol selection in KUrlNavigator (Show More…)	Mar 3 2018, 2:27 PM

Diff 28574

View Options

src/filewidgets/kurlnavigator.cpp