Merge lp:~christian-w/lightdm/qt-binding-keyboard-layouts into lp:lightdm

[Christian Seiler]

Add support for keyboard layouts to Qt bindings

Propagate keyboard layout API to Qt bindings to make it possible to
write Qt greeters that offer layout selection.

The API is not 100% optimal when it comes to actually choosing the
layout itself (shouldn't be part of the model, but some extra
functions), but being able to call them from QML is otherwise not so
trivial.

[David Edmundson (david.edmundson)]

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.

------

+void LayoutsModel::setCurrentLayout(QVariant layoutData)

const & layoutData. Save yourself a copy.

--

I would register LightDMLayout as a metatype
Q_DECLARE_METATYPE(LightDMLayout)

and then you shouldn't need to cast to void*.

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.
We don't want to let the greeters be able to cause a crash.

--

As for the API. I know what you mean, it's a bit sucky but I would agree with you that this is one of the least terrible ways to do it.

I'd change it to a property. Otherwise you need to have a component.onCompleted which is messy; you have the data when you load, you may as well bind to it initially.

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.

--
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.

[Christian Seiler (christian-w)]

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?

[Robert Ancell (robert-ancell)]

Christian - yes, just push new commits. David, any other opposition to this? I'm just waiting on your approval.

[Christian Seiler (christian-w)]

Hi,

> Christian - yes, just push new commits. David, any other opposition
> to this? I'm just waiting on your approval.

I was actually waiting for a comment on my suggestion w.r.t. the layouts
API that I made here, before I then update the Qt bindings:
http://lists.freedesktop.org/archives/lightdm/2014-February/000509.html

Since I don't think anybody is using the current API yet, this would be
the ideal place to redesign them a bit to make them more useful.

If you agree to changing the API, I would provide a new merge request
for the modified Glib API that also includes Qt bindings for those.

Regards,
Christian

[Robert Ancell (robert-ancell)]

David?

[Robert Ancell (robert-ancell)]

Christian - I think it would be worth fixing this and pushing it in as-is. We can update the API at a later date

[Michael Terry (mterry)]

Looks fine to me. I didn't run/test the code, but looks OK. I'm also fine with updating the layouts API, but seems like we can do that later, agreed.

review: Approve

[David Edmundson (david.edmundson)]

I think it would be a good idea for you to address my first two review comments.
Ignore the bit after "The other approach", the bit beforehand about casting will prevent a potential awkward crash in the future.

But feel free to approve it if you think otherwise.

[Christian Seiler (christian-w)]

> I think it would be a good idea for you to address my first two review
> comments.

I didn't do that because I was waiting for a response to my suggestion
about rewriting the API.

But if you feel you first want to have the bindings for the current API
in before discussing API changes, I'll update my merge request for this
tomorrow.

[David Edmundson (david.edmundson)]

Christian, sorry about that.
I lost track of things and forgot.

[Robert Ancell (robert-ancell)]

Let's merge this as is. Please just update the copyright header.

Unmerged revisions

1886. By Christian Seiler on 2014-02-07

Add support for keyboard layouts to Qt bindings

Propagate keyboard layout API to Qt bindings to make it possible to
write Qt greeters that offer layout selection.

=== modified file 'liblightdm-qt/Makefile.am'
--- liblightdm-qt/Makefile.am	2014-02-06 15:35:02 +0000
+++ liblightdm-qt/Makefile.am	2014-02-07 12:39:23 +0000
@@ -25,10 +25,12 @@
 
 common_headers = \
 	QLightDM/Greeter \
+	QLightDM/LayoutsModel \
 	QLightDM/Power \
 	QLightDM/SessionsModel \
 	QLightDM/UsersModel \
 	QLightDM/greeter.h \
+	QLightDM/layoutsmodel.h \
 	QLightDM/power.h \
 	QLightDM/sessionsmodel.h \
 	QLightDM/usersmodel.h
@@ -38,6 +40,7 @@
 
 common_sources = \
 	greeter.cpp \
+	layoutsmodel.cpp \
 	power.cpp \
 	sessionsmodel.cpp \
 	usersmodel.cpp
 
=== added file 'liblightdm-qt/QLightDM/LayoutsModel'
--- liblightdm-qt/QLightDM/LayoutsModel	1970-01-01 00:00:00 +0000
+++ liblightdm-qt/QLightDM/LayoutsModel	2014-02-07 12:39:23 +0000
@@ -0,0 +1,1 @@
+#include "QLightDM/layoutsmodel.h"
 
=== added file 'liblightdm-qt/QLightDM/layoutsmodel.h'
--- liblightdm-qt/QLightDM/layoutsmodel.h	1970-01-01 00:00:00 +0000
+++ liblightdm-qt/QLightDM/layoutsmodel.h	2014-02-07 12:39:23 +0000
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2010-2011 David Edmundson.
+ * Author: David Edmundson <[e-mail]>
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License as published by the Free
+ * Software Foundation; either version 2 or version 3 of the License.
+ * See http://www.gnu.org/copyleft/lgpl.html the full text of the license.
+ */
+
+#ifndef QLIGHTDM_LAYOUTS_MODEL_H
+#define QLIGHTDM_LAYOUTS_MODEL_H
+
+#include <QtCore/QAbstractListModel>
+
+namespace QLightDM {
+class LayoutsModelPrivate;
+
+class Q_DECL_EXPORT LayoutsModel : public QAbstractListModel
+{
+    Q_OBJECT
+
+    Q_ENUMS(LayoutModelRoles)
+
+public:
+    enum LayoutModelRoles {
+        DescriptionRole = Qt::DisplayRole,
+        NameRole = Qt::UserRole,
+        ShortDescriptionRole,
+        InternalDataRole,
+    };
+
+    explicit LayoutsModel(QObject *parent = 0);
+    virtual ~LayoutsModel();
+
+    int rowCount(const QModelIndex &parent) const;
+    QVariant data(const QModelIndex &index, int role=Qt::DisplayRole) const;
+
+    /* These do not technically belong into the model, and could
+     * in principle be static, but we want to be able to use this
+     * stuff from QML, so we just add them as methods here. It's
+     * not optimal, but better than nothing.
+     *
+     * The QVariant is the same as the item's InternalDataRole and
+     * should be considered opaque. Just pass the InternalDataRole
+     * data of an item to setKeyboardLayout to change the current
+     * keyboard layout. */
+    Q_INVOKABLE QVariant getCurrentLayout() const;
+    Q_INVOKABLE void setCurrentLayout(QVariant layoutData);
+
+protected:
+    LayoutsModelPrivate *d_ptr;
+    
+private:
+    Q_DECLARE_PRIVATE(LayoutsModel)
+};
+
+}
+
+#endif // QLIGHTDM_LAYOUTS_MODEL_H
 
