Code review comment for lp:~christian-w/lightdm/qt-binding-keyboard-layouts

Thanks for reviewing!

> 37 + * Copyright (C) 2010-2011 David Edmundson.
> 38 + * Author: David Edmundson <email address hidden>
>
> Either I was very drunk when writing this and forgot, or you've clearly copy and pasted this :)
> Remember to set 2014 too.

Well, most of the file was mainly C&P, I don't claim that my change is
terribly deep. ;-) But I'll fix that.

> ------
>
> +void LayoutsModel::setCurrentLayout(QVariant layoutData)
>
> const & layoutData. Save yourself a copy.

Ok.

> I would register LightDMLayout as a metatype
> Q_DECLARE_METATYPE(LightDMLayout)
>
> and then you shouldn't need to cast to void*.

You mean LightDMLayout*, right? And I could probably do that just
internally? Because I don't want to leak Glib types into the Qt API...

(Err, scratch that, see below...)

> Right now if I did
> myLayoutModel.layout = someRandomQObject from inside the QML it will crash.
>
> QVariant will be able to cast it to void* and static_cast<> almost always succeeds.

Oh, I didn't know that... Ooops.

> We don't want to let the greeters be able to cause a crash.

At least the QML ones; C++ can cause crashes left and right
regardless. ;-)

> The other approach I've seen is to have a property with the current
> index. This has the disadvantage of getting confusing if the model
> becomes dynamicly changing; but has the awesome advantage that you can
> select the right index in a combo box etc without having to have an
> awkward loop in javascript.

Oh, that sounds cool, I'll do that then. Since at least the Glib API
currently doesn't provide any possibility for dynamical changes, I don't
think this is going to be a problem, we just assume everything is
static. Once that changes, we could always revisit.

I also had a minor epiphany for the naming convention, how about
'activeLayoutIndex' for the property name?

And then I also don't need to wrap LightDMLayout in a QVariant, since it
will just be a model index -> win/win. :)

> LayoutsModel::data
>
> FYI if you want to pass against QtModelTest (http://qt-project.org/wiki/Model_Test) you'll need to check bounds here i.e row > 0, row < rowCount(), column == 0 etc and return a QVariant otherwise.
> I doubt I do it anywhere else, so I don't really care about this one.

I just C&P'd the other code. ;-) But I don't object to adding additional
safety checks just in case to this part, I'm just not going to touch the
rest.

Regards,
Christian

PS: Since I'm new to launchpad, what is the etiquette for updating merge
requests? Just push additional commits on top of the previous one to the
branch?