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

List:       kde-core-devel
Subject:    Re: Review request: Kcmgrub2
From:       Alberto Mattea <alberto () mattea ! info>
Date:       2011-04-06 13:03:03
Message-ID: 201104061503.07902 () alby
[Download RAW message or body]


Hi, thanks for the review.

In data mercoledì 6 aprile 2011 00:28:21, Raphael Kubo da Costa ha scritto:
> Alberto Mattea <alberto@mattea.info> writes:
> > Hi all,
> > after 4 releases I think kcmgrub2 has reached an acceptable level of
> > maturity, so I'd ask for a move to kdereview. It is currently in
> > playground-sysadmin (git).
> 
> I only took a quick look, as my Py{Qt,KDE}-fu is not that good.
> 
> Buildsystem-wise:
> 
>   * I did not understand why you used include() instead of
> find_package() in, for example,
> 
>       include(FindPyQt4)

Actually I used an article on the KDE wiki as an example, I didn't know it 
wasn't the standard way of doing it. Now it's fixed.

> 
>   * It's probably a good idea to add some kind of README for packagers
> explaining what the dependencies are.

Ok, done.

> 
> Licensing-wise:
> 
>   * Don't you need to add the appropriate license header to your code
> files?

Yep :-). Done.

> 
> As for kcmgrub2.py itself:
> 
>   * It might be better to set the WhatsThis values in the .ui file
> itself.

Well, I thought about this, but ultimately I chose to put them in the code so 
that in the future I may make them dynamic (example: if python-xlib is not 
available explain why in the resolution whatsthis).

>   * `x' is not a good name for a global variable.

You're right, I changed it to xlibAvailable.

Thanks again,

Alberto

["signature.asc" (application/pgp-signature)]

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