D16663: [import] Use QSharedPointer for file filters

[Alexander Semke]

asemke added a comment.


  In D16663#369911 <https://phabricator.kde.org/D16663#369911>, @croick wrote:
  
  > In D16663#367765 <https://phabricator.kde.org/D16663#367765>, @asemke wrote:
  >
  > > - QSharedPointer has additional overhead for reference counting which we don't need in this case. Can we use std::unique_ptr here?
  >
  >
  > Whenever `LiveDataSourcce::setFilter(currentFileFilter())` is called, we cannot know whether the filter object will exist, when it's used in a LiveDataSource.
  
  
  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.
  
  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.

INLINE COMMENTS

> ImportFileWidget.h:103
>  
> +	mutable QSharedPointer<AbstractFileFilter> currentFilter;
> +

m_currentFilter instead of currentFilter for the private member? Why mutable here?

REPOSITORY
  R262 LabPlot

REVISION DETAIL
  https://phabricator.kde.org/D16663

To: croick, #labplot, asemke
Cc: asemke, kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20181204/e09515f1/attachment.html>