Use the more user-friendly string "File type" in the save dialogs
AbandonedPublic

Authored by ngraham on Apr 11 2018, 11:08 PM.

Details

Reviewers
bruns
alexeymin
abetts
rkflx
Group Reviewers
Frameworks
VDG
Summary

Time to fix a 14-year old wishlist bug! This patch uses the more user-friendly string "File type" to replace the somewhat technical and usually inappropriate string "Filter" in the file save dialogs.

BUG: 79903
FIXED-IN: 5.46

Test Plan

Tested with Gwenview, Konsole, Spectacle, and Kate in a new user account after deploying the change. Here are some before-and-after screenshots:

Gwenview Save dialog, before:

Gwenview Save dialog, after:

Konsole save dialog, before:

Konsole Save dialog, after:

Spectacle save dialog, before:

Spectacle save dialog, after:

Kate save dialog, before:

Kate save dialog, after:

It seems like a straightforward improvement for the common case where the Save dialog has a combobox for choosing the file type. However, Kate has a weird filter box instead. I wonder if having a filter feature even makes any sense when saving files, and if it does--why only for Kate? If necessary I can make the string return to "Filter" for these save-with-filter dialogs, but I'm still left wondering what the feature is actually for or if Kate is affected by a bug that should be fixed...

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D12130
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Apr 11 2018, 11:08 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 11 2018, 11:08 PM
ngraham requested review of this revision.Apr 11 2018, 11:08 PM
ngraham edited the test plan for this revision. (Show Details)Apr 11 2018, 11:09 PM
bruns accepted this revision.Apr 12 2018, 12:58 AM
bruns added a subscriber: bruns.

+1

This revision is now accepted and ready to land.Apr 12 2018, 12:58 AM
alexeymin accepted this revision.Apr 12 2018, 7:08 AM
alexeymin added a subscriber: alexeymin.

+1, looks better

rkflx requested changes to this revision.Apr 12 2018, 8:52 AM
rkflx added a subscriber: rkflx.

BUG: 79904

@bruns @alexeymin I'm not sure you have reviewed this change properly? This bug is about "MacOS styled menus and remote X sessions". If you "Accept" a revision, to keep quality up it is important to check everything (in particular at least read what the bug is about), otherwise please give a "+1" only.

@ngraham I don't think we should change the string (at least if text input in the filter is enabled, but I'd value consistency with the other case too…). The widget is not only about selecting a filetype, but about being able to filter the filenames based on wildcards, much like what you can do in Dolphin. There is a "What's this" help to explain the feature further, but changing it to "File format" hides it even more from discovery, which would be a bad thing and counter your other efforts in this dialog to make things more discoverable.

Also, reading what's available in the filter already makes it pretty obvious that the widget can be used to filter for filetypes too. I doubt users have problems using this only because it says "Filter" on the very left side.

There are some situations where changing the string might be warranted:

  • The file dialog does not allow to filter and displays a fixed (non-editable) combobox of filetypes.
  • Another (more obvious) UI to filter the view is added.

I'm willing to accept the patch if the naming is made more intelligent to account for all situations. Another approach would be to change the API so that applications can override the default "Filter" string. Until that's done, a string which fits all cases has to stay, which is "Filter".

src/filewidgets/kfilewidget.cpp
585–586

I'm talking about this. E.g. an intermediate-level user could filter for *ProjectX*CustomerMeetingApril*, which is not at all about filetypes.

Of course the very simple folder listing in your screenshot does not have the need for filtering like that, but you know very well that in the real world folder listings can get very long, otherwise you would not work on the Places panel ;)

This revision now requires changes to proceed.Apr 12 2018, 8:52 AM
ngraham planned changes to this revision.Apr 12 2018, 6:30 PM
ngraham edited the summary of this revision. (Show Details)

Let's see if I can make this patch more intelligent, because the root of the issue is that the same text field is used for different things in different modes.

ngraham updated this revision to Diff 32015.Apr 12 2018, 9:33 PM

Change the text only when saving (it really is a filter bar in open mode)

It's not too hard to make the label and the "What's this?" text differ for open vs save mode, which I've done here.

There's still the issue that the text doesn't quite fit in save mode, where there's a filter but it's somewhat useless. I wonder: what good is the filter UI for saving? Does anybody use it? What for? For opening, I think it makes sense, since you might want to limit the view so you can more easily find something that you want to open. But for saving... what value does it actually bring to the table? In what circumstance might a user want to filter the view when saving?

ngraham retitled this revision from Use the more user-friendly string "File format" in the open/save dialogs to Use the more user-friendly string "File format" in the save dialogs.Apr 12 2018, 9:41 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
cfeck added a subscriber: cfeck.Apr 12 2018, 9:45 PM
cfeck added inline comments.
src/filewidgets/kfilewidget.cpp
1337

I think with an empty line after the QString variable declarations, the 'if' can breathe a bit :)

1341

Do you say "Save a file as a format" or "Save a file in a format"?

1342

It feels odd that the label is changed in this method. I suggest to move it into the function that calls it.

ngraham updated this revision to Diff 32018.Apr 12 2018, 10:03 PM
ngraham marked 2 inline comments as done.

Improve the formatting, and move the label into its own function

ngraham marked 2 inline comments as done.Apr 12 2018, 10:03 PM
ngraham added inline comments.
src/filewidgets/kfilewidget.cpp
1341

I've always said "save as."

"Save that picture as a JPEG real fast, would ya?"

bruns added inline comments.Apr 12 2018, 10:47 PM
src/filewidgets/kfilewidget.cpp
1341

"Use it to choose which format to use for saving"?

Change the text only when saving (it really is a filter bar in open mode)

Why not use "File formats" (plural) for the Open dialog?

Change the text only when saving (it really is a filter bar in open mode)

Why not use "File formats" (plural) for the Open dialog?

Uhm, maybe because you end up choosing one of them.

ngraham marked an inline comment as done.Apr 13 2018, 2:01 AM

Change the text only when saving (it really is a filter bar in open mode)

Why not use "File formats" (plural) for the Open dialog?

Because in Open mode, it really is a real actual filter box. You can filter not only on file type, but also by name. So the text "Filter" is accurate.

ngraham updated this revision to Diff 32030.Apr 13 2018, 2:18 AM

Use the more familiar and less technical string "File type"

ngraham retitled this revision from Use the more user-friendly string "File format" in the save dialogs to Use the more user-friendly string "File type" in the save dialogs.Apr 13 2018, 2:47 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham marked an inline comment as done.
davidc added a subscriber: davidc.Apr 13 2018, 2:39 PM

"File Type" is a big improvement over "File format."

Clear, concise, offers consistent capitalization throughout the window, and aligns well with the "Name" field above it.

cfeck added a comment.Apr 13 2018, 4:00 PM

"File Type" is a big improvement over "File format."

Clear, concise, offers consistent capitalization throughout the window, and aligns well with the "Name" field above it.

https://community.kde.org/KDE_Visual_Design_Group/HIG/Capitalization says that we shouldn't use title case for edit box labels.

abetts accepted this revision.Apr 13 2018, 4:33 PM
ngraham planned changes to this revision.Apr 13 2018, 5:38 PM

"File Type" is a big improvement over "File format."

Clear, concise, offers consistent capitalization throughout the window, and aligns well with the "Name" field above it.

https://community.kde.org/KDE_Visual_Design_Group/HIG/Capitalization says that we shouldn't use title case for edit box labels.

Ah, good point!

rkflx added a comment.Apr 13 2018, 9:08 PM

I wonder if having a filter feature even makes any sense when saving files, and if it does--why only for Kate?

Well, you should ask Kate. I suspect this is because there cannot be a static list of file types, because there are so much weird text-based file formats out there. Say you work on .xyz files, and want to save complicated_name_1.xyz and to save on typing you first filter for .xyz or even for complicated_name.xyz which you then simply modify to your needs.

Also, KIO is a library, it can be used in all sorts of applications you don't even know about. Maybe there is some weird use case out there where files need to be saved to places with thousands of entries where filtering is useful…

If necessary I can make the string return to "Filter" for these save-with-filter dialogs, but I'm still left wondering what the feature is actually for or if Kate is affected by a bug that should be fixed...

You absolutely should return to "Filter" for this case (even though I agree that for most dialogs it does not make sense), because this is application-defined behaviour which you don't know a thing about. In particular Kate handles it in such a way that what you type in there is not appended as the file extension. Your change and your help text imply it is, but actually trying it out you'll see that Kate won't append the extension (rightly so, because filtering for *.txt and typing aa.txt as the filename should not result in aa.txt.txt).

At least we'll now have setFilterLabel for customization.


in a new user account

That's a bit pointless in this case, why would you let your reviewers jump through such hoops…

src/filewidgets/kfilewidget.cpp
1340–1341

Personally I hate it when a help text just repeats what's already written in the UI. I'd say here you can utilize …the format the file will be saved in., which helps out everyone not understanding "type" in the first place, looking for help, and then finding something they recognize.

After all, Wikipedia also calls the concept "File format" (keep "type" for the label, though).

1344–1345

Indentation?

