Bug 55463 - [EFL] Add play / pause button to media control
Summary: [EFL] Add play / pause button to media control
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on: 55460
Blocks: 56726 56810
  Show dependency treegraph
 
Reported: 2011-03-01 05:48 PST by Gyuyoung Kim
Modified: 2011-03-23 09:21 PDT (History)
6 users (show)

See Also:


Attachments
Prototype (8.46 KB, patch)
2011-03-01 05:59 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Playbutton (4.69 KB, patch)
2011-03-10 00:13 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Pause Button (3.18 KB, patch)
2011-03-10 00:13 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Proposed Patch (15.54 KB, patch)
2011-03-15 03:31 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (15.44 KB, patch)
2011-03-15 04:39 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Screen Capture - Play Button (4.22 KB, image/png)
2011-03-16 00:51 PDT, Gyuyoung Kim
no flags Details
Screen Capture - Pause Button (4.37 KB, image/png)
2011-03-16 00:52 PDT, Gyuyoung Kim
no flags Details
Proposed Patch (15.12 KB, patch)
2011-03-23 01:31 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Proposed Patch (15.12 KB, patch)
2011-03-23 01:35 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (15.12 KB, patch)
2011-03-23 01:39 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Improved Patch (14.27 KB, patch)
2011-03-23 06:02 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (14.42 KB, patch)
2011-03-23 07:01 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-03-01 05:48:00 PST
Add play and pause button to media control.
Comment 1 Gyuyoung Kim 2011-03-01 05:59:39 PST
Created attachment 84222 [details]
Prototype
Comment 2 Gyuyoung Kim 2011-03-01 06:11:09 PST
Does anyone know how to add image files ?
Comment 3 Gyuyoung Kim 2011-03-10 00:13:31 PST
Created attachment 85288 [details]
Playbutton
Comment 4 Gyuyoung Kim 2011-03-10 00:13:58 PST
Created attachment 85289 [details]
Pause Button
Comment 5 Gyuyoung Kim 2011-03-15 03:31:19 PDT
Created attachment 85792 [details]
Proposed Patch
Comment 6 Gyuyoung Kim 2011-03-15 04:39:32 PDT
Created attachment 85794 [details]
Patch

I remove unnecessary variables.
Comment 7 Gyuyoung Kim 2011-03-16 00:51:19 PDT
Created attachment 85916 [details]
Screen Capture - Play Button

This patch paints Play button as attached image.
Comment 8 Gyuyoung Kim 2011-03-16 00:52:17 PDT
Created attachment 85917 [details]
Screen Capture - Pause Button

This patch paints Stop button as attached image.
Comment 9 Gyuyoung Kim 2011-03-18 00:23:19 PDT
Hello Lucas and Leandro,

How do you think about this patch ?
Comment 10 Lucas De Marchi 2011-03-22 04:40:14 PDT
Comment on attachment 85794 [details]
Patch

Why don't you use a single button with 2 states?
Comment 11 Gyuyoung Kim 2011-03-22 05:24:18 PDT
(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.
Comment 12 Gyuyoung Kim 2011-03-23 01:31:15 PDT
Created attachment 86585 [details]
Proposed Patch

I modify to use a single button with 2 states.
Comment 13 WebKit Review Bot 2011-03-23 01:32:25 PDT
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.
Comment 14 Gyuyoung Kim 2011-03-23 01:35:28 PDT
Created attachment 86586 [details]
Proposed Patch

Oops, fix style error.
Comment 15 WebKit Review Bot 2011-03-23 01:37:16 PDT
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.
Comment 16 Gyuyoung Kim 2011-03-23 01:39:32 PDT
Created attachment 86587 [details]
Patch

Oops, I didn't add modified files.
Comment 17 Lucas De Marchi 2011-03-23 05:04:54 PDT
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
Comment 18 Gyuyoung Kim 2011-03-23 06:02:54 PDT
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.
Comment 19 Gyuyoung Kim 2011-03-23 06:27:35 PDT
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";
+        }
Comment 20 Lucas De Marchi 2011-03-23 06:50:16 PDT
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
Comment 21 Gyuyoung Kim 2011-03-23 07:01:32 PDT
Created attachment 86612 [details]
Modified Patch

Yes, I modify patch.
Comment 22 Lucas De Marchi 2011-03-23 07:40:10 PDT
(In reply to comment #21)
> Created an attachment (id=86612) [details]
> Modified Patch
> 
> Yes, I modify patch.

Now it looks good.
Comment 23 Antonio Gomes 2011-03-23 08:56:50 PDT
Comment on attachment 86612 [details]
Modified Patch

rs=me, since lucas reviewed it!
Comment 24 WebKit Commit Bot 2011-03-23 09:21:47 PDT
Comment on attachment 86612 [details]
Modified Patch

Clearing flags on attachment: 86612

Committed r81777: <http://trac.webkit.org/changeset/81777>
Comment 25 WebKit Commit Bot 2011-03-23 09:21:54 PDT
All reviewed patches have been landed.  Closing bug.