[MPRIS] MPRIS Fullscreen Extensions

[Kevin Anthony]

On Wed, Jan 18, 2012 at 1:20 PM, Mirsal Ennaime <mirsal at videolan.org> wrote:

> 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
>
> _______________________________________________
> MPRIS mailing list
> MPRIS at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mpris
>
>


-- 
Thanks
Kevin Anthony
www.NoSideRacing.com

Do you use Banshee?
Download the Community Extensions:
http://banshee.fm/download/extensions/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mpris/attachments/20120119/3cc56da1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-PATCH-Added-Fullscreen-property.patch
Type: text/x-patch
Size: 2463 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mpris/attachments/20120119/3cc56da1/attachment.bin>