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

Proposed by Christian Seiler on 2014-02-07
Status: Needs review
Proposed branch: lp:~christian-w/lightdm/qt-binding-keyboard-layouts
Merge into: lp:lightdm
Diff against target: 249 lines (+213/-0)
4 files modified
liblightdm-qt/Makefile.am (+3/-0)
liblightdm-qt/QLightDM/LayoutsModel (+1/-0)
liblightdm-qt/QLightDM/layoutsmodel.h (+60/-0)
liblightdm-qt/layoutsmodel.cpp (+149/-0)
To merge this branch: bzr merge lp:~christian-w/lightdm/qt-binding-keyboard-layouts
Reviewer Review Type Date Requested Status
Michael Terry 2014-02-07 Approve on 2014-05-15
Review via email: mp+205356@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
David Edmundson (david.edmundson) wrote :

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) wrote :

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) wrote :

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

Christian Seiler (christian-w) wrote :

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) wrote :

David?

Robert Ancell (robert-ancell) wrote :

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) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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

Robert Ancell (robert-ancell) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'liblightdm-qt/Makefile.am'
2--- liblightdm-qt/Makefile.am 2014-02-06 15:35:02 +0000
3+++ liblightdm-qt/Makefile.am 2014-02-07 12:39:23 +0000
4@@ -25,10 +25,12 @@
5
6 common_headers = \
7 QLightDM/Greeter \
8+ QLightDM/LayoutsModel \
9 QLightDM/Power \
10 QLightDM/SessionsModel \
11 QLightDM/UsersModel \
12 QLightDM/greeter.h \
13+ QLightDM/layoutsmodel.h \
14 QLightDM/power.h \
15 QLightDM/sessionsmodel.h \
16 QLightDM/usersmodel.h
17@@ -38,6 +40,7 @@
18
19 common_sources = \
20 greeter.cpp \
21+ layoutsmodel.cpp \
22 power.cpp \
23 sessionsmodel.cpp \
24 usersmodel.cpp
25
26=== added file 'liblightdm-qt/QLightDM/LayoutsModel'
27--- liblightdm-qt/QLightDM/LayoutsModel 1970-01-01 00:00:00 +0000
28+++ liblightdm-qt/QLightDM/LayoutsModel 2014-02-07 12:39:23 +0000
29@@ -0,0 +1,1 @@
30+#include "QLightDM/layoutsmodel.h"
31
32=== added file 'liblightdm-qt/QLightDM/layoutsmodel.h'
33--- liblightdm-qt/QLightDM/layoutsmodel.h 1970-01-01 00:00:00 +0000
34+++ liblightdm-qt/QLightDM/layoutsmodel.h 2014-02-07 12:39:23 +0000
35@@ -0,0 +1,60 @@
36+/*
37+ * Copyright (C) 2010-2011 David Edmundson.
38+ * Author: David Edmundson <kde@davidedmundson.co.uk>
39+ *
40+ * This library is free software; you can redistribute it and/or modify it under
41+ * the terms of the GNU Lesser General Public License as published by the Free
42+ * Software Foundation; either version 2 or version 3 of the License.
43+ * See http://www.gnu.org/copyleft/lgpl.html the full text of the license.
44+ */
45+
46+#ifndef QLIGHTDM_LAYOUTS_MODEL_H
47+#define QLIGHTDM_LAYOUTS_MODEL_H
48+
49+#include <QtCore/QAbstractListModel>
50+
51+namespace QLightDM {
52+class LayoutsModelPrivate;
53+
54+class Q_DECL_EXPORT LayoutsModel : public QAbstractListModel
55+{
56+ Q_OBJECT
57+
58+ Q_ENUMS(LayoutModelRoles)
59+
60+public:
61+ enum LayoutModelRoles {
62+ DescriptionRole = Qt::DisplayRole,
63+ NameRole = Qt::UserRole,
64+ ShortDescriptionRole,
65+ InternalDataRole,
66+ };
67+
68+ explicit LayoutsModel(QObject *parent = 0);
69+ virtual ~LayoutsModel();
70+
71+ int rowCount(const QModelIndex &parent) const;
72+ QVariant data(const QModelIndex &index, int role=Qt::DisplayRole) const;
73+
74+ /* These do not technically belong into the model, and could
75+ * in principle be static, but we want to be able to use this
76+ * stuff from QML, so we just add them as methods here. It's
77+ * not optimal, but better than nothing.
78+ *
79+ * The QVariant is the same as the item's InternalDataRole and
80+ * should be considered opaque. Just pass the InternalDataRole
81+ * data of an item to setKeyboardLayout to change the current
82+ * keyboard layout. */
83+ Q_INVOKABLE QVariant getCurrentLayout() const;
84+ Q_INVOKABLE void setCurrentLayout(QVariant layoutData);
85+
86+protected:
87+ LayoutsModelPrivate *d_ptr;
88+
89+private:
90+ Q_DECLARE_PRIVATE(LayoutsModel)
91+};
92+
93+}
94+
95+#endif // QLIGHTDM_LAYOUTS_MODEL_H
96
97=== added file 'liblightdm-qt/layoutsmodel.cpp'
98--- liblightdm-qt/layoutsmodel.cpp 1970-01-01 00:00:00 +0000
99+++ liblightdm-qt/layoutsmodel.cpp 2014-02-07 12:39:23 +0000
100@@ -0,0 +1,149 @@
101+/*
102+ * Copyright (C) 2010-2011 David Edmundson.
103+ * Author: David Edmundson <kde@davidedmundson.co.uk>
104+ *
105+ * This library is free software; you can redistribute it and/or modify it under
106+ * the terms of the GNU Lesser General Public License as published by the Free
107+ * Software Foundation; either version 2 or version 3 of the License.
108+ * See http://www.gnu.org/copyleft/lgpl.html the full text of the license.
109+ */
110+
111+#include "QLightDM/layoutsmodel.h"
112+
113+#include <QtCore/QString>
114+#include <QtCore/QDebug>
115+#include <QtGui/QIcon>
116+
117+#include <lightdm.h>
118+
119+using namespace QLightDM;
120+
121+class LayoutItem
122+{
123+public:
124+ QString name;
125+ QString shortDescription;
126+ QString description;
127+
128+ LightDMLayout *ldmLayout;
129+};
130+
131+namespace QLightDM {
132+class LayoutsModelPrivate {
133+public:
134+ LayoutsModelPrivate(LayoutsModel *parent);
135+ QList<LayoutItem> layouts;
136+
137+ protected:
138+ LayoutsModel * const q_ptr;
139+
140+ void loadLayouts();
141+ private:
142+ Q_DECLARE_PUBLIC(LayoutsModel)
143+};
144+}
145+
146+LayoutsModelPrivate::LayoutsModelPrivate(LayoutsModel* parent) :
147+ q_ptr(parent)
148+{
149+#if !defined(GLIB_VERSION_2_36)
150+ g_type_init();
151+#endif
152+}
153+
154+void LayoutsModelPrivate::loadLayouts()
155+{
156+ GList *ldmLayouts;
157+
158+ ldmLayouts = lightdm_get_layouts();
159+
160+ for (GList* item = ldmLayouts; item; item = item->next) {
161+ LightDMLayout *ldmLayout = static_cast<LightDMLayout*>(item->data);
162+ Q_ASSERT(ldmLayout);
163+
164+ LayoutItem layout;
165+ layout.name = QString::fromUtf8(lightdm_layout_get_name(ldmLayout));
166+ layout.shortDescription = QString::fromUtf8(lightdm_layout_get_short_description(ldmLayout));
167+ layout.description = QString::fromUtf8(lightdm_layout_get_description(ldmLayout));
168+ layout.ldmLayout = ldmLayout;
169+
170+ layouts.append(layout);
171+ }
172+}
173+
174+LayoutsModel::LayoutsModel(QObject *parent) :
175+ QAbstractListModel(parent),
176+ d_ptr(new LayoutsModelPrivate(this))
177+{
178+ Q_D(LayoutsModel);
179+ // Extend roleNames (we want to keep the "display" role)
180+ QHash<int, QByteArray> roles = roleNames();
181+ roles[NameRole] = "name";
182+ roles[ShortDescriptionRole] = "shortDescription";
183+ roles[DescriptionRole] = "description";
184+ roles[InternalDataRole] = "internalData";
185+ setRoleNames(roles);
186+ d->loadLayouts();
187+}
188+
189+LayoutsModel::~LayoutsModel()
190+{
191+ delete d_ptr;
192+}
193+
194+
195+int LayoutsModel::rowCount(const QModelIndex &parent) const
196+{
197+ Q_D(const LayoutsModel);
198+ if (parent == QModelIndex()) {
199+ return d->layouts.size();
200+ }
201+
202+ return 0;
203+}
204+
205+QVariant LayoutsModel::data(const QModelIndex &index, int role) const
206+{
207+ Q_D(const LayoutsModel);
208+
209+ if (!index.isValid()) {
210+ return QVariant();
211+ }
212+
213+ int row = index.row();
214+ switch (role) {
215+ case LayoutsModel::NameRole:
216+ return d->layouts[row].name;
217+ case LayoutsModel::ShortDescriptionRole:
218+ return d->layouts[row].shortDescription;
219+ case LayoutsModel::DescriptionRole:
220+ return d->layouts[row].description;
221+ case LayoutsModel::InternalDataRole:
222+ return qVariantFromValue(static_cast<void *>(d->layouts[row].ldmLayout));
223+ }
224+
225+ return QVariant();
226+}
227+
228+QVariant LayoutsModel::getCurrentLayout() const
229+{
230+ LightDMLayout *layout = lightdm_get_layout();
231+ if (layout)
232+ return qVariantFromValue(static_cast<void *>(layout));
233+ return QVariant();
234+}
235+
236+void LayoutsModel::setCurrentLayout(QVariant layoutData)
237+{
238+ if (layoutData.isValid() && layoutData.canConvert<void *>()) {
239+ LightDMLayout *layout = static_cast<LightDMLayout *>(layoutData.value<void *>());
240+ if (layout)
241+ lightdm_set_layout(layout);
242+ }
243+}
244+
245+#if QT_VERSION >= QT_VERSION_CHECK(5, 0, 0)
246+#include "layoutsmodel_moc5.cpp"
247+#else
248+#include "layoutsmodel_moc4.cpp"
249+#endif

Subscribers

People subscribed via source and target branches