D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Frameworks, Dolphin, VDG, rkflx.
ngraham requested review of this revision.

REVISION SUMMARY
This patch moves the sort order chooser and options out of the somewhat hidden Settings menu and onto the main toolbar, in conjunction with a few string and icon changes to improve the look and feel.

TEST PLAN
Menu being used in Short view:

Menu being used in Detailed Tree View:

Settings menu no longer has (or needs) it:

It's still there in the contect menu, fully accessible for `KDirOperator` clients:

BRANCH
move-sort-order-chooser-to-toolbar (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D12337

AFFECTED FILES
src/filewidgets/kdiroperator.cpp
src/filewidgets/kfilewidget.cpp

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: anemeth

[Nathaniel Graham]

ngraham updated this revision to Diff 32533.
ngraham added a comment.


Improve a comment

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12337?vs=32532&id=32533

BRANCH
move-sort-order-chooser-to-toolbar (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D12337

AFFECTED FILES
src/filewidgets/kdiroperator.cpp
src/filewidgets/kfilewidget.cpp

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: anemeth

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.
ngraham set the repository for this revision to R241 KIO.
ngraham added a dependency: D12333: Put the open/save dialog's toolbar above all other widgets, like Dolphin does.
Restricted Application added a project: Frameworks.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a task: T8552: Polish Open/Save dialogs.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added a comment.


I know you feel strongly about putting Sort functionality right in the face of users ;) Let me play the devil's advocate to work out whether another solution would be feasible too:

- Introducing a new string to the UI comes at a cost: It draws attention away from what's really important, i.e. the files. I'm not saying sorting is not important, but is it really //that// important like you claim?
- Why would only a single icon get text added next to it?
- The text may be partly motivated by the lack of an appropriate icon. Shouldn't we improve the icons instead?
- This conflicts with Detailed View, where some of the options are more easily accessible by clicking on the table headers. Isn't this confusing for users?

Suggestion for consideration:

- Add well-known <https://www.materialui.co/icon/sort> icon to Breeze.
- Put only an icon in the toolbar, but still in a quite central place so it's within easy reach (as opposed to on the very right).
- Solve the problem where toolbar menu buttons don't get a downwards pointing arrow with Breeze.

F5812548: KIO-toolbar-sort-button.png <https://phabricator.kde.org/F5812548>

I'd prefer this option, but let's hear what others have to say.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: anemeth, michaelh, bruns

[Kai Uwe Broulik]

broulik added a comment.


I don't like how the button floats in "mid air" between the two sides of the toolbar

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: broulik, anemeth, michaelh, bruns

[Mark Gaiser]

markg added a comment.


Sort already is "right in front" of the user. They merely have to click the column headers.
These column headers used to (before the breeze theme, i think the air theme had it) show arrows indicating if something was sorted in ascending or descending.
I don't know why that's gone but that was the universal way to immediately know which column was sorted and in which order. No need to add a button for that.

If the view needs to stay as clean as it is now then you could only show the arrows in the headers when you hover the header (or view) or when it has focus and let them disappear when it loses focus.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.

Post by Mark Gaiser
Sort already is "right in front" of the user. They merely have to click the column headers.

...Only if the view has visible columns. Short View and Tree View don't.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: markg, broulik, anemeth, michaelh, bruns

[Mark Gaiser]

markg added a comment.

Post by Nathaniel Graham

Post by Mark Gaiser
Sort already is "right in front" of the user. They merely have to click the column headers.

...Only if the view has visible columns. Short View and Tree View don't.

Right, then it should be view dependent.
For the views you posted screenshots of (detailed view) the button would be a waste of space.
To me this sounds more useful as a right mouse button menu (Sort by) on views that don't have header names. In a tree view that also allows for fine grained sorting control (aka, only sort the tree node you clicked on)

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.

Post by Mark Gaiser

Post by Nathaniel Graham

Post by Mark Gaiser
Sort already is "right in front" of the user. They merely have to click the column headers.

...Only if the view has visible columns. Short View and Tree View don't.

Right, then it should be view dependent.
For the views you posted screenshots of (detailed view) the button would be a waste of space.
To me this sounds more useful as a right mouse button menu (Sort by) on views that don't have header names. In a tree view that also allows for fine grained sorting control (aka, only sort the tree node you clicked on)

It's already in the context menu. Context menus are expert features, though. Making it view-dependent is an interesting idea, but I'd worry that having it disappear when Detailed Tree View is being used would give users the idea that said view isn't sortable.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.

Post by Henrik Fehlauer
- Introducing a new string to the UI comes at a cost: It draws attention away from what's really important, i.e. the files. I'm not saying sorting is not important, but is it really //that// important like you claim?
- Why would only a single icon get text added next to it?
- The text may be partly motivated by the lack of an appropriate icon. Shouldn't we improve the icons instead?
- This conflicts with Detailed View, where some of the options are more easily accessible by clicking on the table headers. Isn't this confusing for users?

Being able to easily change the sort order between alphabetical and date modified is a critically important feature. Those are the two most frequently used and most commonly useful sort orders. Right now, for Detailed Tree View, this takes one click: good. But for Short View, it's quite hidden: you need to right-click in the view, or click on the settings icon in the corner that evidence suggests people don't use. And both require the at least-intermediate mousing skill of being able to use a sub-menu.

Adding text to the button is motivated by the lack of a good icon, that's true. But I'm not sure this is a surmountable problem. I don't think I've ever seen an icon that adequately communicated "sorting options here!" all by itself. Have you? An icon-only button would not be sufficient here unless we can find a //perfect// icon.

Post by Henrik Fehlauer
- Solve the problem where toolbar menu buttons don't get a downwards pointing arrow with Breeze.

Definitely. That's been on my to-do list for a while. Currently, they get an arrow only if the button is set up to show a menu on a click-and-hold, but not on a regular click. That's just a regular old bug IMHO.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added a subscriber: andreaska.
rkflx added a comment.

Post by Henrik Fehlauer
F5812548: KIO-toolbar-sort-button.png <https://phabricator.kde.org/F5812548>

@broulik @markg Do you like the screenshot above better? The button does not float in the middle, and takes less space.

@andreaska Any tips for a better sort icon? Would it be too much to ask for a new one? (Our current icons often contain characters or a sorting direction, while here we'd need a more generic icon describing a menu containing various sorting options.)

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added a comment.


@ngraham One more idea: Instead of having the text right in the toolbar, move it as a header inside the menu, e.g. like this:

--- Sort by ---
Name
Size
...

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.

Post by Henrik Fehlauer
--- Sort by ---
Name
Size
...

If we want to emphasize the settings menu, I think we need to make it a lot more prominent and discoverable. I'd be on board with something like this if we gave it text:

[sliders icon] More settings

Or something like that.

Also, we really need to fix the look of headers in Breeze menus...

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


FWIW, here's the macOS sort icon, as shown in the open dialog:

F5813078: 0-macOS open.png <https://phabricator.kde.org/F5813078>

I'm not sure it's good enough, and I know that it's not considered very discoverable in the Mac world.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added a comment.

Post by Nathaniel Graham

Post by Henrik Fehlauer
--- Sort by ---
Name
Size
...

[sliders icon] More settings

Ah, I was not talking about the settings menu. I wanted to show a text-less button, and have the "Sort By" text only visible inside the menu of this separate sort button.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Mark Gaiser]

markg added a comment.

Post by Henrik Fehlauer

Post by Henrik Fehlauer
F5812548: KIO-toolbar-sort-button.png <https://phabricator.kde.org/F5812548>

@broulik @markg Do you like the screenshot above better? The button does not float in the middle, and takes less space.
@andreaska Any tips for a better sort icon? Would it be too much to ask for a new one? (Our current icons often contain characters or a sorting direction, while here we'd need a more generic icon describing a menu containing various sorting options.)

Well, the icon certainly is better and without text is much better as well, but i still don't like (nor see the value) of having this sort option that prominent "in your face".
You can sort right now via:

- RMB on the list -> Sorting -> take your pick
- Settings icon -> Sorting -> take your pick
- one-click sorting on the headers (if you open a view with headers)

You think we need a **fourth** option?

I did also just notice that the arrow icon in a header is visible when the view is sorted on that column.
On top of that, the default view (when you don't change anything as a user) is a the detailed view (thus with a visible sort indicator) and one-click sorting on the headers.
You can't satisfy all users, in "my" opinion the current way (without this patch) is fine.

Just because the space is there now doesn't mean the it should be filled with icons ;)

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.

Post by Mark Gaiser
Well, the icon certainly is better and without text is much better as well, but i still don't like (nor see the value) of having this sort option that prominent "in your face".
- Settings icon -> Sorting -> take your pick

This patch removes that.

Post by Mark Gaiser
- RMB on the list -> Sorting -> take your pick

I can remove it from there too if you'd like :-) (only for the file dialog obviously, not for all `KDirOperator` clients)

Post by Mark Gaiser
- one-click sorting on the headers (if you open a view with headers)

Again, only if the view shows column headers.

Post by Mark Gaiser
I did also just notice that the arrow icon in a header is visible when the view is sorted on that column.

If even you--an expert software developer--only just noticed that now, I think that's a fairly good confirmation that it's not a very discoverable feature. :-)

Post by Mark Gaiser
On top of that, the default view (when you don't change anything as a user) is a the detailed view

Not yet; the default view is still Short View, which doesn't show column headers. Even if we change it (in D12327 <https://phabricator.kde.org/D12327>), we will still have one of the two primarily-featured modes lacking headers and therefore an easilt discoverable way to change the sort order.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


If I step back, I think one reason why I find myself objecting to icon-only toolbuttons is because of an in-my-opinion unfortunate confluence of factors that make them difficult to distinguish as interactive UI elements:

- No border: doesn't actually look like a button unless hovered over
- No text: often difficult to distinguish meaning
- Minimalistic monochrome line-art icons in Breeze theme: often doesn't scream "I'm a clickable icon!" and can resemble decorations instead--or even worse, mere visual noise
- No downward-pointing arrows on menu buttons

IMHO, this is a classic case of an "excess of minimalism," where too much symbolism and meaning has been drained away in an effort to distill something to its basic essence. The effort was successful, but went too far!

Icon-only toolbuttons IMHO only really work where they use icons that are truly universally recognizable. If we can't find such an icon, I think it's hard to make the case that an icon-only toolbutton is appropriate.

Nevertheless, this needs to be a broader discussion, and we can't address it as a part of the open/save dialog initiative. Because of this, we need to work around some of the limitations it imposes on us IMHO.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added a comment.


Hm, now you are getting quite off-topic. I thought we wanted to make sorting easier?

Post by Nathaniel Graham
Nevertheless, this needs to be a broader discussion, and we can't address it as a part of the open/save dialog initiative. Because of this, we need to work around some of the limitations it imposes on us IMHO.

On the contrary: Because of this, the sorting button should blend into the existing style, until the broader discussion reached an agreement. Adding workarounds everywhere where you push your personal opinion causes inconsistency and problems down the road when issues are fixed at their proper place.

Your points are generic, you don't answer the question why it is suddenly the sorting button where all issues ("perfect" icon, text, button-like look etc.) have to be fixed immediately. You may not agree with some styling choices, but bringing the topic up in every review can get slightly tiring for everyone involved. Fix it in Breeze, instead, if you must.

It's just a sorting button, and even macOS does not display text for it or has a perfect icon. To get what you want you even have to move all buttons to the side to create space, creating more problems


While you are discussing about the ideal button, people are objecting to the very idea to have this button. I'd rather go for the compromise here, than in the end to have no button at all. Personally I don't use sorting often and know of nobody who does, nevertheless I'd agree to the experiment of making it available more broadly. This does not mean sorting has to be the literal centerpiece of the dialog, though.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham planned changes to this revision.
ngraham added a comment.


You make a lot of good points, Henrik. One of these days I'll learn that whenever you and I disagree, you're probably right! :-)

I'll agree to an icon-only button if we can get a better icon and make Breeze display downward-pointing arrows on menu buttons.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


Better "sort options" icon - https://bugs.kde.org/show_bug.cgi?id=393318
Bring back arrows in menu toolbuttons - https://bugs.kde.org/show_bug.cgi?id=344746

On the arrow issue, apparently this is actually a design decision and not a bug. If others also feel that this should be revisited, I would appreciate some support in the matter.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham updated this revision to Diff 33459.
ngraham added a comment.


- Address review comments

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12337?vs=32533&id=33459

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

AFFECTED FILES
src/filewidgets/kdiroperator.cpp
src/filewidgets/kfilewidget.cpp

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


Now we have a problem. With no text in the toolbar button, the actual items don't reveal their purpose so readily unless we change all of their strings to say "Sort by Name" (etc.). But if we do that, then the context menu will have "Sorting > Sort by name. Thoughts?

The issue is that the strings for the submenu items are intimately tied to the text of the parent menu item or toolbar button. Opposition to a toolbar button with a label leads the submenu items being labeled wrong wrong for one of those two UIs.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added a comment.

Post by Nathaniel Graham
Now we have a problem. With no text in the toolbar button, the actual items don't reveal their purpose so readily unless we change all of their strings to say "Sort by Name" (etc.). But if we do that, then the context menu will have "Sorting > Sort by name. Thoughts?

What about injecting a header as proposed in D12337#249821 <https://phabricator.kde.org/D12337#249821>?

Or could we simply remove the context menu (only for the file dialog)?

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added a comment.

Post by Nathaniel Graham
the context menu will have "Sorting > Sort by name. Thoughts?

Actually I'd still find that kind of acceptable.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham updated this revision to Diff 33464.
ngraham added a comment.


- Address review comments: Sorting > Sort by Name

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12337?vs=33459&id=33464

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

AFFECTED FILES
src/filewidgets/kdiroperator.cpp
src/filewidgets/kfilewidget.cpp

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


Yeah, once all is said and done, we might be able to remove both the View and Sorting menu items from the file dialog's context menu.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

kdiroperator.cpp:1888
d->actionCollection->addAction(QStringLiteral("sorting menu"), sortMenu);
+ sortMenu->setIcon(QIcon::fromTheme(QStringLiteral("itemize")));

While we agreed upon wanting a better icon, until that's done I'd prefer `view-sort-ascending` instead. For me, `itemize` has no connection to sorting at all, sorry.

I'm aware my alternative shows a specific mode, but TBH I don't think users will be put off too much by this detail, in particular because it is the only sorting-related icon in the dialog.

Anyway, that's just my preference. Let me know if you think `itemize` is vastly better.

kfilewidget.cpp:365
opsWidgetLayout->setSpacing(0);
- //d->toolbar = new KToolBar(this, true);
- d->toolbar = new KToolBar(d->opsWidget, true);
+ d->toolbar = new KToolBar(this, true);
d->toolbar->setObjectName(QStringLiteral("KFileWidget::toolbar"));

?

kfilewidget.cpp:365-369
- //d->toolbar = new KToolBar(this, true);
- d->toolbar = new KToolBar(d->opsWidget, true);
+ d->toolbar = new KToolBar(this, true);
d->toolbar->setObjectName(QStringLiteral("KFileWidget::toolbar"));
d->toolbar->setMovable(false);
- opsWidgetLayout->addWidget(d->toolbar);

?

kfilewidget.cpp:554-559
+ // Tweak the look and feel of the sort menu button
+ foreach(QToolButton* button, d->toolbar->findChildren<QToolButton*>()) {
+ if (button->defaultAction() == coll->action(QStringLiteral("sorting menu"))) {
+ button->setPopupMode(QToolButton::InstantPopup);
+ }
+ }

This also worked for me, and would avoid the `foreach`:

KActionMenu *x = new KActionMenu(QIcon::fromTheme(QStringLiteral("configure")), i18n("Options"), this);
x->setMenu(coll->action(QStringLiteral("sorting menu"))->menu());
x->setDelayed(false);
d->toolbar->addAction(x);

kfilewidget.cpp:561
+
+
KUrlCompletion *pathCompletionObj = new KUrlCompletion(KUrlCompletion::DirCompletion);

Unintentional newline?

kfilewidget.cpp:1410
boxLayout->setMargin(0); // no additional margin to the already existing
+ boxLayout->addWidget(toolbar);

?

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham updated this revision to Diff 33498.
ngraham added a comment.


Revert accidental merge, and simplify the single-click menu activation

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12337?vs=33464&id=33498

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

AFFECTED FILES
src/filewidgets/kdiroperator.cpp
src/filewidgets/kfilewidget.cpp

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham marked 6 inline comments as done.
ngraham added inline comments.

INLINE COMMENTS

rkflx wrote in kdiroperator.cpp:1888
While we agreed upon wanting a better icon, until that's done I'd prefer `view-sort-ascending` instead. For me, `itemize` has no connection to sorting at all, sorry.
I'm aware my alternative shows a specific mode, but TBH I don't think users will be put off too much by this detail, in particular because it is the only sorting-related icon in the dialog.
Anyway, that's just my preference. Let me know if you think `itemize` is vastly better.

The problem conceptually is that `view-sort-ascending` is semantically inaccurate for anything but ascending order. We don't actually have an icon yet that means "general sort options are here!" That's covered by https://bugs.kde.org/show_bug.cgi?id=393318, and I've pinged Andreas again. No matter what flawed we choose as a placeholder, I'm going to wait for the better icon before landing this, so for now let's just leave it the way it is.

rkflx wrote in kfilewidget.cpp:365
?

Oops, somehow `arc` merged this patch with D12333 <https://phabricator.kde.org/D12333> when I downloaded it. Not sure how I managed to do that. Cleaned up now.

rkflx wrote in kfilewidget.cpp:365-369
?

Same, sorry.

rkflx wrote in kfilewidget.cpp:554-559
KActionMenu *x = new KActionMenu(QIcon::fromTheme(QStringLiteral("configure")), i18n("Options"), this);
x->setMenu(coll->action(QStringLiteral("sorting menu"))->menu());
x->setDelayed(false);
d->toolbar->addAction(x);

Found an even simpler way. :)

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham marked 8 inline comments as done.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

kdiroperator.cpp:1887
KActionMenu *sortMenu = new KActionMenu(i18n("Sorting"), this);
+ sortMenu->setIcon(QIcon::fromTheme(QStringLiteral("itemize")));
+ sortMenu->setDelayed(false);

Before the patch lands, this icon will be changed to whatever https://bugs.kde.org/show_bug.cgi?id=393318 yields.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.
This revision is now accepted and ready to land.


Thanks, LGTM now.

INLINE COMMENTS

ngraham wrote in kdiroperator.cpp:1888
The problem conceptually is that `view-sort-ascending` is semantically inaccurate for anything but ascending order. We don't actually have an icon yet that means "general sort options are here!" That's covered by https://bugs.kde.org/show_bug.cgi?id=393318, and I've pinged Andreas again. No matter what flawed we choose as a placeholder, I'm going to wait for the better icon before landing this, so for now let's just leave it the way it is.

I think it is a misconception that toolbar icons represent state. I don't know of any toolbar in our software where this is the case, so why should users suddenly expect that what's on the icon represents exactly what is happening, e.g. A-Z ascending? It is merely an example of what type of actions they can expect when clicking on the button.

Icons are a symbolic representation of general concept, not a literal display of a specific state.

---

Anyway, if you want block everything on that, that's what we'll do.

REPOSITORY
R241 KIO

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham added a comment.


I see your point @rkflx, but I think it's a mistake to use an icon whose appearance suggests that it represents state when in fact it doesn't--that's the kind of thing that causes the misconception. Since ascending alphabetical order is one of the available states, using an icon that with "ascending" and "alphabetical" iconography is bound to confuse and frustrate a few people who mistakenly expect it to reflect the active sort order.

Either way, since this contains a string change, it's headed for 5.47 anyway (thanks for being a stickler and reminding me to pay attention to this), so I think we have some time to get a better icon.

REPOSITORY
R241 KIO

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: andreaska, markg, broulik, anemeth, michaelh, bruns

[Nathaniel Graham]

ngraham updated this revision to Diff 34312.
ngraham added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


Merge master

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12337?vs=33498&id=34312

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

AFFECTED FILES
src/filewidgets/kdiroperator.cpp
src/filewidgets/kfilewidget.cpp

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham marked 2 inline comments as done.
ngraham added inline comments.

INLINE COMMENTS

rkflx wrote in kdiroperator.cpp:1888
I think it is a misconception that toolbar icons represent state. I don't know of any toolbar in our software where this is the case, so why should users suddenly expect that what's on the icon represents exactly what is happening, e.g. A-Z ascending? It is merely an example of what type of actions they can expect when clicking on the button.
Icons are a symbolic representation of general concept, not a literal display of a specific state.
---
Anyway, if you want block everything on that, that's what we'll do.

Seems like the new icon may be a long time in coming, and after giving it more thought, I've come around to your position (an increasingly common occurrence! :) ).

REPOSITORY
R241 KIO

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham updated this revision to Diff 34313.
ngraham added a comment.


`view-sort-ascending` it is, for now

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12337?vs=34312&id=34313

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

AFFECTED FILES
src/filewidgets/kdiroperator.cpp
src/filewidgets/kfilewidget.cpp

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham added a comment.


Done. I've created the `file-dialog-improvements` branch and landed this there.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham closed this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Henrik Fehlauer]

rkflx added a comment.


Let's commit this on a separate branch, as getting micro-approvals from #Frameworks <https://phabricator.kde.org/tag/frameworks/> does not work that well. Then we can implement (in the form of reviewed Diffs as before) what we already discussed in a bigger setting, e.g.

- rework the view modes
- set new defaults
- put the New folder button on the bottom
- decide on D12647: Move the inline preview button into the menu <https://phabricator.kde.org/D12647>
- figure out D12218: Remove Reload button from the file dialogs' toolbar <https://phabricator.kde.org/D12218>
- figure out Up
- hopefully abandon D12333: Put the open/save dialog's toolbar above all other widgets, like Dolphin does <https://phabricator.kde.org/D12333> for good


and finally present the final outcome to a wider audience for approval before merging. This way can we make progress as time allows, without shipping incomplete features or get bogged down with dependencies. Note I'm not proposing to wait for //everything// you put into T8552 <https://phabricator.kde.org/T8552>.

(Sorry for not being able to help more currently, but I first wanna reduce my Gwenview and Spectacle backlogs
)

REPOSITORY
R241 KIO

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham removed a dependency: D12333: Put the open/save dialog's toolbar above all other widgets, like Dolphin does.

REPOSITORY
R241 KIO

BRANCH
arcpatch-D12337

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D12337

To: ngraham, #frameworks, #dolphin, #vdg, rkflx
Cc: kde-frameworks-devel, andreaska, markg, broulik, anemeth, michaelh, ngraham, bruns