<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/105252/">http://git.reviewboard.kde.org/r/105252/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In general very nice, but there are a couple of serious issues. See the comments below.</pre>
<br />
<div>
<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/105252/diff/4/?file=67781#file67781line72" style="color: black; font-weight: bold; text-decoration: underline;">plugins/chartshape/ChartConfigWidget.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">public:</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">72</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QIcon</span> <span class="n">datasetMarkerIcon</span><span class="p">(</span><span class="n">KDChart</span><span class="o">::</span><span class="n">MarkerAttributes</span><span class="o">::</span><span class="n">MarkerStyle</span> <span class="n">markerStyle</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">We should isolate kdchart more, not less. This means that we shouldn't introduce a dependency of kdchart enums into the config widget. Instead we should use an ODF specific enum here. This comment goes for all the places where the code references the kdchart marker enum, except for the internals of DataSet.cpp.</pre>
</div>
<br />
<div>
<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/105252/diff/4/?file=67785#file67785line278" style="color: black; font-weight: bold; text-decoration: underline;">plugins/chartshape/ChartTool.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">QWidget *ChartTool::createOptionWidget()</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">278</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">widget</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">dataSetMarkerChanged</span><span class="p">(</span><span class="n">DataSet</span><span class="o">*</span><span class="p">,</span><span class="n">KDChart</span><span class="o">::</span><span class="n">MarkerAttributes</span><span class="o">::</span><span class="n">MarkerStyle</span><span class="p">)),</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Same comment as above regarding kdchart internal enums.</pre>
</div>
<br />
<div>
<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/105252/diff/4/?file=67790#file67790line49" style="color: black; font-weight: bold; text-decoration: underline;">plugins/chartshape/kdchart/src/KDChartMarkerAttributes.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">namespace KDChart {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">enum</span> <span class="n">MarkerStyle</span> <span class="p">{</span> <span class="n">Marker<span class="hl">Circle</span></span> <span class="o">=</span> <span class="mi">0</span><span class="p">,</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">enum</span> <span class="n">MarkerStyle</span> <span class="p">{</span> <span class="n">Marker<span class="hl">Square</span></span><span class="hl"> </span> <span class="o">=</span> <span class="mi">0</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't understand this change at all. Especially since I bothered to make a special comment that we keep the binary compatibility with other kdchart users by keeping the order of the markers. There is no way that kdab is going to accept a patch that changes something fundamental like this. For binary compatibility with old code we can never change enums, only add to them.</pre>
</div>
<br />
<p>- Inge</p>
<br />
<p>On June 15th, 2012, 6:25 a.m., Brijesh Patel 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, Inge Wallin and C. Boemann.</div>
<div>By Brijesh Patel.</div>
<p style="color: grey;"><i>Updated June 15, 2012, 6:25 a.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;">This patch adds UI for markers and implements setting of marker for datasets.</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>plugins/chartshape/ChartConfigWidget.h <span style="color: grey">(aa89815)</span></li>
<li>plugins/chartshape/ChartConfigWidget.cpp <span style="color: grey">(4f4a705)</span></li>
<li>plugins/chartshape/ChartConfigWidget.ui <span style="color: grey">(b142635)</span></li>
<li>plugins/chartshape/ChartTool.h <span style="color: grey">(779b7ef)</span></li>
<li>plugins/chartshape/ChartTool.cpp <span style="color: grey">(bab225a)</span></li>
<li>plugins/chartshape/DataSet.h <span style="color: grey">(023c060)</span></li>
<li>plugins/chartshape/DataSet.cpp <span style="color: grey">(2821254)</span></li>
<li>plugins/chartshape/Legend.cpp <span style="color: grey">(e74a77f)</span></li>
<li>plugins/chartshape/kdchart/src/KDChartAbstractDiagram.h <span style="color: grey">(a410cb8)</span></li>
<li>plugins/chartshape/kdchart/src/KDChartMarkerAttributes.h <span style="color: grey">(f8b7646)</span></li>
<li>plugins/chartshape/kdchartpatches/README <span style="color: grey">(9d3f184)</span></li>
<li>plugins/chartshape/kdchartpatches/arrange-markers.diff <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105252/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>