Code review comment for lp:~ruben-reina-dev/appcenter/installed-apps-list-fixes

Revision history for this message
Rubén Reina (ruben-reina-dev) wrote :

Hello Adam,

Thank you for your tips and suggestions, they were really useful!

I have added the following changes to the branch:
1. "installed_view" variable in MainWindow is now a static auto-generated
property "public static Views.InstalledView installed_view { get; private
set; }". It always will be assigned because in the "static constructor" of
the MainWindow the variable is assigned with an instance of InstalledView
2. Fix code style issue.

The branch is ready to be reviewed again, so please, check it again when
you can.
Thank you.

2016-06-25 22:26 GMT+02:00 Adam Bieńkowski <email address hidden>:

> Review: Needs Fixing code
>
> Some things about the code:
> * I am not sure if a separate installed_view variable is needed here. In
> vala you can use something like:
> "get; private set;"
>
> * We use "lower_case" variables, and we should keep that also there, the
> name "mainWindow" should become "main_window" to keep the same code style.
>
> * I don't like the use of get_toplevel () here. Maybe you could try to get
> make installed_view a static object, this way, you wouldn't need to get the
> MainWindow instance. If not, you can use Gtk's built in functions to get
> that window like this:
>
> var main_window = (MainWindow)((Gtk.Application)Application.get_default
> ()).get_active_window ();
> Also, make sure that the instance of MainWindow is not null, before doing
> any operations with it.
>
> Otherwise, it looks OK.
> --
>
> https://code.launchpad.net/~ruben-reina-dev/appcenter/installed-apps-list-fixes/+merge/298372
> You are the owner of
> lp:~ruben-reina-dev/appcenter/installed-apps-list-fixes.
>

« Back to merge proposal