[PATCH wayland-protocols] text: Create second version of text input protocol

[Rui Tiago Cação Matos]

Hi,

thanks for sending this. I was actually going to start a discussion
about the existing protocol but let's go from here instead. Comments
inline:

First a question of scope: a client would only ever need to instance
one text_input object per wl_seat and then "associate" it with a
particular wl_surface through the activate request. Am I right?

Not having a destroy request isn't so bad if that's the intended scope
but I think we should still have one.

On Thu, Jan 28, 2016 at 8:52 AM, Jan Arne Petersen <janarne at gmail.com> wrote:
> +      Text is generally UTF-8 encoded, indices and lengths are in bytes.

Why bytes instead of characters? It seems to me that using characters
is less error prone since an offset in bytes might end up in the
middle of an UTF-8 encoded character.

> +      Serials are used to synchronize the state between the text input and
> +      an input method. New serials are sent by the text input in the
> +      commit_state request and are used by the input method to indicate
> +      the known text input state in evsents like preedit_string, commit_string,
> +      and keysym. The text input can then ignore events from the input method
> +      which are based on an outdated state (for example after a reset).

Any particular reason for serials to be generated and controlled by
clients? For the intended use case it seems ok but it's odd when
compared with other serials in wayland.

> +    <request name="show_input_panel">
> +      <description summary="show input panels">
> +       Requests input panels (virtual keyboard) to show.
> +      </description>
> +    </request>
> +    <request name="hide_input_panel">
> +      <description summary="hide input panels">
> +       Requests input panels (virtual keyboard) to hide.
> +      </description>
> +    </request>

Do we really need these requests? Isn't the fact that a text input is
currently active enough for the compositor/IM to decide on its own if
it should show an input panel?

> +    <request name="reset">
> +      <description summary="reset">
> +       Should be called by an editor widget when the input state should be
> +       reset, for example after the text was changed outside of the normal
> +       input method flow.
> +      </description>
> +    </request>

Shouldn't this have a serial argument if we go with client controlled serials?

Otherwise reset could remain like this but adding specification to
what happens with serials sent in events from this point onwards.

> +    <request name="set_content_type">
> +      <description summary="set content purpose and hint">
> +       Sets the content purpose and content hint. While the purpose is the
> +       basic purpose of an input field, the hint flags allow to modify some
> +       of the behavior.
> +
> +       When no content type is explicitly set, a normal content purpose with
> +       default hints (auto completion, auto correction, auto capitalization)
> +       should be assumed.

No strong opinion, particularly since these are just hints and thus
compositors/IMs are free to ignore them (right?), but I think the
default should be "none".

> +    <request name="set_cursor_rectangle">
> +      <arg name="x" type="int"/>
> +      <arg name="y" type="int"/>
> +      <arg name="width" type="int"/>
> +      <arg name="height" type="int"/>
> +    </request>

This needs specification. Why is it a rectangle and what does it
represent exactly? Are the coordinates relative to the activated
wl_surface? What are compositors expected to do with it?

> +    <enum name="update_state">
> +      <description summary="update_state flags">
> +       Defines the reason for sending an updated state.
> +      </description>
> +      <entry name="change" value="0" summary="updated state because it changed"/>
> +      <entry name="request" value="1" summary="sent state after request"/>
> +      <entry name="reset" value="2" summary="updated state after reset"/>
> +      <entry name="enter" value="3" summary="updated state after switching focus to a different widget on client side"/>
> +    </enum>
> +    <request name="update_state">
> +      <arg name="serial" type="uint" summary="used to identify the known state"/>
> +      <arg name="flags" type="uint" enum="update_state"/>
> +    </request>

This seems very underspecified. Care to elaborate how it's supposed to be used?

> +    <request name="invoke_action">
> +      <arg name="button" type="uint"/>
> +      <arg name="index" type="uint"/>
> +    </request>

Underspecified.

> +    <event name="enter">
> +      <description summary="enter event">
> +       Notification that this seat's text-input focus is on a certain surface.
> +
> +       When the seat has one or more keyboards the text-input focus follows the
> +       keyboard focus.

