Review Request 121250: SoK Implement Cuesheet Support

[vedant agarwala]

On Fri, Dec 5, 2014 at 8:54 PM, Nitul Datt <nitul1991 at gmail.com> wrote:

>
> On 5 Dec 2014 07:33, "Vedant Agarwala" <vedant.kota at gmail.com> wrote:
> >
> > This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121250/
> >
> > shared/CMakeLists.txt (Diff revision 4)
> >
> > 3
> >
> > "../src/"
> >
> > What is this!!!
> >
> > I just told you in a previous issue not to use such relative includes.
> >
> > Just exclude everything with ".." anywhere in its include line. no
> "../../foo"
> >
> > Nitul please go through some CMake documentation and understand how (and
> why) CMake files are written.
> >
> > This is most definitely not supposed to be here.
> >
>
> 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.
>
This is wrong.

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.
So move the class (or function) from /src/core-impl/...... to
/shared/collectionscanner

>  >
> > shared/CMakeLists.txt (Diff revision 4)
> >
> > 50
> >
> >     "../src/core-impl/capabilities/AlbumActionsCapability.cpp"
> >
> > Again. please remove all of these.
> >
> > Does this even look correct to you?
> >
> > /src/* files are supposed to be in amaroklib and not in
> collectionscanner.
> >
> > But you will not know what this means until you go through some CMake
> documentation.
> >
>
> I have to include src/* files because I have included the CueFileSupport
> class in the new CueSheet class in the collectionscanner.
>
> >
> > shared/collectionscanner/CueSheet.cpp (Diff revision 4)
> >
> > 49
> >
> >
> >
> > again whitespace errors.
> >
> > Setup your IDE/Editor. Ask on IRC (in #kde preferably) if you are not
> sure how to.
> >
> > Did you just remove manually remove whitespaces in the previous review
> request? :-D
> >
>
> I use KDevelop and I have set it up to remove trailing spaces. I don't
> know how they still keep showing up.
>
> >
> > Still a lot of newbie mistakes. I cannot do a proper review until you
> have a successful build.
> > Are you expecting me to fix these CMake errors for you?
> >
>
> 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.
> 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.
>
*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).

Yes it has to function as an independent  utility.

>  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.
>
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.
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.

> > - Vedant Agarwala
> >
> >
> > On December 4th, 2014, 11:52 p.m. UTC, Nitul Datt wrote:
> >
> > Review request for Amarok and Vedant Agarwala.
> > By Nitul Datt.
> >
> > Updated Dec. 4, 2014, 11:52 p.m.
> >
> > Repository: amarok
> > Description
> >
> > This is an initial implementation for a CueSheet class in the
> CollectionScanner. There are some errors though, which have yet to be
> sorted out.
> >
> > Diffs
> > shared/CMakeLists.txt (31ca0f4)
> > shared/collectionscanner/CueSheet.h (PRE-CREATION)
> > shared/collectionscanner/CueSheet.cpp (PRE-CREATION)
> > shared/collectionscanner/Directory.h (faacce4)
> > shared/collectionscanner/Directory.cpp (a65884b)
> >
> > View Diff
>
> --
> Regards,
> Nitul Datt
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20141206/4effe592/attachment.html>