Differential D12591

KFileWidget: Provide faster access to the icon position setting
AbandonedPublic
Actions

Authored by rkflx on Apr 29 2018, 6:09 AM.

Details

Reviewers
ngraham
Group Reviewers
Frameworks
Maniphest Tasks
T8552: Polish Open/Save dialogs
Summary

The file dialog allows to move the file name labels from next to the
icon to right under the icon for Short View. However, having to go in
a deeply nested Settings button → Icon Position → Above File Name
submenu makes it both hard to discover and cumbersome to use. Ideally
this setting could be accessed directly from the menu for good usability.

Making it easy to change back settings is especially important in light
of D12326, which will enable Icons Above File Name by default.

We linearize the submenu entries from the submenu to the top-level menu,
which works just fine as there are only two entries, increasing the
overall menu size by only one item while removing an entire submenu.

While the submenu itself is not advertised in the API docs, it was
publicly accessible, and therefore is kept for compatiblity reasons
(although with a slight change in wording).

Ref T8552

FIXED-IN: 5.47

Test Plan

kdialog --getopenfilename, switch view modes via the settings
button, check that the icon position settings are only enabled for
View → Short View and still work properly.

Before:

After:

Diff Detail

Repository
R241 KIO
Branch
icon-position-usability (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
rkflx created this revision.Apr 29 2018, 6:09 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 29 2018, 6:09 AM
rkflx requested review of this revision.Apr 29 2018, 6:09 AM
rkflx added a comment.Apr 29 2018, 6:15 AM

If this gets approved, I'll do the same change to K3b for consistency, which was the only other app showing the submenu I could find on https://lxr.kde.org.

elvisangelaccio added a subscriber: elvisangelaccio.Apr 29 2018, 5:09 PM

While the submenu itself is not advertised in the API docs, it was publicly accessible

Where? Isn't it a member of KDirOperator::Private? (I'm referring to decorationMenu)

ngraham accepted this revision.Apr 29 2018, 9:55 PM
ngraham added a subscriber: ngraham.

+1, much better than having a submenu with only two items when the main menu is itself not very big. Works great.

One thought: with this patch, there will be two disabled and inapplicable items in the menu when you're not using Short View, which might confuse people. Might be appropriate to only show them for Short View.

This revision is now accepted and ready to land.Apr 29 2018, 9:55 PM
rkflx planned changes to this revision.Apr 30 2018, 10:44 PM
In D12591#255639, @elvisangelaccio wrote:

While the submenu itself is not advertised in the API docs, it was publicly accessible

Where? Isn't it a member of KDirOperator::Private? (I'm referring to decorationMenu)

That was my first thought too, but unfortunately it leaks out via the actionCollection(). See my patch to K3b (D12598), where it was used that way. I'd rather not break third-party users I don't know about…

In D12591#255732, @ngraham wrote:

One thought: with this patch, there will be two disabled and inapplicable items in the menu when you're not using Short View, which might confuse people. Might be appropriate to only show them for Short View.

Isn't not confusing people what the "disabled state" is there for, compared to making widgets invisible? At least that's how it's done in most other places: Disable options which don't apply. Granted, the connection to Short View is a bit hard to discover, but that was the case before the patch too (even more so).

Nevertheless, I'll hold of with the patch for now. Another more radical way forward would be to merge Next and Above as two separate View modes like in Dolphin (and get rid of the rest?). Seems like we need more discussion about the modes in general, let's do that in T8552.

ngraham added a comment.May 1 2018, 1:57 AM
In D12591#256212, @rkflx wrote:
In D12591#255639, @elvisangelaccio wrote:

While the submenu itself is not advertised in the API docs, it was publicly accessible

Where? Isn't it a member of KDirOperator::Private? (I'm referring to decorationMenu)

That was my first thought too, but unfortunately it leaks out via the actionCollection(). See my patch to K3b (D12598), where it was used that way. I'd rather not break third-party users I don't know about…

In D12591#255732, @ngraham wrote:

One thought: with this patch, there will be two disabled and inapplicable items in the menu when you're not using Short View, which might confuse people. Might be appropriate to only show them for Short View.

Isn't not confusing people what the "disabled state" is there for, compared to making widgets invisible? At least that's how it's done in most other places: Disable options which don't apply. Granted, the connection to Short View is a bit hard to discover, but that was the case before the patch too (even more so).

Nevertheless, I'll hold of with the patch for now. Another more radical way forward would be to merge Next and Above as two separate View modes like in Dolphin (and get rid of the rest?). Seems like we need more discussion about the modes in general, let's do that in T8552.

+1, would approve. I've commented there.

rkflx abandoned this revision.Aug 24 2018, 10:47 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptAug 24 2018, 10:47 PM

Revision Contents
Changeset List

Diff 33273

View Options

src/filewidgets/kdiroperator.cpp

Loading...
View Options

src/filewidgets/kfilewidget.cpp

Loading...