> On Sept. 9, 2014, 8:02 p.m., Thomas Lübking wrote: > > treeview.cpp, line 232 > > <https://git.reviewboard.kde.org/r/120120/diff/2/?file=310611#file310611line232> > > > > Maybe rather try to limit to the font height instead? > > Christoph Feck wrote: > 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). > > Thomas Lübking wrote: > 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) > > Albert Astals Cid wrote: > Boris? Can you address this comments? > > Boris Egorov wrote: > 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. > > Thomas Lübking wrote: > 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. > > Boris Egorov wrote: > 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. > http://i.imgur.com/ASZwuYG.png > http://i.imgur.com/VwXzQtX.png
..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 ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/#review66152 ----------------------------------------------------------- On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120120/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2014, 8:10 p.m.) > > > Review request for kde-workspace. > > > Bugs: 338883 > https://bugs.kde.org/show_bug.cgi?id=338883 > > > Repository: kmenuedit > > > Description > ------- > > Remove code which restricts app icons to 20x20 pixels > > > Diffs > ----- > > treeview.cpp 99165ca > > Diff: https://git.reviewboard.kde.org/r/120120/diff/ > > > Testing > ------- > > Build app, run it. > > > Thanks, > > Boris Egorov > >
