Review Request 109966: Don't try to change the icon of the "sort" action menu

[Frank Reininghaus]

> On May 10, 2013, 9:07 a.m., Frank Reininghaus wrote:
> > Thanks for the new patch! Looks reasonable, but I wonder why this could be an issue for the "view_mode" menu at all? The only situation in which this would become a problem is when there is a view mode without icon, right?
> > 
> > I'm OK with this being committed to KDE/4.10 (except for the "view_mode" changes, unless there is a very good reason to change it that I'm overlooking). If I shall do it for you, please let me know.
> > 
> > Well, there is actually a small problem left: Imagine you have set icons for the "Sort by" menu and for "Sort by Date", but not for "Sort by Name". When you switch to sorting by date, the menu's icon will be changed, but it does not change back when you switch to "Sort by Name". But I don't see a straightforward way to fix this right now, and I guess it's a rather unlikely corner case anyway.
> 
> Kai Uwe Broulik wrote:
>     > I guess it's a rather unlikely corner case anyway.
>     Actually, I have all my folders sorted by Date and sometimes, when I have to skim through a huge list I sort them by names :)
> 
> Frank Reininghaus wrote:
>     And did you manually choose icons for the "Sort by" menu and for *some* of the specific "Sort by xyz" actions, but not for all of them? That's the unlikely corner case I meant.
> 
> Daniel Faust wrote:
>     > I wonder why this could be an issue for the "view_mode" menu at all?
>     That's how I interpreted Todd's comment. It shouldn't be a problem, it's just the only case that I found that was similar to the "sort" problem.
>     I'm pretty indifferent about it. It should never happen anyway.
>     
>     > there is actually a small problem left
>     Yes, this could actually become very confusing once the icon designers make use of this feature. The only way I see is to store the default icon before changing the icon for the first time.
>     
>     > I'm OK with this being committed to KDE/4.10
>     I have a KDE account but only committed to amarok as of now. So I would like to do it myself. I would create two separate commits for KDE/4.10 (with changelog entry, etc.) and master. Anything else?
> 
> Daniel Faust wrote:
>     Well I should have taken a look at the git log first, it seems you are merging KDE/4.10 into master once in a while :)

Yes indeed, you only have to commit to KDE/4.10 :-) We don't really have a changelog in kde-baseapps, but make sure that you add "FIXED-IN: 4.10.4" to the commit message.

About the "view_mode" stuff: I'd say that we better leave that out and only change the sorting-related icon handling. Maybe Todd meant that not only the icon in the tool bar, but also the one in the "View/Sort..." menu should be changed, but both is controlled by the action's icon anyway.

I think we can leave the "custom icons set for both the menu and the specific 'sort by' actions" open for the time being (unless you are extremely motivated to fix that too).

Thanks for your work, this is much appreciated!


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109966/#review32299
-----------------------------------------------------------


On May 8, 2013, 1:24 p.m., Daniel Faust wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109966/
> -----------------------------------------------------------
> 
> (Updated May 8, 2013, 1:24 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> When changing the sorting through the "sort" action menu, dolphin will try to change the icon.
> However this change fails and just removes the icon.
> 
> In my opinion there is no need at all to change the icon, it should always stay the same (it can be changed by the user with the kde tool bar editor btw.).
> 
> 
> This addresses bug 255819.
>     http://bugs.kde.org/show_bug.cgi?id=255819
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/dolphinviewactionhandler.cpp c7832d7 
> 
> Diff: http://git.reviewboard.kde.org/r/109966/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Daniel Faust
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130510/b9559321/attachment.htm>