Bug 60561 – [Qt] Implements a disable appearance for Media Elements of Qt port.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Alexis Menard (darktears)]

[Qt] Implements a disable appearance for Media Elements of Qt port.

[Alexis Menard (darktears)]

Created attachment 92984 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 92984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92984&action=review

> Source/WebCore/platform/qt/RenderThemeQt.cpp:1228
> +        fgColor = Qt::gray;

I could have used platformInactiveSelectionForegroundColor or platformInactiveSelectionBackgroundColor but they really look crap on my Linux KDE.

[Eric Carlson]

Comment on attachment 92984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92984&action=review

> Source/WebCore/platform/qt/RenderThemeQt.cpp:1227
> +    if (mediaElement && (!hasSource(mediaElement) || !mediaElement->hasAudio()))

Do you really want paint the thumb as disabled if the element has no audio track?

[Alexis Menard (darktears)]

(In reply to comment #3)
> (From update of attachment 92984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92984&action=review
> 
> > Source/WebCore/platform/qt/RenderThemeQt.cpp:1227
> > +    if (mediaElement && (!hasSource(mediaElement) || !mediaElement->hasAudio()))
> 
> Do you really want paint the thumb as disabled if the element has no audio track?

Should I check instead if maxTimeSeekable > 0 ?

[Eric Carlson]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 92984 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=92984&action=review
> > 
> > > Source/WebCore/platform/qt/RenderThemeQt.cpp:1227
> > > +    if (mediaElement && (!hasSource(mediaElement) || !mediaElement->hasAudio()))
> > 
> > Do you really want paint the thumb as disabled if the element has no audio track?
> 
> Should I check instead if maxTimeSeekable > 0 ?

It depends entirely on what you are trying to accomplish. If you want to make it look like seeking is disabled when a file has video but no audio, the current code is probably fine.

How are you hoping to help the user with this UI change?

[Alexis Menard (darktears)]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 92984 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=92984&action=review
> > > 
> > > > Source/WebCore/platform/qt/RenderThemeQt.cpp:1227
> > > > +    if (mediaElement && (!hasSource(mediaElement) || !mediaElement->hasAudio()))
> > > 
> > > Do you really want paint the thumb as disabled if the element has no audio track?
> > 
> > Should I check instead if maxTimeSeekable > 0 ?
> 
> It depends entirely on what you are trying to accomplish. If you want to make it look like seeking is disabled when a file has video but no audio, the current code is probably fine.
> 
> How are you hoping to help the user with this UI change?

What I'm trying to achieve is that when we don't have yet meta-data, no source basically we're not able to play the video, the controls will look disabled. As soon as data comes in then they look active again. If you have Chromium you'll see.

[Tor Arne Vestbø]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (From update of attachment 92984 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=92984&action=review
> > > > 
> > > > > Source/WebCore/platform/qt/RenderThemeQt.cpp:1227
> > > > > +    if (mediaElement && (!hasSource(mediaElement) || !mediaElement->hasAudio()))
> > > > 
> > > > Do you really want paint the thumb as disabled if the element has no audio track?
> > > 
> > > Should I check instead if maxTimeSeekable > 0 ?
> > 
> > It depends entirely on what you are trying to accomplish. If you want to make it look like seeking is disabled when a file has video but no audio, the current code is probably fine.
> > 
> > How are you hoping to help the user with this UI change?
> 
> What I'm trying to achieve is that when we don't have yet meta-data, no source basically we're not able to play the video, the controls will look disabled. As soon as data comes in then they look active again. If you have Chromium you'll see.


How about we use the readyState:

 - HAVE_NOTHING: If preload is none, draw play-button i normal color (so the user can initiate load), everything else disabled. If preload is not none (metadata or auto), draw everything disabled, until....
 - HAVE_METADATA: start drawing play-button using normal colors regardless, same goes for other controls, unless you want to delay them to HAVE_CURRENT_DATA?

In all cases, if there are errors, we should reflect those by disabling (graying out) all controls.

[Alexis Menard (darktears)]

Created attachment 93166 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 93166 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93166&action=review

> Source/WebCore/platform/qt/RenderThemeQt.cpp:1232
> +    if (!mediaElementCanPlay(o))
> +        fgColor = Qt::gray;

Is disabled always grey with all themes?

> Source/WebCore/platform/qt/RenderThemeQt.cpp:1455
> +    p.painter->setBrush(getMediaControlForegroundColor(o->parent()));

Maybe make it more clear who is the parent here

[Alexis Menard (darktears)]

Created attachment 93202 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #10)
> Created an attachment (id=93202) [details]
> Patch

I use the palette now. So whatever color is used in Disabled will be picked.

[Kenneth Rohde Christiansen]

Thanks ;-)

[WebKit Commit Bot]

Comment on attachment 93202 [details]
Patch

Clearing flags on attachment: 93202

Committed r86299: <http://trac.webkit.org/changeset/86299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r86299 cherry-picked into qtwebkit-2.2 with commit ba1970d <http://gitorious.org/webkit/qtwebkit/commit/ba1970d>