Wayland clients aren't aware of how many hardware keyboards exist. In
fact there might be none and a compositor is still free to advertise
the keyboard capability and assign keyboard focus (e.g. a touch screen
only device). I don't think we should mention wl_keyboard at all. Just
say that each wl_seat has a text-input focus and this is the enter
event for said focus.

> +    <event name="modifiers_map">
> +      <description summary="modifiers map">
> +       Transfer an array of 0-terminated modifiers names. The position in
> +       the array is the index of the modifier as used in the modifiers
> +       bitmask in the keysym event.
> +      </description>
> +      <arg name="map" type="array"/>
> +    </event>

What's the purpose of this event?

> +    <event name="input_panel_state">
> +      <description summary="state of the input panel">
> +       Notify when the visibility state of the input panel changed.
> +      </description>
> +      <arg name="state" type="uint"/>
> +    </event>

Unsure if we need this event at all. But if we do, it must at least
specify that this event will only be sent if this client currently
holds the text-input focus and the type should be be an enum.

> +    <event name="preedit_string">
> +      <description summary="pre-edit">
> +       Notify when a new composing text (pre-edit) should be set around the
> +       current cursor position. Any previously set composing text should
> +       be removed.

Should that read: "Any previously set _preedit_ text should be removed" ?

> +       The commit text can be used to replace the preedit text on reset

Can? I think this should be a "should" like below.

> +       The text input should also handle all preedit_style and preedit_cursor
> +       events occurring directly before preedit_string.
> +      </description>
> +      <arg name="serial" type="uint" summary="serial of the latest known text input state"/>
> +      <arg name="text" type="string"/>
> +      <arg name="commit" type="string"/>
> +    </event>


> +    <event name="commit_string">
> +      <description summary="commit">
> +       Notify when text should be inserted into the editor widget. The text to
> +       commit could be either just a single character after a key press or the
> +       result of some composing (pre-edit). It could be also an empty text
> +       when some text should be removed (see delete_surrounding_text) or when
> +       the input cursor should be moved (see cursor_position).
> +
> +       Any previously set composing text should be removed.

Again, you mean s/composing/pre-edit/ right?

> +    <event name="cursor_position">
> +      <description summary="set cursor to new position">
> +       Notify when the cursor or anchor position should be modified.
> +
> +       This event should be handled as part of a following commit_string
> +       event.
> +      </description>
> +      <arg name="index" type="int"/>
> +      <arg name="anchor" type="int"/>
> +    </event>

"anchor" needs to be specified. Does it mean that the text between
index and anchor should be selected?

> +    <event name="keysym">
> +      <description summary="keysym">
> +       Notify when a key event was sent. Key events should not be used
> +       for normal text input operations, which should be done with
> +       commit_string, delete_surrounding_text, etc. The key event follows
> +       the wl_keyboard key event convention. Sym is a XKB keysym, state a
> +       wl_keyboard key_state. Modifiers are a mask for effective modifiers
> +       (where the modifier indices are set by the modifiers_map event)
> +      </description>
> +      <arg name="serial" type="uint" summary="serial of the latest known text input state"/>
> +      <arg name="time" type="uint"/>
> +      <arg name="sym" type="uint"/>
> +      <arg name="state" type="uint"/>
> +      <arg name="modifiers" type="uint"/>
> +    </event>

Just like the modifiers_map event above, I don't think having this in
the protocol is a good idea. If the compositor/IM wants to send key
events to a client it can just use the wl_keyboard interface. Am I
missing something?

> +    <event name="request_surrounding_text">
> +      <description summary="request surrounding text from client">
> +       Request to get the surrounding text and cursor position sent from the client.
> +      </description>
> +      <arg name="serial" type="uint" summary="serial of the latest known text input state"/>
> +      <arg name="flags" type="uint"/>
> +      <arg name="before_cursor" type="int"/>
> +      <arg name="after_cursor" type="int"/>
> +    </event>
> +    <event name="request_state">
> +      <description summary="request state from client">
> +       Request to get the surrounding text and cursor position sent from the client.
> +      </description>
> +      <arg name="serial" type="uint" summary="serial of the latest known text input state"/>
> +      <arg name="flags" type="uint"/>
> +    </event>

Both these events need clarification. What's flags? How should clients respond?

I think that's all for now. Thanks,
Rui