Fix pinning apps when their .desktop file has a space in the file name.
Needs ReviewPublic
Actions

Authored by hein on Feb 8 2018, 9:41 AM.

Details

Reviewers

broulik
ivan

Group Reviewers

Plasma

Summary

Things I have concerns about with this patch and would appreciate
thoughtful input on:

To get URLs through StrictMode parsing we need to start percent- encoding spaces, but that means isValid() fails us on scheme- less URLs without a seperate scheck for QUrl::scheme().isEmpty(). Is rejecting scheme-less URLs going to break anything? I can't think of any cases where the URLs won't have file: or applications: as scheme nowadays.
I'm worried the use of QUrl::toPercentEncoding might break some existing pinned launchers because it's going to percent-encode things that weren't percent-encoded before. Can we be confident QUrl::operator== will abstract over this?

Diff Detail

Repository

R120 Plasma Workspace

Branch

Plasma/5.12

Lint

No Linters Available

Unit

No Unit Test Coverage

hein created this revision.Feb 8 2018, 9:41 AM

Restricted Application added a project: Plasma. · View Herald TranscriptFeb 8 2018, 9:41 AM

hein requested review of this revision.Feb 8 2018, 9:41 AM

Is rejecting scheme-less URLs going to break anything?

I think we're doing a quite good job of ensuring we have a file: or other scheme, so this should'nt cause behavior changes.

Can we be confident QUrl::operator== will abstract over this?

Being confident about QUrl stuff is tricky :) but naive testing showed that QUrl considers a URL with spaces and percent-encoded spaces equal.

libtaskmanager/startuptasksmodel.cpp
208	`QUrl::toPercentEncoding` encodes all UTF-8 characters and slashes and what not whereas in `toString` you only tell it to `EncodeSpaces`
libtaskmanager/tasktools.cpp
731	Is KActivities smart enough to relate the two with and without spaces?

Ivan, could you weigh in on the KActivities spaces angle?

libtaskmanager/startuptasksmodel.cpp
208	Ah yeah, probably better to make it consistent. This happened because I initially tried toPercentEncoding around the string, which of course fails because it encodes the colon after the scheme.
libtaskmanager/tasktools.cpp
731	I don't know, good question. I should CC Ivan on this. If It uses QUrl::operator==, I guess so.

I can not say with certainty - never tested html encoding, but I would expect it to work in general.

The potential problem might be that sqlite part might consider "hello world" and "hello%20world" to be different.

Is this expected to fully fix https://bugs.kde.org/show_bug.cgi?id=385942?

@ivan Could you suggest a good testcase? I'm a bit wary of pushing this into the LTS branch with a low amount of confidence.

In D10383#203804, @ngraham wrote:

Is this expected to fully fix https://bugs.kde.org/show_bug.cgi?id=385942?

No, because the reporters there used the ticket for multiple distinct issues. D10405 fixes another bug reported there. Both together should resolve things, though.

any update on this?

Revision Contents
Changeset List

			Path	Packages
M			libtaskmanager/launchertasksmodel.cpp (9 lines)
M			libtaskmanager/startuptasksmodel.cpp (2 lines)
M			libtaskmanager/tasktools.cpp (14 lines)
M			libtaskmanager/xwindowtasksmodel.cpp (2 lines)

Commit	Tree	Parents	Author	Summary	Date
fad25605a392	aa8d8a2b4b17	7abd8d0da447	Eike Hein	Fix pinning apps when their .desktop file has a space in the file name. (Show More…)	Feb 8 2018, 9:36 AM