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

[Jan Arne Petersen]

Hi Rui,

On 28/01/16 21:16, Rui Tiago Cação Matos wrote:
> 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?

Yes. There is one text_input object per wl_seat, which can be used to
track the text focus via enter/leave and to enable text input per
wl_suface via enable/disable (renamed in v3).

So there is only text input happening when the surface which has text
focus is also enabled.

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

Added them.

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

Yes text needs also to be valid UTF-8. Bytes seemed easier to use for
different toolkits with UTF-16 strings. But yes one could also use
characters if there is a big demand for it.

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

Yes usually serials are used in Wayland on compositor side to make sure
that requests from clients are valid (setting a cursor in response to an
enter event for example).

Here we need to make sure that events from the compositor still make
sense for the client. In this case serials need to be generated on
client side.

>> +    <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?

Added some more description. Basically when the virtual keyboard is
hidden by pressing some hide button (or sliding it away or whatever)
there needs to be a way for the client to request it again.

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

Removed reset. It is part of update_state now.

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

Yes, they might be not relevant for all input methods. Removed the
defaults, which should be handled by toolkits (what to set when nothing
special is specified on an input widget).

>> +    <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?

"Sets the cursor outline as a rectangle relative to the surface.

Allows the compositor to put a window with word suggestions near the
cursor." from patch v3

>> +    <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?

See better description in v3:
"Allows to atomically send state updates from client.

 This request should follow after a batch of state updating requests
 like set_surrounding_text, set_content_type, set_cursor_rectangle and
 set_preferred_language.

 The flags field indicates why an updated state is sent to the input
 method.

 Reset should be used by an editor widget after the text was changed
 outside of the normal input method flow.

 For "change" it is enough to send the changed state, else the full
 state should be send."

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

"Allows to invoke an action when tapping or clicking the currently
 composed word in the text input.

 This can be used by input methods to offer more word suggestions." from
patch v3

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

Yes text focus also works without hardware keyboard and without
wl_keyboards being there. But if there is a hardware keyboard text focus
must be the same as keyboard focus (or horrible things will happen)

>> +    <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?

It allows in keysym to reference modifiers via a bitfield instead of
having to send an array of strings.

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

That is valid for nearly all events (only send to the client with
text-input focus). Added rectangle arguments to allow to move text input
fields away from underneath a virtual keyboard.

>> +    <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" ?

Composing text == pre-edit.

> 
>> +       The commit text can be used to replace the preedit text on reset
> 
> Can? I think this should be a "should" like below.

It depends a bit what happened exactly to a text field on client side.

>> +       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?

Yes. Added better description to v3

>> +    <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?

There might be no hardware keyboard present (so no wl_keyboard
interface). This is mostly useful for synthetic key events generated by
a virtual keyboard (enter, backspace, cursor, whatever), which cannot be
send via wl_keyboard.

>> +    <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?

See better names/description in PATCH v3:

" <event name="configure_surrounding_text">
    <description summary="configure amount of surrounding text to be
send from client">
    Configure what amount of surrounding text is sent by the
    set_surrounding_text request on the following state information
    updates.
   </description>
   <arg name="before_cursor" type="int"/>
   <arg name="after_cursor" type="int"/>
</event>

<event name="demand_full_state">
  <description summary="demand the full state from client">
  Ask to get full current state information sent from the client via
  state requests (set_surrounding_text, set_content_hint, ...) and
  update_state.
  </description>
  <arg name="serial" type="uint" summary="serial of the latest known
text input state"/>
  <arg name="flags" type="uint" summary="currently unused"/>
</event>
"

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

Thanks,
Jan Arne

-- 
Jan Arne Petersen | jan.petersen at kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts