[KFileWidget] Allow double quotes in filenames
Needs RevisionPublic

Authored by meven on Nov 27 2019, 5:38 PM.

Details

Reviewers
ngraham
dfaure
Group Reviewers
Frameworks
Summary

Use " " to detect if there is multiple files and split filenames allowing them to contain ".

BUG: 185433
FIXED-IN: 5.65

Test Plan
  1. touch ~/\"test.txt
  2. Use KFileWidget to select this file (you can use kfilewidgettest_saving_gui.cpp)

ctest

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D25572
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19244
Build 19262: arc lint + arc unit
meven created this revision.Nov 27 2019, 5:38 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 27 2019, 5:38 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Nov 27 2019, 5:38 PM
meven updated this revision to Diff 70489.Nov 28 2019, 12:13 PM

Clean include, update comment

meven updated this revision to Diff 70490.Nov 28 2019, 12:16 PM

Add comment

meven updated this revision to Diff 70499.Nov 28 2019, 2:02 PM

Avoid using unnecessarly QRegularExpression

This isn't working for me. When I create a file named "foobar".txt, the file dialog is still unable to open it, saying:

The file "/home/nate/Desktop/Foobar" could not be found

tests/kfilewidgettest_saving_gui.cpp
31 ↗(On Diff #70499)

These changes seem unrelated

meven updated this revision to Diff 70651.Dec 1 2019, 9:03 AM

Remove unrelated change

meven added a comment.Dec 1 2019, 9:06 AM

This isn't working for me. When I create a file named "foobar".txt, the file dialog is still unable to open it, saying:

That's precisely the unit test included, it works for.
I think the app you tested with did not use your compiled kio because of kdeinit. You can use the included or recompile and use gwenview as it does not use kdeinit, you can also restart kdeinit before launching the app you want to test with :

$ kdeinit5
kdeinit5: Shutting down running client.
dfaure added a comment.Dec 1 2019, 1:16 PM

Restarting kdeinit is only needed when making changes that affect kioslaves.
Here it's just about a widget, all you need is to start the application from a terminal.

ngraham accepted this revision.Dec 1 2019, 5:35 PM

Thanks Méven. I can confirm that it works in Gwenview with no other changes. I have to re-compile Kolourpaint, but then it works from there too. Odd.

This revision is now accepted and ready to land.Dec 1 2019, 5:35 PM
dfaure requested changes to this revision.Dec 1 2019, 5:59 PM

I think this needs more work, we lost some tolerance here, AFAICS.

src/filewidgets/kfilewidget.cpp
1751–1754

What if I had "file1.txt" "file2.txt" and I manually edit that to remove the second file, ending up with just "file1.txt"? The double-quotes will no longer be removed, because of this modified condition.

1770–1772

Isn't there a " at this position, always?
Initially, at position 0, and then later, we know we hit " " so index1 = index2+2 could be done at the end of the loop (instead of +1), and here we don't need an indexOf call anymore.
I assume this was done in order to be tolerant to things like two spaces or something, but now the separator is hardcoded to " " so we *know* there's a quote at index2+2 after finding a quote at index2.

Well, I guess we can keep an initial indexOf (outside the loop) in case there's a leading space (because a user manually removed the first file).

So this needs additional unittests for leading space, trailing space, single-quoted-file .... and I wonder if we are OK with no longer supporting two spaces (e.g. because the user manually removed some intermediate file in the list).

The separator is in fact " +" in regexp syntax, then?

1772

this could move to after the if() block on the next line.
If index1 < 0, there's no point in calculating index2.

1780

Isn't the last character a double-quote (in the normal case)? Don't we want to stop just before it, to avoid grabbing it?

Of course this opens the question of what to do if the last character is NOT a double-quote, but
"file1.txt" "file2.txt looks weird. Well, getting file2.tx as a result would be even weirder, so better go backwards from the end of the string until "not space and not double-quote", for some tolerance. (another case for the unittest)

This revision now requires changes to proceed.Dec 1 2019, 5:59 PM