Bug 60561 - [Qt] Implements a disable appearance for Media Elements of Qt port.
Summary: [Qt] Implements a disable appearance for Media Elements of Qt port.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-10 10:54 PDT by Alexis Menard (darktears)
Modified: 2011-05-16 14:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2011-05-10 10:58 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (2.82 KB, patch)
2011-05-11 13:28 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (3.08 KB, patch)
2011-05-11 15:55 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2011-05-10 10:54:43 PDT
[Qt] Implements a disable appearance for Media Elements of Qt port.
Comment 1 Alexis Menard (darktears) 2011-05-10 10:58:20 PDT
Created attachment 92984 [details]
Patch
Comment 2 Alexis Menard (darktears) 2011-05-10 11:02:26 PDT
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.
Comment 3 Eric Carlson 2011-05-10 11:28:56 PDT
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?
Comment 4 Alexis Menard (darktears) 2011-05-10 11:36:59 PDT
(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 ?
Comment 5 Eric Carlson 2011-05-10 11:39:51 PDT
(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?
Comment 6 Alexis Menard (darktears) 2011-05-10 12:05:02 PDT
(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.
Comment 7 Tor Arne Vestbø 2011-05-11 07:37:54 PDT
(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.
Comment 8 Alexis Menard (darktears) 2011-05-11 13:28:46 PDT
Created attachment 93166 [details]
Patch
Comment 9 Kenneth Rohde Christiansen 2011-05-11 14:11:49 PDT
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
Comment 10 Alexis Menard (darktears) 2011-05-11 15:55:58 PDT
Created attachment 93202 [details]
Patch
Comment 11 Alexis Menard (darktears) 2011-05-11 15:56:29 PDT
(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.
Comment 12 Kenneth Rohde Christiansen 2011-05-11 15:57:22 PDT
Thanks ;-)
Comment 13 WebKit Commit Bot 2011-05-11 18:20:46 PDT
Comment on attachment 93202 [details]
Patch

Clearing flags on attachment: 93202

Committed r86299: <http://trac.webkit.org/changeset/86299>
Comment 14 WebKit Commit Bot 2011-05-11 18:20:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Ademar Reis 2011-05-16 14:03:03 PDT
Revision r86299 cherry-picked into qtwebkit-2.2 with commit ba1970d <http://gitorious.org/webkit/qtwebkit/commit/ba1970d>