=== added file 'liblightdm-qt/layoutsmodel.cpp'
--- liblightdm-qt/layoutsmodel.cpp	1970-01-01 00:00:00 +0000
+++ liblightdm-qt/layoutsmodel.cpp	2014-02-07 12:39:23 +0000
@@ -0,0 +1,149 @@
+/*
+ * Copyright (C) 2010-2011 David Edmundson.
+ * Author: David Edmundson <[e-mail]>
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License as published by the Free
+ * Software Foundation; either version 2 or version 3 of the License.
+ * See http://www.gnu.org/copyleft/lgpl.html the full text of the license.
+ */
+
+#include "QLightDM/layoutsmodel.h"
+
+#include <QtCore/QString>
+#include <QtCore/QDebug>
+#include <QtGui/QIcon>
+
+#include <lightdm.h>
+
+using namespace QLightDM;
+
+class LayoutItem
+{
+public:
+    QString name;
+    QString shortDescription;
+    QString description;
+    
+    LightDMLayout *ldmLayout;
+};
+
+namespace QLightDM {
+class LayoutsModelPrivate {
+public:
+    LayoutsModelPrivate(LayoutsModel *parent);
+    QList<LayoutItem> layouts;
+
+    protected:
+        LayoutsModel * const q_ptr;
+
+        void loadLayouts();
+    private:
+        Q_DECLARE_PUBLIC(LayoutsModel)
+};
+}
+
+LayoutsModelPrivate::LayoutsModelPrivate(LayoutsModel* parent) :
+    q_ptr(parent)
+{
+#if !defined(GLIB_VERSION_2_36)
+    g_type_init();
+#endif  
+}
+
+void LayoutsModelPrivate::loadLayouts()
+{
+    GList *ldmLayouts;
+
+    ldmLayouts = lightdm_get_layouts();
+
+    for (GList* item = ldmLayouts; item; item = item->next) {
+       LightDMLayout *ldmLayout = static_cast<LightDMLayout*>(item->data);
+       Q_ASSERT(ldmLayout);
+
+       LayoutItem layout;
+       layout.name = QString::fromUtf8(lightdm_layout_get_name(ldmLayout));
+       layout.shortDescription = QString::fromUtf8(lightdm_layout_get_short_description(ldmLayout));
+       layout.description = QString::fromUtf8(lightdm_layout_get_description(ldmLayout));
+       layout.ldmLayout = ldmLayout;
+
+       layouts.append(layout);
+   }
+}
+
+LayoutsModel::LayoutsModel(QObject *parent) :
+    QAbstractListModel(parent),
+    d_ptr(new LayoutsModelPrivate(this))
+{
+    Q_D(LayoutsModel);
+    // Extend roleNames (we want to keep the "display" role)
+    QHash<int, QByteArray> roles = roleNames();
+    roles[NameRole] = "name";
+    roles[ShortDescriptionRole] = "shortDescription";
+    roles[DescriptionRole] = "description";
+    roles[InternalDataRole] = "internalData";
+    setRoleNames(roles);
+    d->loadLayouts();
+}
+
+LayoutsModel::~LayoutsModel()
+{
+    delete d_ptr;
+}
+
+
+int LayoutsModel::rowCount(const QModelIndex &parent) const
+{
+    Q_D(const LayoutsModel);
+    if (parent == QModelIndex()) {
+        return d->layouts.size();
+    }
+
+    return 0;
+}
+
+QVariant LayoutsModel::data(const QModelIndex &index, int role) const
+{
+    Q_D(const LayoutsModel);
+
+    if (!index.isValid()) {
+        return QVariant();
+    }
+
+    int row = index.row();
+    switch (role) {
+    case LayoutsModel::NameRole:
+        return d->layouts[row].name;
+    case LayoutsModel::ShortDescriptionRole:
+        return d->layouts[row].shortDescription;
+    case LayoutsModel::DescriptionRole:
+        return d->layouts[row].description;
+    case LayoutsModel::InternalDataRole:
+        return qVariantFromValue(static_cast<void *>(d->layouts[row].ldmLayout));
+    }
+
+    return QVariant();
+}
+
+QVariant LayoutsModel::getCurrentLayout() const
+{
+    LightDMLayout *layout = lightdm_get_layout();
+    if (layout)
+        return qVariantFromValue(static_cast<void *>(layout));
+    return QVariant();
+}
+
+void LayoutsModel::setCurrentLayout(QVariant layoutData)
+{
+    if (layoutData.isValid() && layoutData.canConvert<void *>()) {
+        LightDMLayout *layout = static_cast<LightDMLayout *>(layoutData.value<void *>());
+        if (layout)
+            lightdm_set_layout(layout);
+    }
+}
+
+#if QT_VERSION >= QT_VERSION_CHECK(5, 0, 0)
+#include "layoutsmodel_moc5.cpp"
+#else
+#include "layoutsmodel_moc4.cpp"
+#endif