[PATCH weston-ivi-shell v4 1/9] ivi application protocol:
Pekka Paalanen
ppaalanen at gmail.com
Wed Apr 23 03:40:37 PDT 2014
Hi,
it's been a long while since I have looked at this, but I got a bit of
time to come back. I hope you haven't abandoned this effort yet. :-)
I looked at the PDF from your post on March 6th, 2014, and some of my
own comments I gave at that time to recall what this was about, but I
probably still forgot something.
New comments below. I started by reading the global interface, and
moved on to ivi_surface after it, so the comments might seem
temporally strange if you read just top-down.
On Mon, 17 Mar 2014 15:23:22 +0900
Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp> wrote:
> Add interface ivi_application, which creates ivi_surface objects tied
> to a given wl_surface with a given id. The given id can be used in a
> shell to identify which application is assigned to a wl_surface and
> layout the surface wherever the shell wants. ivi_surface objects can
> be used to receive status of wl_surface in the scenegraph of the
> compositor.
>
> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
> ---
>
> Changes for v2:
> - Rename "error" to "warning" because meaning of "error" in wayland is fatal.
>
> Changes for v3:
> - Move "warning" from ivi_application to ivi_surface.
> - Squash Makefile.
> - Add description to ivi_surface:destroy.
> - Update description of ivi_application:surface_create.
>
> Changes for v4:
> - Remove detail description of server side from ivi_surface::destroy
> - Add clear discripton what client shall do if it encounters warning in ivi_surface
> ::warning.
> - Add decription what happens when client tries to tie a wl_surface to multiple
> ivi_surfaces.
>
> protocol/Makefile.am | 3 +-
> protocol/ivi-application.xml | 99 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 101 insertions(+), 1 deletion(-)
> create mode 100644 protocol/ivi-application.xml
>
> diff --git a/protocol/Makefile.am b/protocol/Makefile.am
> index 5e331a7..9913f16 100644
> --- a/protocol/Makefile.am
> +++ b/protocol/Makefile.am
> @@ -8,7 +8,8 @@ protocol_sources = \
> text-cursor-position.xml \
> wayland-test.xml \
> xdg-shell.xml \
> - scaler.xml
> + scaler.xml \
> + ivi-application.xml
>
> if HAVE_XMLLINT
> .PHONY: validate
> diff --git a/protocol/ivi-application.xml b/protocol/ivi-application.xml
> new file mode 100644
> index 0000000..37ad489
> --- /dev/null
> +++ b/protocol/ivi-application.xml
> @@ -0,0 +1,99 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="ivi_application">
> +
> + <copyright>
> + Copyright (C) 2013 DENSO CORPORATION
> + Copyright (c) 2013 BMW Car IT GmbH
> +
> + Permission is hereby granted, free of charge, to any person obtaining a copy
> + of this software and associated documentation files (the "Software"), to deal
> + in the Software without restriction, including without limitation the rights
> + to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + copies of the Software, and to permit persons to whom the Software is
> + furnished to do so, subject to the following conditions:
> +
> + The above copyright notice and this permission notice shall be included in
> + all copies or substantial portions of the Software.
> +
> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + THE SOFTWARE.
> + </copyright>
> +
> + <interface name="ivi_surface" version="1">
> + <description summary="application interface to surface in ivi compositor"/>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroy ivi_surface">
> + This removes link from surface_id to wl_surface and destroys ivi_surface.
> + </description>
> + </request>
> +
> + <event name="visibility">
> + <description summary="visibility of surface in ivi compositor has changed">
> + The new visibility state is provided in argument visibility.
> + If visibility is 0, the surface has become invisible.
> + If visibility is not 0, the surface has become visible.
> + </description>
> + <arg name="visibility" type="int"/>
> + </event>
> +
> + <enum name="warning_code">
> + <description summary="possible warning codes returned by ivi compositor">
> + These warning codes define all possible warning codes returned by ivi compositor
Codes define codes? :-)
> + on server-side warnings.
> + invalid_wl_surface: invalid wl_surface is set. This happens if wl_surface is destroyed before this.
Ooh, does this mean that invalid_wl_surface is emitted if wl_surface is
destroyed before the ivi_surface? Try to use more nouns and less
pronouns in specification language to keep things explicit.
I was about ask what happens if the wl_surface gets destroyed first.
> + surface_id_in_use: surface_id is already assigned by another application.
> + </description>
> + <entry name="invalid_wl_surface" value="1" summary="wl_surface is invalid"/>
> + <entry name="surface_id_in_use" value="2" summary="surface_id is in use and can not be shared"/>
> + </enum>
> +
> + <event name="warning">
> + <description summary="server-side warning detected">
> + The ivi compositor encountered warning while processing a request by this
> + application. The warning is defined by argument warning_code and optional
> + warning_text. If the warning is detected, client shall destroy the ivi_surface
> + object.
> +
> + When ivi compositor encounters warnings, a request is canceled and there is no
> + mapping from id_surface/wl_surface to this ivi_surface. Even if there is no
> + mapping but destroying this ivi_surface is recommended.
I would propose a slightly different wording here, which may or may not
be what you intended above.
"When a warning event is sent, the compositor turns the ivi_surface
object inert. The ivi_surface will not deliver further events, all
requests on it are ignored except 'destroy', and the association to the
surface_id is removed. The client should destroy the ivi_surface
object. If an inert ivi_surface object is used as an argument to any
other object's request, that request will [produce a fatal error /
produce a warning / be ignored]."
There are no requests other than 'destroy' on ivi_surface, but it is
better to be explicit just in case later someone adds a new request. On
the final part, you can pick what suits best, or say that the effect is
undefined.
> + </description>
> + <arg name="warning_code" type="int"/>
> + <arg name="warning_text" type="string" allow-null="true"/>
> + </event>
> +
> + </interface>
> +
> + <interface name="ivi_application" version="1">
> + <description summary="interface for ivi applications to use ivi compositor features"/>
Could mention that this is a global interface, isn't it?
> + <request name="surface_create">
> + <description summary="create ivi_surface with numeric ID in ivi compositor">
> + surface_create will create a interface:ivi_surface with numeric ID; surface_id in
> + ivi compositor. These surface_ids are defined as unique in the system to identify
> + it inside of ivi compositor. The ivi compositor implements business logic how to
> + set properties of the surface with surface_id according to status of the system.
> + E.g. a unique ID for Car Navigation application is used for implementing special
> + logic of the application about where it shall be located.
> +
> + Created ivi_surface notifies warnings when following situation happens,
> + - Invalid wl_surface is set. This happen if wl_surface is destroy before this.
How is it possible to destroy the wl_surface before issuing a
ivi_application.surface_create request? IOW, I cannot understand when a
wl_surface could be invalid due to "destroy". But it could be if the
wl_surface already has a role.
> + - surface_id is already assigned by another application.
> + If a client sets a wl_surface to multiple id_surfaces, no warning is issued. Property
> + of the surface can be controlled by multiple ivi_surface. However this case shall
> + be avoided by a client.
By id_surface, do you mean creating multiple ivi_surface objects with
the same or different surface_id for a single wl_surface? Would it ever
make any sense to do that?
Is it legal or illegal to do that? If it is illegal, there should be a
fatal protocol error. I know you prefer non-fatal warnings, but even
Wayland core has fatal errors like for invalid object id. I would think
that creating several ivi_surfaces for the same wl_surface would be a
similar problem as use-after-free producing an invalid object id, if it
really is illegal.
Also, if ivi_application.surface_create assigns a role to a wl_surface,
then you cannot have several ivi_surfaces referencing the same
wl_surface.
In Weston, a role is identified by weston_surface::configure function
pointer. More conceptually, a role defines how the surface behaves on
screen: cursor surface moves with the pointer, ivi_surface is
controlled by your... ivi-controller or something. In principle,
without a role, a surface cannot be shown at all because the compositor
does not know how or when to show it.
> + </description>
> + <arg name="id_surface" type="uint"/>
id_surface here, surface_id in the doc; maybe pick one form, and use
that everywhere?
I wonder, could we use a more specific term for the ID? Would "ivi_id"
be unambiguous? So we could talk about surface's ivi_id, as opposed to
surface_id which is not obviously an IVI concept. Every Wayland protocol
object has an id, and I would like avoid any chances of confusing
wl_surface id with ivi_id.
> + <arg name="surface" type="object" interface="wl_surface"/>
> + <arg name="id" type="new_id" interface="ivi_surface"/>
> + </request>
> +
> + </interface>
> +
> +</protocol>
This is looking good, mostly just some details in the wording to be
tuned. :-)
I will see if I can review more of the patches, but I would also
suggest the following, in case you are still interested in pushing this
upstream.
Wait for Weston 1.5 to be released. We are currently in freeze, so
there is no point in re-sending until that is done. Check that the 1.5
stable branch has been created, or at least the release has been made,
before you rebase and re-send this series.
Thanks,
pq
More information about the wayland-devel
mailing list