[MPRIS] MPRIS Fullscreen Extensions

[Mirsal Ennaime]

Hello Kevin, and thank you for your patch,
Comments inline.

On Wed, 2012-01-18 at 12:47 -0500, Kevin Anthony wrote:
> Here's the current patch, but i'm not sure what i should do when you said
> "We also can't really say that the media player should implement these
> additions, since we want to be backwards compatible"

We can't require these new properties to be present without breaking
backwards compatibility.


> diff --git a/spec/Root_Node.xml b/spec/Root_Node.xml
> index 191acaa..f96719e 100644
> --- a/spec/Root_Node.xml
> +++ b/spec/Root_Node.xml
> @@ -49,6 +49,41 @@
>        </tp:docstring>
>      </property>
>  
> +    <property name="Fullscreen" type="b"
> tp:name-for-bindings="Fullscreen" access="readwrite">
> +      <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
> +        <p>Causes the media player toggle into or out of fullscreen
> mode.</p>
> +        <p>
> +          <strong>true</strong> toggles from normal mode to
> fullscreen mode
> +        </p>
> +        <p>
> +          <strong>false</strong> toggles from fullscreen mode to
> normal mode
> +        </p>

The term "toggle" might be a bit confusing, "switch" would probably be a
better fit.

> +        <p>
> +          The media player should ignore calls when
> <strong>true</strong> and
> +          already in fullscreen mode or <strong>false</strong> when
> not in fullscreen
> +          mode
> +        </p>

Using less ambiguous wording would be nice here, like: "The media player
should ignore calls to org.freedesktop.DBus.Properties.Set for this
property if the value given as an argument is the same as the current
one" or something along these lines.

> +        <p>
> +          The media player may refuse to allow clients to enter or
> exit fullscreen.

I would rather use "The media player may not allow..." but that's
probably subjective.

> +          In this case, the
> <tp:member-ref>CanSetFullscreen</tp:member-ref> property is
> +          <strong>false</strong> and this method does nothing.

This is not a method, so "changing this property" instead of "this
method" would be better IMHO, and we should state that CanSetFullscreen
might not even exist.

> +        </p>
> +        <p>If the media player does niot have a UI, this should be
> implemented</p>

there's a typo (s/niot/not), and we can't require these properties to be
implemented.

> +      </tp:docstring>
> +    </property>
> +
> +    <property name="CanSetFullscreen" type="b"
> tp:name-for-bindings="Can_Set_Fullscreen" access="read">
> +      <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
> +        <p>
> +          If <strong>false</strong>, calling
> +          <tp:member-ref>Fullscreen</tp:member-ref> will have no
> effect, and may

Same as earlier, "changing the Fullscreen property" instead of "calling
Fullscren"

> +          raise a NotSupported error.

I don't think we should spec this: "and may raise an error" should be
enough.

Again, thank you very much for your time !

All the best,

-- 
mirsal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/mpris/attachments/20120118/b70824b0/attachment.pgp>