<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/110019/">http://git.reviewboard.kde.org/r/110019/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 17th, 2013, 5:44 p.m. UTC, <b>Thibaut Gridel</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;">Interesting Patch, please provide the details of what gets changed in AnnotationPlugin for commenting.
>From the commit, I see that the DockWidget is created from the Plugin class and all the UI belongs there.
I think that code should move to another class to separate concerns: the Plugin deals with master data and plugin lifecycle,
then a possible widget class uses data from the Plugin.</pre>
</blockquote>
<p>On April 17th, 2013, 6:18 p.m. UTC, <b>Adrian Draghici</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;">This is the diff for the AnnotatePlugin class: http://paste.kde.org/726710/
Having the virtual function createDockWidget() in RenderPlugin and overriding it in each plugin for a custom widget seems fairly clean to me.
How do you suggest refactoring it?</pre>
</blockquote>
<p>On April 18th, 2013, 5:22 p.m. UTC, <b>Bernhard Beschow</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;">@Thibaut: Yes, I see that as a problem, too. In particular, I think that adding *application-specific* logic to the generic RenderPlugin class (which is supposed to render some features onto the map) is a sign for an architectural issue.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">How shall we move on here? Are there objections against the Dock Widget approach in general? If not I'd suggest moving the code from the plugin somewhere next to ControlView (outside of the library and plugins, to be shared by the Marble Qt and KDE application). In my opinion *all* code from the plugin could move to the applications (the UI related parts) and the library (geographics scene related parts).
Shall we remove, or at least deprecate the existing toolbar functionality in RenderPlugin as well? It's application specific as well and in Marble only used by this plugin.
</pre>
<br />
<p>- Dennis</p>
<br />
<p>On April 16th, 2013, 7:01 p.m. UTC, Adrian Draghici wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Adrian Draghici.</div>
<p style="color: grey;"><i>Updated April 16, 2013, 7:01 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;">KML Editor Dock Widget replacing the previous toolbar used to display the actions provided by the annotate plugin</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=317791">317791</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/QtMainWindow.h <span style="color: grey">(b02cd36)</span></li>
<li>src/QtMainWindow.cpp <span style="color: grey">(cc69642)</span></li>
<li>src/lib/BranchFilterProxyModel.h <span style="color: grey">(3ad87a5)</span></li>
<li>src/lib/RenderPlugin.h <span style="color: grey">(330730a)</span></li>
<li>src/lib/RenderPlugin.cpp <span style="color: grey">(b9b95c9)</span></li>
<li>src/plugins/render/annotate/AnnotatePlugin.h <span style="color: grey">(e6111c2)</span></li>
<li>src/plugins/render/annotate/AnnotatePlugin.cpp <span style="color: grey">(72d2ab3)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110019/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>