<table><tr><td style="">asemke added a subscriber: sgerlach.<br />asemke added inline comments.
</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/D14523">View Revision</a></tr></table><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/D14523#inline-76789">View Inline</a><span style="color: #4b4d51; font-weight: bold;">croick</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.cpp:612</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Let's decouple this problem from your patch that is fixing another problem. Let's finalize your patch and submitt it. Maybe <a href="https://phabricator.kde.org/p/sgerlach/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@sgerlach</a> can have another look at this piece of code where already another FIXME is placed.</p></div></div><br /><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/D14523#inline-76792">View Inline</a><span style="color: #4b4d51; font-weight: bold;">croick</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.cpp:1106</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">fileTypeChanged()</tt> gets the fileType as <tt style="background: #ebebeb; font-size: 13px;">int</tt>. I would have to explicitly cast this to <tt style="background: #ebebeb; font-size: 13px;">AbstractFileFilter::FileType</tt> for the call of <tt style="background: #ebebeb; font-size: 13px;">updateContent()</tt>. I thought I should stick to the current code, but I can change it, if you think that it is important.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Let's cast this to the enum value like we do this in ImportFileWidget::fileNameChanged() already and in all other widgets where we cast from integers coming from the comboboxes to the internal enum values, also in order to benefit from the compiler checks for the missing switch-cases. The current code in fileTypeChanged() is not good in this respect, yes. So, if it's not a big issue for you, let's clean up here in your patch.</p></div></div><br /><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/D14523#inline-76790">View Inline</a><span style="color: #4b4d51; font-weight: bold;">croick</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.cpp:624</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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 (<tt style="background: #ebebeb; font-size: 13px;">fileName</tt> in <tt style="background: #ebebeb; font-size: 13px;">absolutePath()</tt> or just the temporary return value) would be deleted once the function returns.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ok. This is a valid point. I forgot that we are having here a new function.</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/D14523">https://phabricator.kde.org/D14523</a></div></div><br /><div><strong>To: </strong>croick, LabPlot<br /><strong>Cc: </strong>sgerlach, fkristof, asemke, kde-edu, narvaez, apol<br /></div>