<table><tr><td style="">rkflx requested changes to this revision.<br />rkflx added a comment.<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/D14449">View Revision</a></tr></table><br /><div><div><p>Thanks, looking better than before now. There are still some improvements you could make:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">There is a superfluous space before the comma in the second line.</li>
<li class="remarkup-list-item">I'd prefer the bar to be a bit wider by default. However, it turns out there is a problem with my original suggestion (see inline comment).</li>
<li class="remarkup-list-item">The vertical spacing between the first and the second line is too big, it should be the same as for <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Size:</span></span></span>. However, there should still be enough spacing so it also looks good with the Oxygen style. As far as I can see this is an issue with how Breeze renders the <tt style="background: #ebebeb; font-size: 13px;">KCapacityBar</tt>, in particular the bounding rect contains unnecessary margins (<kbd title="Shift" style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">⇧</kbd><span class="kbd-join" style="padding: 0 4px; color: #92969D;">+</span><kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">Ctrl</kbd>-click on it in GammaRay and compare Breeze and Oxygen). Of course that's material for a patch in a different repo, but the "hole" in your current screenshot does not look good (the second line is closer to the bottom than to the first line, which is bad!), and it would be better to fix the problem there before landing the KIO patch.</li>
</ul></div></div><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/D14449#inline-77226">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kpropertiesdialog.cpp:1184-1186</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="n">QStorageInfo</span> <span style="color: #aa2211">*</span><span class="n">storage</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QStorageInfo</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"/"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">quint64</span> <span class="n">size</span> <span style="color: #aa2211">=</span> <span class="n">storage</span><span style="color: #aa2211">-></span><span class="n">bytesTotal</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">quint64</span> <span class="n">free</span> <span style="color: #aa2211">=</span> <span class="n">storage</span><span style="color: #aa2211">-></span><span class="n">bytesAvailable</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That's a bit odd: Why would you have to query that information here again, if in the original implementation it is already available?</p>
<p style="padding: 0; margin: 8px;">Perhaps all calls to <tt style="background: #ebebeb; font-size: 13px;">setText</tt> should take place only in one part of the code, e.g. <tt style="background: #ebebeb; font-size: 13px;">slotFreeSpaceResult</tt>.</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/D14449#inline-77230">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kpropertiesdialog.cpp:1188</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="n">l</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QLabel</span><span class="p">(</span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_frame</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">grid</span><span style="color: #aa2211">-></span><span class="n">addWidget</span><span class="p">(</span><span class="n">l</span><span class="p">,</span> <span class="n">curRow</span><span class="p">,</span> <span style="color: #601200">2</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">l</span><span style="color: #aa2211">-></span><span class="n">setText</span><span class="p">(</span><span class="n">i18nc</span><span class="p">(</span><span style="color: #766510">"Device usage information"</span><span class="p">,</span><span style="color: #766510">"%1 used , %2 free"</span><span class="p">,</span> <span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">convertSize</span><span class="p">(</span><span class="n">size</span> <span style="color: #aa2211">-</span> <span class="n">free</span><span class="p">),</span> <span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">convertSize</span><span class="p">(</span><span class="n">free</span><span class="p">)));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Do you need <tt style="background: #ebebeb; font-size: 13px;">curRow++</tt> here, in case additional rows are added later on?</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/D14449#inline-77227">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kpropertiesdialog.cpp:1189</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="n">grid</span><span style="color: #aa2211">-></span><span class="n">addWidget</span><span class="p">(</span><span class="n">l</span><span class="p">,</span> <span class="n">curRow</span><span class="p">,</span> <span style="color: #601200">2</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">l</span><span style="color: #aa2211">-></span><span class="n">setText</span><span class="p">(</span><span class="n">i18nc</span><span class="p">(</span><span style="color: #766510">"Device usage information"</span><span class="p">,</span><span style="color: #766510">"%1 used , %2 free"</span><span class="p">,</span> <span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">convertSize</span><span class="p">(</span><span class="n">size</span> <span style="color: #aa2211">-</span> <span class="n">free</span><span class="p">),</span> <span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">convertSize</span><span class="p">(</span><span class="n">free</span><span class="p">)));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Translators will only see the string and the context, so "Device usage information" is not enough. It would be better to also describe what the parameters are for, see <tt style="background: #ebebeb; font-size: 13px;">slotFreeSpaceResult</tt> for an example.</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/D14449#inline-77229">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kpropertiesdialog.cpp:1279</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="n">d</span><span style="color: #aa2211">-></span><span class="n">m_capacityBar</span><span style="color: #aa2211">-></span><span class="n">setMaximumWidth</span><span class="p">(</span><span style="color: #601200">175</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_capacityBar</span><span style="color: #aa2211">-></span><span class="n">setText</span><span class="p">(</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't think we can set a maximum width for the complete bar, because it can conflict with languages with longer translations.</p>
<p style="padding: 0; margin: 8px;">More importantly, for the Oxygen widget style the text in a <tt style="background: #ebebeb; font-size: 13px;">KCapacityBar</tt> might be drawn inline and the bar should span the complete width of the dialog, so I don't think we should fiddle too much with the sizes of <tt style="background: #ebebeb; font-size: 13px;">m_capacityBar</tt> or its sub-components.</p>
<p style="padding: 0; margin: 8px;">I guess we have to live with the longer bar for Breeze, which could already become quite long without your patch if you resized the dialog. For other languages it is also less of an issue.</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/D14449#inline-77228">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kpropertiesdialog.cpp:1281</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; "> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_capacityBar</span><span style="color: #aa2211">-></span><span class="n">setText</span><span class="p">(</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">i18nc</span><span class="p">(</span><span style="color: #766510">"Available space out of total partition size (percent used)"</span><span class="p">,</span> <span style="color: #766510">"%1<span class="bright"> free</span> of %2<span class="bright"> (%3% used)</span>"</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">KIO</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">convertSize</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">available</span></span><span class="bright"></span><span class="p"><span class="bright">)</span>,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18nc</span><span class="p">(</span><span style="color: #766510">"Available space out of total partition size (percent used)"</span><span class="p">,</span> <span style="color: #766510">"%1<span class="bright">% used</span> of %2"</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">percentUsed</span></span><span class="p">,</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Please also update the translation context when you change the string to translate.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14449">https://phabricator.kde.org/D14449</a></div></div><br /><div><strong>To: </strong>shubham, ngraham, Frameworks, rkflx<br /><strong>Cc: </strong>rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns<br /></div>