<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Dec 5, 2014 at 8:54 PM, Nitul Datt <span dir="ltr"><<a href="mailto:nitul1991@gmail.com" target="_blank">nitul1991@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><p dir="ltr"><br>
On 5 Dec 2014 07:33, "Vedant Agarwala" <<a href="mailto:vedant.kota@gmail.com" target="_blank">vedant.kota@gmail.com</a>> wrote:<br>
><br>
> This is an automatically generated e-mail. To reply, visit: <a href="https://git.reviewboard.kde.org/r/121250/" target="_blank">https://git.reviewboard.kde.org/r/121250/</a><br>
><br>
> shared/CMakeLists.txt (Diff revision 4)<br>
><br>
> 3<br>
><br>
>    "../src/"<br>
><br>
> What is this!!!<br>
><br>
> I just told you in a previous issue not to use such relative includes.<br>
><br>
> Just exclude everything with ".." anywhere in its include line. no "../../foo"<br>
><br>
> Nitul please go through some CMake documentation and understand how (and why) CMake files are written.<br>
><br>
> This is most definitely not supposed to be here.<br>
></p>
</span><p dir="ltr">I had to include the src directory to include the code in the CueFileSupport class found in core-impl/meta. That's why I had to use the relative path.</p></blockquote><div>This is wrong.<br></div><div><br></div><div>I saw the `CueFileSupport::locateCueSheet` function and it seems its not being use anywhere in amarok. Maybe it is the case for the entire CueFileSupport class.</div><div>So move the class (or function) from /src/core-impl/...... to /shared/collectionscanner</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><p dir="ltr"> </p><span class="">
<p dir="ltr">><br>
> shared/CMakeLists.txt (Diff revision 4)<br>
><br>
> 50<br>
><br>
>     "../src/core-impl/capabilities/AlbumActionsCapability.cpp"<br>
><br>
> Again. please remove all of these.<br>
><br>
> Does this even look correct to you?<br>
><br>
> /src/* files are supposed to be in amaroklib and not in collectionscanner.<br>
><br>
> But you will not know what this means until you go through some CMake documentation.<br>
></p>
</span><p dir="ltr">I have to include src/* files because I have included the CueFileSupport class in the new CueSheet class in the collectionscanner. </p><span class="">
<p dir="ltr">><br>
> shared/collectionscanner/CueSheet.cpp (Diff revision 4)<br>
><br>
> 49<br>
><br>
>  <br>
><br>
> again whitespace errors.<br>
><br>
> Setup your IDE/Editor. Ask on IRC (in #kde preferably) if you are not sure how to.<br>
><br>
> Did you just remove manually remove whitespaces in the previous review request? :-D<br>
></p>
</span><p dir="ltr">I use KDevelop and I have set it up to remove trailing spaces. I don't know how they still keep showing up. </p><span class="">
<p dir="ltr">><br>
> Still a lot of newbie mistakes. I cannot do a proper review until you have a successful build.<br>
> Are you expecting me to fix these CMake errors for you?<br>
></p>
</span><p dir="ltr">Ofcourse not, I just wanted your opinion on whether I should use the code from the CueFileSupport class, because in doing so I have to include all the classes it references from src/* in the collectionscanner. <br>
So should I just copy over the code from that file into the CueSheet class with the appropriate changes so that I don't have to include so many classes from src/* in the collectionscanner, which I understand should work as an independent utility.</p></blockquote><div>*Never* just copy code from the same project. It leads to code duplication, which is bad for many reasons. When you arrive at a situation like this, create a common module that is reused (maybe a new class that can be inherited, or a function, or a #include).</div><div><br></div><div>Yes it has to function as an independent  utility.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><p dir="ltr"> </p>
<p dir="ltr">P.S. - I had checked Stackoverflow on how to add src files from other directories in CMake files and it said to use relative paths.</p></blockquote><div>You *can* but it is against KDE guidelines. Classes in the same folder are supposed to be added in CMake files. The cmake file in the topmost directory will link them together if needed.</div><div>We do not a cmake file linking /shared and /src . It means it is not supposed to be done. If you are breaking guidelines, it means you are doing something incorrectly.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">
<p dir="ltr">> - Vedant Agarwala<br>
><br>
><br>
> On December 4th, 2014, 11:52 p.m. UTC, Nitul Datt wrote:<br>
><br>
> Review request for Amarok and Vedant Agarwala.<br>
> By Nitul Datt.<br>
><br>
> Updated Dec. 4, 2014, 11:52 p.m.<br>
><br>
> Repository: amarok<br>
> Description<br>
><br>
> This is an initial implementation for a CueSheet class in the CollectionScanner. There are some errors though, which have yet to be sorted out.<br>
><br>
> Diffs<br>
> shared/CMakeLists.txt (31ca0f4)<br>
> shared/collectionscanner/CueSheet.h (PRE-CREATION)<br>
> shared/collectionscanner/CueSheet.cpp (PRE-CREATION)<br>
> shared/collectionscanner/Directory.h (faacce4)<br>
> shared/collectionscanner/Directory.cpp (a65884b)<br>
><br>
> View Diff</p>
</span><p dir="ltr">--<br>
Regards, <br>
Nitul Datt</p>
</blockquote></div><br></div></div>