[Buildroot] [v6] package: add qt5virtualkeyboard
Gaël PORTAY
gael.portay at savoirfairelinux.com
Sat Apr 1 21:37:16 UTC 2017
Hello Thomas,
On Sat, Apr 01, 2017 at 03:09:38PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 30 Mar 2017 13:18:42 -0400, Gaël PORTAY wrote:
> > This patch adds the Qt virtualkeyboard package.
> >
> > Signed-off-by: Gaël PORTAY <gael.portay at savoirfairelinux.com>
> > Reviewed-by: Peter Seiderer <ps.report at gmx.net>
>
> I was about to apply this, but there's still one thing unclear to me.
> I'll be making some minor suggestions along the way (but those I would
> have fixed myself when applying if I didn't had the doubt I'm referring
> to). See below.
>
> > +config BR2_PACKAGE_QT5VIRTUALKEYBOARD
> > + bool "qt5virtualkeyboard"
> > + depends on BR2_PACKAGE_QT5_VERSION_LATEST
> > + depends on BR2_PACKAGE_QT5DECLARATIVE_QUICK
> > + select BR2_PACKAGE_QT5SVG
> > + help
> > + Qt Virtual Keyboard is a virtual keyboard framework that consists of a
> > + C++ backend supporting custom input methods as well as a UI frontend
> > + implemented in QML.
> > +
> > + This module is licensed under GPLv3.
>
> You can remove this part, it is incorrect (can be a commercial
> license), and already documented in the package itself.
Done.
>
> > +if BR2_PACKAGE_QT5VIRTUALKEYBOARD
>
> One empty line should be added here.
>
Done.
> > +config BR2_PACKAGE_QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS
> > + string "language layouts"
> > + default "en_GB"
> > + help
> > + The Virtual Keyboard supports the following languages:
> > + - Arabic (ar_AR)
> > + - Danish (da_DK)
> > + - English (en_GB)
> > + - Finnish (fi_FI)
> > + - French (fr_FR)
> > + - German (de_DE)
> > + - Hindi (hi_IN)
> > + - Italian (it_IT)
> > + - Japanese (ja_JP)
> > + - Korean (ko_KR)
> > + - Norwegian (nb_NO)
> > + - Persian/Farsi (fa_FA)
> > + - Polish (pl_PL)
> > + - Portugese (pt_PT)
> > + - Romanian (ro_RO)
> > + - Russian (ru_RU)
> > + - Simplified Chinese (zh_CN)
> > + - Traditional Chinese (zh_TW)
> > + - Spanish (es_ES)
> > + - Swedish (sv_SE)
> > +
> > + Note: all is a flag for activating all languages.
>
> I believe something like this would be clearer:
>
> Note: the special value "all" can be used to install support
> for all supported languages.
>
Done.
> > +
> > +config BR2_PACKAGE_QT5VIRTUALKEYBOARD_HANDWRITING
> > + bool "handwriting"
> > + help
> > + Handwriting support, with gestures for fullscreen input.
> > +
> > + Lipi Toolkit (LipiTk) is an open source toolkit for online Handwriting
> > + Recognition.
>
> One empty line should be added here.
>
Done.
>
> > +QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS = $(call qstrip,$(BR2_PACKAGE_QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS))
> > +QT5VIRTUALKEYBOARD_ALL_LAYOUTS = $(findstring all,$(QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS))
> > +ifneq ($(strip $(QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS)),)
> > +QT5VIRTUALKEYBOARD_QMAKEFLAGS += CONFIG+="$(foreach lang,$(QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS),lang-$(lang))"
>
> I don't understand how this will behave when the "all" flag is used.
> You will have:
>
> CONFIG+=lang-all
>
> is this actually working?
>
Yes.
The Makefile will call qmake with CONFIG+=lang-all.
I tested it and it works fine. CONFIG+=lang-all acts as an alias for all
languages. You can have a look to [1] & [2].
> > +QT5VIRTUALKEYBOARD_OPENWNN = $(findstring ja_JP,$(QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS))
> > +ifneq ($(strip $(QT5VIRTUALKEYBOARD_OPENWNN)$(QT5VIRTUALKEYBOARD_ALL_LAYOUTS)),)
>
> Is the strip really needed in this condition (and the similar ones
> below) ?
>
> Perhaps we could do:
>
> ifneq ($(findstring zh_CN,$(QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS)),)
> QT5VIRTUALKEYBOARD_INSTALL_PINYIN = YES
> endif
>
> ifneq ($(findstring ja_JP,$(QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS)),)
> QT5VIRTUALKEYBOARD_INSTALL_OPENWNN = YES
> endif
>
> ...
>
> ifneq ($(findstring all,$(QT5VIRTUALKEYBOARD_LANGUAGE_LAYOUTS)),)
> QT5VIRTUALKEYBOARD_INSTALL_PINYIN = YES
> QT5VIRTUALKEYBOARD_INSTALL_OPENWNN = YES
> ...
> endif
>
> And then use the QT5VIRTUALKEYBOARD_INSTALL_<foo> variables to know
> what should be installed.
It is another solution; which is much more nice.
Done.
>
> > +ifneq ($(strip $(QT5VIRTUALKEYBOARD_3RDPARTY_PARTS)),)
>
> strip really needed?
Indeed, the strip here is definelty not needed.
>
> Thanks,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thanks!
I will resend a new version.
Regards,
Gael
[1] https://github.com/qt/qtvirtualkeyboard/blob/5.8/src/config.pri#L23-L44
[2] https://doc.qt.io/qt-5/qtvirtualkeyboard-build.html
More information about the buildroot
mailing list