[Differential] [Accepted] D1631: Implement wl_text_input and zwp_text_input_v2 interfaces

[sebas (Sebastian Kügler)]

sebas accepted this revision.
sebas added a reviewer: sebas.
sebas added a comment.
This revision is now accepted and ready to land.


  a few niggles, but generally, look good to me.

INLINE COMMENTS

> outputmanagement.cpp:94
>  {
> +    Q_UNUSED(parent);
>      OutputConfiguration *config = new OutputConfiguration(this);

Seems unrelated (not a problem, unless I'm missing something)

> plasmawindowmanagement.cpp:128
>              windows.removeAll(window);
> +            if (activeWindow == window) {
> +                activeWindow = nullptr;

Seems unrelated (not a problem, unless I'm missing something)

> plasmawindowmanagement.cpp:357
>      Private *p = cast(data);
> -    if (p->desktop == number) {
> +    if (p->desktop == static_cast<quint32>(number)) {
>          return;

Seems unrelated (not a problem, unless I'm missing something)

> plasmawindowmodel.cpp:60
>  
> -    QObject::connect(window, &PlasmaWindow::unmapped,
> -        [window, this] {
> -            const int row = windows.indexOf(window);
> -            if (row != -1) {
> -                q->beginRemoveRows(QModelIndex(), row, row);
> -                windows.removeAt(row);
> -                q->endRemoveRows();
> -            }
> +    auto removeWindow = [window, this] {
> +        const int row = windows.indexOf(window);

the changes in this file are unrelated, but do make sense (I don't care much if you push them together)

> plasma-window-management.xml:20
>  
> -  <interface name="org_kde_plasma_window_management" version="3">
> +  <interface name="org_kde_plasma_window_management" version="4">
>      <description summary="application windows management">

changes here are also unrelated?

> text-input-unstable-v2.xml:153
> +      <entry name="phone" value="4" summary="input a phone number"/>
> +      <entry name="url" value="5" summary="input an URL"/>
> +      <entry name="email" value="6" summary="input an email address"/>

"a URL"

> registry.h:451
> +     *
> +     * Prefer using createTextInputManagerUnstableV0 instead.
> +     * @see createTextInputManagerUnstableV0

A small note about the protocol (i.e. which version of the interface should be used) probably wouldn't hurt (as the two creators may end up being confusing). This is a bit high-level though, for APIDOCs, but still raises this question.

> textinput.h:49
> + *
> + * The TextInput allows to have text composed by the Compositor and be sent to
> + * the client.

move this above the previous paragraph, more logical order (general purpose, then implementation caveats)

> textinput.h:81
> +    /**
> +     * Enable text input in a @p surface (usually when a text entry inside of it has focus).
> +     *

Does this actually show the panel then? (Could be more clear what's expected here, what's the relationship between enable/disable and show*/hide*)

> textinput.h:157
> +         */
> +        Lowercase = 1 << 3,
> +        /**

I think LowerCase, UpperCase and TitleCase feel more in line with the other fields here?

> textinput_interface.h:134
> +         */
> +        Lowercase = 1 << 3,
> +        /**

LowerCase, UpperCase, etc. as well?

> textinput_interface.h:216
> +         */
> +        Datetime,
> +        /**

DateTime?

REPOSITORY
  rKWAYLAND KWayland

BRANCH
  graesslin/text-input

REVISION DETAIL
  https://phabricator.kde.org/D1631

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, Plasma, sebas
Cc: sebas, broulik, plasma-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160525/26125bbb/attachment-0001.html>