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

[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
> 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);

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180502/ca5e76ad/attachment.htm>