<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105428/">http://git.reviewboard.kde.org/r/105428/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 6th, 2012, 9:08 p.m., <b>Jarosław Staniek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/105428/diff/2/?file=71401#file71401line36" style="color: black; font-weight: bold; text-decoration: underline;">libs/main/KoDetailsPane.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class QStandardItemModel;</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">36</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">static</span> <span class="k">const</span> <span class="kt">int</span> <span class="n">iconExtent</span> <span class="o">=</span> <span class="mi">64</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If this is in public API used outside of the lib, I would suggest changing to static method (the same applies to the previewExtent attr).
OR: why the 64 constant itself needs to be in .h, not in .cpp?
If you need the value only in the library (classes that inherit from KoDetailsPane) then please move it to protected area (still, changing to const method is good idea).
Explanation:
http://stackoverflow.com/questions/4470802/exporting-constants-from-a-dll?answertab=active#tab-top
</pre>
</blockquote>
<p>On July 6th, 2012, 9:56 p.m., <b>Friedrich W. H. Kossebau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">These extent values are currently only used in the library, and only by KoDetailsPane and subclasses, not by any consumers of those classes.
So moving to protected might be a good idea (but no need to backport, or?). But then, see below.
About turning the constants into static methods: does the static const integer end up in a real memory location? I always thought of "(static) const int x = 1" as basically a "#define x 1" substitute with advantage of type checking. I see the difference With strings, but for types which match register sizes I thought they would be just use as defined const values, in any case and not just with optimization flags. So that is not true?
Are enums the only way to get that? Anyone knows about that for sure and can give a reference (while I try to search an answer myself)?</pre>
</blockquote>
<p>On July 6th, 2012, 10:01 p.m., <b>Jarosław Staniek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">OK, so moving to protected (and removing initialization from the header) is good idea, no need to backport.
About turning the constants into static methods: it's all related to compatibility with msvc compilers, even when simple types are involved; if I look at Qt API - there are no such use cases.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Okay, will push a fix then. But using enums, to get the const value terms, like done for StdSizes of KIconLoader, see http://api.kde.org/4.8-api/kdelibs-apidocs/kdeui/html/classKIconLoader.html#pub-types
So fine with the following patch? Or should I better have the first letter uppercase, i.e. IconExtent and PreviewExtent?
@@ -34,9 +34,6 @@ class KoDetailsPane : public QWidget, public Ui_KoDetailsPaneBase
Q_OBJECT
public:
- static const int iconExtent = 64;
- static const int previewExtent = 128;
-
KoDetailsPane(QWidget* parent, const KComponentData &_componentData, const QString& header);
virtual ~KoDetailsPane();
@@ -67,6 +64,12 @@ protected slots:
void changePalette();
+protected:
+ enum Extents {
+ iconExtent = 64,
+ previewExtent = 128
+ };
+
</pre>
<br />
<p>- Friedrich W. H.</p>
<br />
<p>On July 5th, 2012, 1:18 p.m., Friedrich W. H. Kossebau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated July 5, 2012, 1:18 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Real reason for this patch is that KFileItem::pixmap(...) is broken for items constructed with KFileItem(mode_t, mode_t, KUrl, bool) and no other mimetype related call before the one to ::pixmap. It will always return a pixmap with the "unknown" icon.
You can see this for files for which there is no preview created (for whatever reason).
But the patch fixes also the size mismatch between the fetched previews (200x200) and the usual ODF preview of 128x128 as well as that the preview just from the icon is set in size 128. And makes the code shorter.
Also load the big previews only on-demand, not by default. Could spare up to ~500 kB (10 files * (128*128 - 64*64) * 4 bytes) in theory, but I did not measure what the real effect is.
Also using KIcon instead of setting an own deep copy of the pixmap per each KoFileListItem should spare even more memory, due to KIcon reusing existing icon pixmaps.
Okay to backport to 2.5 as well?</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tried the recent template pane at least in Krita, Words, Sheets</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>libs/main/KoDetailsPane.h <span style="color: grey">(6da6133)</span></li>
<li>libs/main/KoDetailsPane.cpp <span style="color: grey">(8aa5965)</span></li>
<li>libs/main/KoRecentDocumentsPane.h <span style="color: grey">(98baeb8)</span></li>
<li>libs/main/KoRecentDocumentsPane.cpp <span style="color: grey">(521c7e6)</span></li>
<li>libs/main/KoTemplatesPane.cpp <span style="color: grey">(cc76c05)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105428/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>