src/filewidgets/kfilewidget.h
362

Needs @since and possibly @param.

This addition to the API might also be worth mentioning in the commit message.

rkflx added a comment.Apr 13 2018, 9:19 PM

Forgot something important:

You should look at Okular's Save As too, where the Automatically select filename extension checkbox get's disabled once you deviate from the pre-populated file types, i.e. when you type a custom filter. Obviously you should not change the text of the label dynamically in this case, so you'll have to go with Filter once the widget is editable anyway.

ngraham updated this revision to Diff 32093.Apr 14 2018, 3:32 AM

First fix the casing

If necessary I can make the string return to "Filter" for these save-with-filter dialogs, but I'm still left wondering what the feature is actually for or if Kate is affected by a bug that should be fixed...

You absolutely should return to "Filter" for this case (even though I agree that for most dialogs it does not make sense), because this is application-defined behaviour which you don't know a thing about.

I might need some help with this. It seems that the apps whose save dialogs show a combobox in place of a filter list are creating a QDialog and setting the Mime types list with QDialog::setMimeTypeFilter(). I haven't yet managed to figure out how this call is intercepted and replaced with a KFileDialog, or how KFileDialog can detect that there's a combobox of MIME types instead of a filter widget.

in a new user account

That's a bit pointless in this case, why would you let your reviewers jump through such hoops…

Not sure what you mean?

in a new user account

That's a bit pointless in this case, why would you let your reviewers jump through such hoops…

Not sure what you mean?

Making your reviewers create a new user account while following the test plan for a change which does not depend on configuration files seems like a waste of time. Better include only relevant steps in the test plan.

Btw, the qt tag is from Qt3 days. I'm not sure if it must be richtext, if so then a simple html or even KUIT markups are better choice :)

Not sure about string freeze though..

rkflx added a comment.Apr 20 2018, 4:12 PM

I might need some help with this. It seems that the apps whose save dialogs show a combobox in place of a filter list are creating a QDialog and setting the Mime types list with QDialog::setMimeTypeFilter(). I haven't yet managed to figure out how this call is intercepted and replaced with a KFileDialog, or how KFileDialog can detect that there's a combobox of MIME types instead of a filter widget.

In Gwenview, grep for "Save Image" (has prepopulated combobox) and "Open Image" (does not have combobox, but editable and prepopulated filter). The difference seems to be the missing selectMimeTypeFilter for the latter.

Note than in your comment you are referring to setMimeTypeFilter[s], which for Gwenview is called in both cases, thus not leading into a code path you are interested in.

Did not yet have the time (with all the other Diffs to comment on ;) to look into how this translates to KFileDialog, but does this help already?

I might need some help with this. It seems that the apps whose save dialogs show a combobox in place of a filter list are creating a QDialog and setting the Mime types list with QDialog::setMimeTypeFilter(). I haven't yet managed to figure out how this call is intercepted and replaced with a KFileDialog, or how KFileDialog can detect that there's a combobox of MIME types instead of a filter widget.

In Gwenview, grep for "Save Image" (has prepopulated combobox) and "Open Image" (does not have combobox, but editable and prepopulated filter). The difference seems to be the missing selectMimeTypeFilter for the latter.

Note than in your comment you are referring to setMimeTypeFilter[s], which for Gwenview is called in both cases, thus not leading into a code path you are interested in.

Did not yet have the time (with all the other Diffs to comment on ;) to look into how this translates to KFileDialog, but does this help already?

Yes, and I did more investigation myself, and all the glue is in plasma-integration. Some changes might be needed there too (or maybe all the changes will be needed there, we'll see!)

rkflx added a comment.May 13 2018, 9:57 PM

Yes, and I did more investigation myself, and all the glue is in plasma-integration. Some changes might be needed there too (or maybe all the changes will be needed there, we'll see!)

Are you sure about the relation to plasma-integration? To me it seems that the widget can also be used standalone. Also, in KDE4 plasma-integration was not even there, yet the string was shown in the UI, and different types of dialogs were possible. My guess would be that this can be solved entirely in KIO, perhaps by following code paths relating to selectMimeTypeFilter.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 13 2018, 9:57 PM
rkflx added a comment.May 20 2018, 8:45 PM

@ngraham I guess you are still working on this, or did you move on to other things in the meantime? (And please set the status to "Changes planned", so this does not clog up the review queue).

ngraham planned changes to this revision.May 20 2018, 8:49 PM
rkflx resigned from this revision.Aug 24 2018, 10:48 PM
ngraham abandoned this revision.May 2 2019, 2:49 PM

Superseded by D20964, which is the correct change.