Code review comment for lp://qastaging/~andreagrandi/unity-2d/main

Revision history for this message
Andrea Grandi (andreagrandi) wrote :

Hi,

> Hi, thanks for the patch :)
> I tested it and it works well.

nice to hear :)

> * You are never deleting the monitor that you create with
> monitor = g_file_monitor_directory (file, G_FILE_MONITOR_NONE, NULL, NULL);
> You should probably save it to a member variable and call g_object_unref on it
> in the Trash destructor.

fixed

> * Can you please change the name of the function initTrashIcon to
> updateTrashIcon ? (It's updating the icon many times, not initializing it only
> once)

fixed

> * You don't need to pass to fileChanged all the same arguments as
> fileChangedProxy. In fact you can just have fileChanged without any arguments
> and check if the event_type is the right type in fileChangedProxy.

fixed

> * We have some coding guidelines in place and we can only accept software that
> conform to these. You can find the guidelines in the CODING file in the root
> of the lp:unity-2d repo.

read

> For example you need to change function names like start_monitoring_trash with
> startMonitoringTrash, names of member variables to start with "m_" (like
> m_iconName instead of iconName), names of local variables like _this to
> something like currentInstance or something similar.
> Please have a look at the guidelines for other coding standards we follow
> (like how to put parenthesis after if/else and so on).

fixed

> * Finally, if you did not submit it already, you have to take care of
> accepting the Canonical contributor agreement. You can find instructions at
> http://www.canonical.com/contributors . Hopefully you are ok with that.

done!

Please let me know if the new patch is ok :)

« Back to merge proposal