<table><tr><td style="">ervin requested changes to this revision.<br />ervin added inline comments.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24846">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24846#inline-140318">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">main.cpp:141</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Does this <tt style="background: #ebebeb; font-size: 13px;">KIconTheme</tt> instance need caching? I recall creating those parses a tonne of files and directories and is quite slow</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Agreed, it'll hit the disk quite a bit, this might be a problem in a function which takes part in bindings (more than one which will change around the same time).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24846#inline-140766">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.cpp:176</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">       <span style="color: #aa4000">return</span> <span class="n">m_model</span><span style="color: #aa2211">-></span><span class="n">selectedTheme</span><span class="p">()</span> <span style="color: #aa2211">!=</span> <span class="n">KIconTheme</span><span style="color: #aa2211">::</span><span class="n">current</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Indentation looks wrong here.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24846#inline-140326">View Inline</a><span style="color: #4b4d51; font-weight: bold;">bport</span> wrote in <span style="color: #4b4d51; font-weight: bold;">IconSizePopup.qml:47-53</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I wiil look if I can simplify that</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I like the intent of trying to have names instead of just indices everywhere... but that might not be feasible without extra complexity we'll want to avoid indeed.</p>

<p style="padding: 0; margin: 8px;">I think the more fundamental problem is that the settings are based on those names, while the invokables (most notably availableIconSizes) are based on indices. Probably something to address, like have names everywhere and just a function to map from index to name in the iconTypeList.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R119 Plasma Desktop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24846">https://phabricator.kde.org/D24846</a></div></div><br /><div><strong>To: </strong>bport, ervin, mart, Plasma, crossi<br /><strong>Cc: </strong>broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart<br /></div>