[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Moving KScreen and libkscreen to extragear
From:       David Edmundson <david () davidedmundson ! co ! uk>
Date:       2013-07-08 18:19:08
Message-ID: CAGeFrHAcVieNGP47UpvDzCdycAowo5_1z5ABvTzh5iPfkjYW7Q () mail ! gmail ! com
[Download RAW message or body]

Code wise, things looks pretty good.

Minor comments:
 - the library is GPL, not LGPL which is the norm for libraries.
Is this deliberate?

 - Inside kscreen you have a copy of the metadata.desktop file twice.
Just have the one and install it to the two places.
You might also want to use the version number from your main CMakeLists.txt
in your .desktop file too, otherwise updating can get messy.

-Generator::biggestOutput
rename the local variable "total"...it's not a total of anything, I had to
re-read it 10 times to realise the code was correct.


In general though +1 from me.

[Attachment #3 (text/html)]

Code wise, things looks pretty good.<div><br></div><div>Minor comments:</di=
v><div>=A0- the library is GPL, not LGPL which is the norm for libraries.</=
div><div>Is this deliberate?</div><div><br></div><div>=A0- Inside kscreen y=
ou have a copy of the metadata.desktop file twice.</div>
<div>Just have the one and install it to the two places.=A0</div><div>You m=
ight also want to use the version number from your main CMakeLists.txt in y=
our .desktop file too, otherwise updating can get messy.</div><div><br></di=
v>
<div>-Generator::biggestOutput</div><div>rename the local variable &quot;to=
tal&quot;...it&#39;s not a total of anything, I had to re-read it 10 times =
to realise the code was correct.</div><div><br></div><div><br></div><div>
In general though +1 from me.</div>


[prev in list] [next in list] [prev in thread] [next in thread]