<table><tr><td style="">asemke added a comment.
</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/D16663">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D16663#369911" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D16663#369911</a>, <a href="https://phabricator.kde.org/p/croick/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@croick</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D16663#367765" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D16663#367765</a>, <a href="https://phabricator.kde.org/p/asemke/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@asemke</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><ul class="remarkup-list">
<li class="remarkup-list-item">QSharedPointer has additional overhead for reference counting which we don't need in this case. Can we use std::unique_ptr here?</li>
</ul></div>
</blockquote>
<p>Whenever <tt style="background: #ebebeb; font-size: 13px;">LiveDataSourcce::setFilter(currentFileFilter())</tt> is called, we cannot know whether the filter object will exist, when it's used in a LiveDataSource.</p></div>
</blockquote>
<p>Usually there is always a filter which is created for the preview of the selected file. And when we call LiveDataSource::setFilter(currentFileFilter()) we create a new filter in currentFileFilter() anyway if no filter is available yet. So, there will always be an filter object when we close the dialog with Ok.</p>
<p>My point here is, there is actually never a moment where we have more than one reference to this filter - we either use it in ImportFileDialog or later in LiveDataSource. With this we can transfer the ownership when we close the import dialog/widget (use the raw point as "usuall" in LabPlot or make it more explicit via unique_ptr) and there is actually no need for ref-counting. But you have a valid point with the destruction of ImportFileWidget. When destroying this widget we cannot know whether the created filter is going to be used (Ok button clicked vs. Cancel button clicked) without adding additional logic. You propose to use the shared pointer to implement this logic. This is ok, I think. The only not so nice thing about this is that we "pollute" LiveDataSource and MQTTClient because of some internal details of ImportFileWidget... So, if you don't have any other ideas here, submitt your patch. Please also check my minor comment in the code.</p></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/D16663#inline-95013">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.h:103</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">mutable</span> <span class="n">QSharedPointer</span><span style="color: #aa2211"><</span><span class="n">AbstractFileFilter</span><span style="color: #aa2211">></span> <span class="n">currentFilter</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">m_currentFilter instead of currentFilter for the private member? Why mutable here?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R262 LabPlot</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D16663">https://phabricator.kde.org/D16663</a></div></div><br /><div><strong>To: </strong>croick, LabPlot, asemke<br /><strong>Cc: </strong>asemke, kde-edu, narvaez, apol<br /></div>