Bug 55463 – [EFL] Add play / pause button to media control

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Add play and pause button to media control.

[Gyuyoung Kim]

Created attachment 84222 [details]
Prototype

[Gyuyoung Kim]

Does anyone know how to add image files ?

[Gyuyoung Kim]

Created attachment 85288 [details]
Playbutton

[Gyuyoung Kim]

Created attachment 85289 [details]
Pause Button

[Gyuyoung Kim]

Created attachment 85792 [details]
Proposed Patch

[Gyuyoung Kim]

Created attachment 85794 [details]
Patch

I remove unnecessary variables.

[Gyuyoung Kim]

Created attachment 85916 [details]
Screen Capture - Play Button

This patch paints Play button as attached image.

[Gyuyoung Kim]

Created attachment 85917 [details]
Screen Capture - Pause Button

This patch paints Stop button as attached image.

[Gyuyoung Kim]

Hello Lucas and Leandro,

How do you think about this patch ?

[Lucas De Marchi]

Comment on attachment 85794 [details]
Patch

Why don't you use a single button with 2 states?

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 85794 [details])
> Why don't you use a single button with 2 states?

Ok, I will try it soon.

[Gyuyoung Kim]

Created attachment 86585 [details]
Proposed Patch

I modify to use a single button with 2 states.

[WebKit Review Bot]

Attachment 86585 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/efl/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 86586 [details]
Proposed Patch

Oops, fix style error.

[WebKit Review Bot]

Attachment 86586 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/efl/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 86587 [details]
Patch

Oops, I didn't add modified files.

[Lucas De Marchi]

Comment on attachment 86587 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1099
> +
> +    MediaControlPlayButtonElement* button = static_cast<MediaControlPlayButtonElement*>(node);
> +    emitMediaButtonSignal(MediaPlayPauseButton, button->displayType() == MediaPlayButton ? PlayButton : PauseButton, rect);

Can't you just forward button->displayType() ?

> Source/WebCore/platform/efl/RenderThemeEfl.h:68
> +#if ENABLE(VIDEO)
> +enum MediaButton {
> +    PlayButton,
> +    PauseButton
> +};

I you do so, this enum is unnecessary.

> Source/WebKit/efl/DefaultTheme/widget/mediacontrol/playpausebutton/playpause_button.edc:41
> +      script {
> +          public isPlayed;
> +          public isPaused;        
> +   
> +          public show() {
> + 
> +              if (get_int(isPaused) == 1) {
> +                  set_state(PART:"playpause_button", "pause", 0.0);
> +              } else if (get_int(isPlayed) == 1) {
> +                  set_state(PART:"playpause_button", "play", 0.0);
> +              } 
> +          }

Considering you can't be in 'play' and 'pause' state at the same time. Why do you use two variables to keep the state? See below comment on 'programs'

> Source/WebKit/efl/DefaultTheme/widget/mediacontrol/playpausebutton/playpause_button.edc:63
> +      parts {
> +         part {
> +            name: "playpause_button";
> +            type: IMAGE;
> +            description {
> +               state: "play" 0.0;
> +               min: 25 25;
> +               image {
> +                  normal: "widget/mediacontrol/playpausebutton/playbutton.png";
> +                  border: 1 1 1 1;
> +               }
> +            }
> +            description {
> +               state: "pause" 0.0;
> +               min: 25 25;
> +               image {
> +                  normal: "widget/mediacontrol/playpausebutton/pausebutton.png";
> +                  border: 1 1 1 1;
> +               }
> +            }

IMO, it looks better like this:
      parts {
         part {
            name: "playpause_button";
            type: IMAGE;
            description { state: "default" 0.0;
               min: 25 25;
            }
            description { state: "play" 0.0;
               inherit: "default" 0.0;
               image.normal: "widget/mediacontrol/playpausebutton/playbutton.png";
            }
            description { state: "pause" 0.0;
               inherit: "default" 0.0;
               image.normal: "widget/mediacontrol/playpausebutton/pausebutton.png";
            }

> Source/WebKit/efl/DefaultTheme/widget/mediacontrol/playpausebutton/playpause_button.edc:74
> +      programs {
> +         program {
> +            name: "play";
> +            signal: "play";
> +            script {
> +               set_int(isPlayed, 1);
> +               set_int(isPaused, 0);
> +               show();

Better yet, you can completely remove the 'show()' implementation and set the state here:
               signal: "play";
               action: STATE_SET "play" 0.0;
               target: "playpause_button";

And analogous for 'pause' signal

[Gyuyoung Kim]

Created attachment 86607 [details]
Improved Patch

Demarchi,  Thank you for your review. I think your suggestion is better than previous patch. So, I modify this patch according to your comment.

[Gyuyoung Kim]

BTW, current patch is working well. But, is it clear to add "pause" program as well as "play" program ?

+        program {
+             signal: "pause";
+             action: STATE_SET "pause" 0.0;
+             target: "playpause_button";
+        }

[Lucas De Marchi]

Comment on attachment 86607 [details]
Improved Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1065
> +    if (mediaElementType == MediaPlayButton)
> +        edje_object_signal_emit(entry->o, "play", "");
> +    else if (mediaElementType == MediaPauseButton)
> +        edje_object_signal_emit(entry->o, "pause", "");
> +
> +    return true;

Return false if it's not a recognized control:

    if (mediaElementType == MediaPlayButton)
        edje_object_signal_emit(entry->o, "play", "");
    else if (mediaElementType == MediaPauseButton)
        edje_object_signal_emit(entry->o, "pause", "");
    else
        return false;

    return true;

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1100
> +    emitMediaButtonSignal(MediaPlayPauseButton, button->displayType() == MediaPlayButton ? MediaPlayButton : MediaPauseButton, rect);
> +
> +    return paintThemePart(object, MediaPlayPauseButton, info, rect);

You only need to pass button->displayType() directly... otherwise any other control will be drawn as a Pause button.

if (!emitMediaButtonSignal(MediaPlayPauseButton, button->displayType(), rect))
    return false;

return paintThemePart(object, MediaPlayPauseButton, info, rect);

> Source/WebKit/efl/DefaultTheme/widget/mediacontrol/playpausebutton/playpause_button.edc:53
> +         program {
> +             signal: "play";
> +             action: STATE_SET "play" 0.0;
> +             target: "playpause_button";
> +         }

You forgot the "pause" program

[Gyuyoung Kim]

Created attachment 86612 [details]
Modified Patch

Yes, I modify patch.

[Lucas De Marchi]

(In reply to comment #21)
> Created an attachment (id=86612) [details]
> Modified Patch
> 
> Yes, I modify patch.

Now it looks good.

[Antonio Gomes]

Comment on attachment 86612 [details]
Modified Patch

rs=me, since lucas reviewed it!

[WebKit Commit Bot]

Comment on attachment 86612 [details]
Modified Patch

Clearing flags on attachment: 86612

Committed r81777: <http://trac.webkit.org/changeset/81777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.