Don't select file extension
ClosedPublic

Authored by anemeth on Apr 26 2018, 6:24 PM.

Details

Summary

In the filepicker dialog, if only one file is selected then in the file name line edit only selects the filename, without the extension

Test Plan

Diff Detail

Repository
R241 KIO
Branch
select_filename_only (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
anemeth created this revision.Apr 26 2018, 6:24 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 26 2018, 6:24 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
anemeth requested review of this revision.Apr 26 2018, 6:24 PM
anemeth edited the summary of this revision. (Show Details)Apr 26 2018, 6:28 PM
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: Frameworks, VDG.
anemeth edited the summary of this revision. (Show Details)
anemeth added a subscriber: ngraham.
anemeth updated this revision to Diff 33162.Apr 26 2018, 6:32 PM
  • Clarify comment

Nice! But I notice that even though the correct part of the text is selected, the field doesn't actually receive focus.

Nice! But I notice that even though the correct part of the text is selected, the field doesn't actually receive focus.

Well, that was (will be) included in D12538
Should I add dependency to it?

Why don't we do it here, since it's more related--or even in a separate patch, since now that I test without the patch, it seems that it's actually an unrelated pre-existing bug.

Why don't we do it here, since it's more related--or even in a separate patch, since now that I test without the patch, it seems that it's actually an unrelated pre-existing bug.

Created D12545

This is so nice! Let me test extensively...

There will be some problems with files that have multiple extensions, for example .tar.gz or .tar.bz
We could create a list of the most common multiextension filetypes, but I only know these two and Google isn't helpful.
Can some of you help me out here?
Should I even add such a list?

ngraham requested changes to this revision.Apr 26 2018, 7:18 PM

Let's only enable this behavior for the Save dialogs. For the Open dialogs, it seems sensible that you'd actually want the whole text selected (but not focused, see D12545), so that you can easily replace the whole thing if you want to file a file by name. Having the filename extension unselected would make it more difficult to do that, since you'd likely need to remove the extension manually.

This revision now requires changes to proceed.Apr 26 2018, 7:18 PM
ngraham added a comment.EditedApr 26 2018, 7:23 PM

There will be some problems with files that have multiple extensions, for example .tar.gz or .tar.bz
We could create a list of the most common multiextension filetypes, but I only know these two and Google isn't helpful.
Can some of you help me out here?
Should I even add such a list?

Here's how this was recently done for Gwenview, if it helps: https://cgit.kde.org/gwenview.git/commit/?id=e85f4b589b93a5648b0150dca435018cd2fbae02

const QMimeDatabase db;
const QString extension = db.suffixForFileName(filename);
int selectionLength = filename.length();
if (extension.length() > 0) {
    // The filename contains an extension. Assure that only the filename
    // gets selected.
    selectionLength -= extension.length() + 1;
}
d->mFilename->setSelection(0, selectionLength);
rkflx added a subscriber: rkflx.Apr 26 2018, 7:25 PM
anemeth updated this revision to Diff 33173.Apr 26 2018, 9:06 PM

Only enable it for save dialog

anemeth added a comment.EditedApr 26 2018, 9:09 PM

Turns out this was already implemented before, but was not currently utilized.
These 3000+ lines files are hard to overview...
I removed these checks: && !locationEdit->isVisible()
I assume at one point the filename line edit was once set to hide/show on demand.

ngraham accepted this revision.Apr 27 2018, 1:19 PM

Nice. Looks like this does what it's supposed to for open and save, in both single and double-click mode. Tested with Kate, Spectacle, and Gwenview.

This revision is now accepted and ready to land.Apr 27 2018, 1:19 PM
rkflx accepted this revision.Apr 27 2018, 6:55 PM

Thanks for the patch. Pondered over the code, but could not find anything wrong.

I removed these checks: && !locationEdit->isVisible()
I assume at one point the filename line edit was once set to hide/show on demand.

Beineri originally implemented this in 0134fb3a5e50, apparently. However, I don't understand why the check has a !. How could this ever work? At least in the latest KDE3 it is already broken.

ngraham edited the summary of this revision. (Show Details)Apr 27 2018, 7:57 PM

Given that this is fixing a fairly straightforward bug, I feel pretty confident about this patch. Frameworks people: any objections, or shall we land this tomorrow?

elvisangelaccio added inline comments.
src/filewidgets/kfilewidget.cpp
1304

This comment makes little sense to me. Since it was part of the same commit spotted by @rkflx (0134fb3a5e50), I'd just remove it.

1344

Same for this comment.

anemeth updated this revision to Diff 33325.Apr 30 2018, 12:36 PM

Remove some comments

anemeth marked 2 inline comments as done.Apr 30 2018, 12:37 PM

Always nice to see a patch with more red than green in it. :)

elvisangelaccio accepted this revision.Apr 30 2018, 10:53 PM
ngraham closed this revision.May 1 2018, 1:42 AM