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
