DevHeads.net

Review Request 120120: kmenuedit: do not resize app icons

Review request for kde-workspace.

Repository: kmenuedit

Description
Remove code which restricts app icons to 20x20 pixels

Diffs
treeview.cpp 99165ca

Diff: <a href="https://git.reviewboard.kde.org/r/120120/diff/" title="https://git.reviewboard.kde.org/r/120120/diff/">https://git.reviewboard.kde.org/r/120120/diff/</a>

Testing
Build app, run it.
Problem: icon still appears small on dragging (hardcoded size 24x24)

Thanks,

Boris Egorov

Comments

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/09/2014 - 13:06

(Updated Sept. 9, 2014, 5:06 p.m.)

Review request for kde-workspace.

Changes
Add bug number to summary

Summary (updated)
kmenuedit: do not resize app icons (fixes #338883)

Repository: kmenuedit

Description
Remove code which restricts app icons to 20x20 pixels

Diffs
treeview.cpp 99165ca

Diff: <a href="https://git.reviewboard.kde.org/r/120120/diff/" title="https://git.reviewboard.kde.org/r/120120/diff/">https://git.reviewboard.kde.org/r/120120/diff/</a>

Testing
Build app, run it.
Problem: icon still appears small on dragging (hardcoded size 24x24)

Thanks,

Boris Egorov

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/09/2014 - 14:37

(Updated Sept. 9, 2014, 6:37 p.m.)

Review request for kde-workspace.

Changes
Fix dragged item size

Repository: kmenuedit

Description
Remove code which restricts app icons to 20x20 pixels

Diffs (updated)
treeview.cpp 99165ca

Diff: <a href="https://git.reviewboard.kde.org/r/120120/diff/" title="https://git.reviewboard.kde.org/r/120120/diff/">https://git.reviewboard.kde.org/r/120120/diff/</a>

Testing (updated)
Build app, run it.

Thanks,

Boris Egorov

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/09/2014 - 16:10

(Updated Sept. 9, 2014, 8:10 p.m.)

Review request for kde-workspace.

Bugs: 338883
<a href="https://bugs.kde.org/show_bug.cgi?id=338883" title="https://bugs.kde.org/show_bug.cgi?id=338883">https://bugs.kde.org/show_bug.cgi?id=338883</a>

Repository: kmenuedit

Description
Remove code which restricts app icons to 20x20 pixels

Diffs
treeview.cpp 99165ca

Diff: <a href="https://git.reviewboard.kde.org/r/120120/diff/" title="https://git.reviewboard.kde.org/r/120120/diff/">https://git.reviewboard.kde.org/r/120120/diff/</a>

Testing
Build app, run it.

Thanks,

Boris Egorov

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/28/2014 - 12:03

(Updated Sept. 28, 2014, 4:03 p.m.)

Status
This change has been marked as submitted.

Review request for kde-workspace.

Bugs: 338883
<a href="https://bugs.kde.org/show_bug.cgi?id=338883" title="https://bugs.kde.org/show_bug.cgi?id=338883">https://bugs.kde.org/show_bug.cgi?id=338883</a>

Repository: kmenuedit

Description
Remove code which restricts app icons to 20x20 pixels

Diffs
treeview.cpp 99165ca

Diff: <a href="https://git.reviewboard.kde.org/r/120120/diff/" title="https://git.reviewboard.kde.org/r/120120/diff/">https://git.reviewboard.kde.org/r/120120/diff/</a>

Testing
Build app, run it.

Thanks,

Boris Egorov

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Christoph Feck at 09/23/2014 - 17:04

Ship it!

No problem if we want to discuss it longer, and eventually change icon sizes to match text sizes (as is done in Skulpture style) or optionally allow configuring icon sizes.

But right now, limiting to a hardcoded 20px value is a bug, that affects usability on HighDPI screens, and should be fixed.

Anything else will likely affect several places in KDE code, and could be discussed, but not specific to this bug.

- Christoph Feck

On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Christoph Feck at 09/28/2014 - 13:54

<a href="https://techbase.kde.org/Contribute/Get_a_Contributor_Account" title="https://techbase.kde.org/Contribute/Get_a_Contributor_Account">https://techbase.kde.org/Contribute/Get_a_Contributor_Account</a>

- Christoph

On Sept. 28, 2014, 4:03 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/28/2014 - 13:52

Christoph, where can I do this?

- Boris

On Sept. 28, 2014, 4:03 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Christoph Feck at 09/23/2014 - 17:07

Boris, did you request commit rights in the mean time? :)

- Christoph

On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By =?utf-8?Q?Thoma... at 09/09/2014 - 16:02

treeview.cpp
<https://git.reviewboard.kde.org/r/120120/#comment46161>

Maybe rather try to limit to the font height instead?

- Thomas L├╝bking

On Sept. 9, 2014, 6:37 nachm., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/23/2014 - 11:20

..or I still don't get what's wrong, sorry. I see that if I set small icon size to 36 that many apps becomes awful, but kmenuedit looks fine for me.

- Boris

On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/23/2014 - 07:33

Thanks for explanation, Thomas.
I think it would be better to use KIconLoader::Small. As for aligning, is it completely impossible to make it like in dolphin? Icon size is changing and it is still aligned correctly in Compact View mode.
<a href="http://i.imgur.com/ASZwuYG.png" title="http://i.imgur.com/ASZwuYG.png">http://i.imgur.com/ASZwuYG.png</a>
<a href="http://i.imgur.com/VwXzQtX.png" title="http://i.imgur.com/VwXzQtX.png">http://i.imgur.com/VwXzQtX.png</a>

- Boris

On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By =?utf-8?Q?Thoma... at 09/21/2014 - 14:46

The user can set the size to whatever he wants - the question is whether KIconLoader::Small is the preferable icon size *in this particular context* or whether the icon size should follow the text height for a somewhat aligned look.

You don't need to warn the user, rather add Jens Reuterberg to this review for an artistical comment.

- Thomas

On Sept. 9, 2014, 8:10 nachm., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Boris Egorov at 09/21/2014 - 14:41

So, as I understand, Thomas proposes to limit icon size to customized text size in both directions (min/max). On the other hand, I agree with Christoph that we must respect user settings. We should look how similar settings handled in another KDE apps.
I think if we enforce some limitation, we should warn user (at runtime or in documentation at least) that icon size cannot be set to any size.

- Boris

On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Albert Astals Cid at 09/18/2014 - 17:09

Boris? Can you address this comments?

- Albert

On set. 9, 2014, 8:10 p.m., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By =?utf-8?Q?Thoma... at 09/09/2014 - 16:30

Apparently is was fixed to 20px to "somehow align to the font height on my system"

Things may look weird if the icon is "slightly" larger than the text (you don't get a text with a large icon as in an iconview, but a small icon and text with "broken" line spacing.
Also, the icon may as well end up being much smaller than the text (if the small icon size is kept at 22px, but large text is used for a11y reasons)

- Thomas

On Sept. 9, 2014, 8:10 nachm., Boris Egorov wrote:

Re: Review Request 120120: kmenuedit: do not resize app icons (f

By Christoph Feck at 09/09/2014 - 16:09

Why? We use "Small" icon size next to text everywhere (buttons, menu items etc), so we expect the user to set a sane size (and the user expects the developer to respect that size).

- Christoph

On Sept. 9, 2014, 6:37 p.m., Boris Egorov wrote: