D14523: [import] Update file content overview after file type change

Christoph Roick noreply at phabricator.kde.org
Wed Aug 1 22:06:37 BST 2018

croick marked an inline comment as done.
croick added inline comments.


> asemke wrote in ImportFileWidget.cpp:612
> This part Christoph copied simply from another place... This logic we have in many places in the code where we striclty rely on the fixed ordering of the evaluation which has also to be guaranteed by the compiler. Many people are used to this and I think this is ok...

I agree with Alexander. What's actually bothering me here, is the assumption that a relative path is supposed to be relative to the home folder. Maybe there is some reason for this, but it seems weird at least.

> fkristof wrote in ImportFileWidget.cpp:1106
> AbstractFileFilter::FileType fileType

`fileTypeChanged()` gets the fileType as `int`. I would have to explicitly cast this to `AbstractFileFilter::FileType` for the call of `updateContent()`. I thought I should stick to the current code, but I can change it, if you think that it is important.

> asemke wrote in ImportFileWidget.cpp:624
> I meant const reference. Though the difference with respect to the performance between copying an object and creating a reference to it might be very small for implicitly shared Qt classes (QString is implicitely shared), it's still matter of convention and of code styly. With const QString& you _explicitly_ say I do not want to copy and I'm not going to change the reference - this is what we try to consistenly use throughout the code.

I agree that arguments should be passed by reference, not by value, but I cannot have a const reference to a temporary object. The object it is referring to (`fileName` in `absolutePath()` or just the temporary return value)  would be deleted once the function returns.

  R262 LabPlot


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

More information about the kde-edu mailing list