Code review comment for lp:~registry/stellarium/socis2015

Revision history for this message
Fabien Chéreau (xalioth) wrote :

Hi Florian, Georg,

thanks, that's huge amount of work that you did! I haven't checked the code in a (too) long time, so it took me time to sort out your changes from the ones of the trunk!
General comment: the code is very clean and very well documented, thanks! That's really professional work :)

Comments on the StelProperty:
 - I understand the need for StelProperty/StelPropertyMgr for scripting and more generally GUI/remote controller etc... it is definitely something missing from the current trunk
 - I am not quite sure about creating a new StelProperty class though, as it doesn't seem brings much more than the regular Qt property. If you would adopt the convention used in QtScript of designing a property by the qobjectname.propertyname you may more or less be able to avoid it I think. The property manager would then simply manage the collection of QObject containing the properties to expose and you could use a more standard approach, which will match the scripting language API. There is somehow a merge to do with the existing scripting API.
 - Also I'm a bit concerned with the partial duplication of features with StelAction, or at least the additional complexity to grasp the difference and when to use which. Did you consider forcing StelAction to use a StelProperty/QProperty and remove the other kind of binding to functions? I think it would make sense.

On the RemoteControl plugin:
 - it would be good to move all related data inside the plugin folder instead of keeping them in data/ (i.e. move data/webroot stuff in the plguing/RemoteControl/ folder)
 - did you double check all third parties libraries licenses?
 - this plugin should also not be activated by default for security reasons (I didn't check if it's the case)

review: Needs Fixing

« Back to merge proposal