Bug 33967 - [GTK] Mute/unmute button on <video> elements are backwards
Summary: [GTK] Mute/unmute button on <video> elements are backwards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-21 12:37 PST by Mike Hommey
Modified: 2010-04-22 08:36 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.14 KB, patch)
2010-01-21 12:39 PST, Mike Hommey
eric: review-
Details | Formatted Diff | Diff
2010-04-21 Diego Escalante Urrelo <descalante@igalia.com> (3.00 KB, patch)
2010-04-21 23:33 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
2010-04-21 Diego Escalante Urrelo <descalante@igalia.com> (3.02 KB, patch)
2010-04-21 23:36 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Hommey 2010-01-21 12:37:39 PST
The button was added as part of #26304.

When the sound is muted, the icon displays the "audio-volume-high" icon, and on the contrary, when the sound is unmuted, the "audio-volume-muted" icon is displayed. While when you read the latter this way it can sound normal, it actually makes no sense, because it doesn't reflect the current state of the sound.

The patch I'm attaching will fix the obvious problem, but I think the variable names should be changed to something that makes it more obvious, like "m_mutedButton" and "m_unmutedButton" (which would reverse their meaning).

What do you think ?
Comment 1 Mike Hommey 2010-01-21 12:39:06 PST
Created attachment 47138 [details]
Patch

With the patch
Comment 2 Eric Seidel (no email) 2010-01-21 16:46:04 PST
Comment on attachment 47138 [details]
Patch

I don't like these variable names.  m_pauseButton will cause pause to happen, or shows the fact that it's currently paused?  The way this is written, it seems mute is being used the opposite.
Comment 3 Eric Seidel (no email) 2010-01-21 16:46:47 PST
Comment on attachment 47138 [details]
Patch

Every change requires a ChangeLog.  see http://webkit.org/coding/contributing.html
Comment 4 Mike Hommey 2010-01-22 00:10:52 PST
(In reply to comment #3)
> (From update of attachment 47138 [details])
> Every change requires a ChangeLog.  see
> http://webkit.org/coding/contributing.html

I know that ;)

I first wanted to know how this should be fixed. We seem to agree on the fact that the variable names are ambiguous. 
How about renaming them to a past participle form as proposed in comment #0 ? (m_mutedButton, m_pausedButton, etc.)
Comment 5 Eric Carlson 2010-02-03 20:16:47 PST
Or you could change the name to indicate the action they will take: m_willMuteButton, m_willPauseButton, etc.
Comment 6 Xan Lopez 2010-02-12 02:30:44 PST
(In reply to comment #5)
> Or you could change the name to indicate the action they will take:
> m_willMuteButton, m_willPauseButton, etc.

I think considering how they are used:

return paintMediaButton(paintInfo.context, r, mediaElement->muted() ? m_unmuteButton.get() : m_muteButton.get(), m_panelColor, m_mediaIconSize);

the names of the buttons are already OK (and consistent with the other buttons). It's just that what they do when clicked is the opposite to the status they should reflect, since they reverse themselves. So if we do what comment #0 suggests at the end, or similar, we'd make one thing clear but the other unclear (ie, the state would match the name but not the action).

So IMHO it's fine to just commit what the patch does, perhaps adding a small comment explaining it.
Comment 7 Eric Carlson 2010-02-12 08:04:07 PST
(In reply to comment #6)
> I think considering how they are used:
> 
> return paintMediaButton(paintInfo.context, r, mediaElement->muted() ?
> m_unmuteButton.get() : m_muteButton.get(), m_panelColor, m_mediaIconSize);
> 
> the names of the buttons are already OK (and consistent with the other
> buttons). It's just that what they do when clicked is the opposite to the
> status they should reflect, since they reverse themselves. So if we do what
> comment #0 suggests at the end, or similar, we'd make one thing clear but the
> other unclear (ie, the state would match the name but not the action).
> 
> So IMHO it's fine to just commit what the patch does, perhaps adding a small
> comment explaining it.

I am also fine with adding comments to the code and leaving the names as-is.
Comment 8 Diego Escalante Urrelo 2010-04-21 23:33:05 PDT
Created attachment 54030 [details]
2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

Reviewed by NOBODY (OOPS!).

        [GTK] Mute/unmute button on <video> elements are backwards
        https://bugs.webkit.org/show_bug.cgi?id=33967

        Fix mute/unmute buttons icons-action relation and explain why their
        variable names and corresponding icons seem to be misleading but are
        actually right. Original patch by Mike Hommey.

        * platform/gtk/RenderThemeGtk.cpp:
        (WebCore::RenderThemeGtk::initMediaStyling):
Comment 9 Diego Escalante Urrelo 2010-04-21 23:36:12 PDT
Created attachment 54031 [details]
2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

review!

2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

Reviewed by NOBODY (OOPS!).

        [GTK] Mute/unmute button on <video> elements are backwards
        https://bugs.webkit.org/show_bug.cgi?id=33967

        Fix mute/unmute buttons icons-action relation and explain why their
        variable names and corresponding icons seem to be misleading but are
        actually right. Original patch by Mike Hommey.

        * platform/gtk/RenderThemeGtk.cpp:
        (WebCore::RenderThemeGtk::initMediaStyling):
Comment 10 WebKit Commit Bot 2010-04-22 08:36:51 PDT
Comment on attachment 54031 [details]
2010-04-21  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 54031

Committed r58097: <http://trac.webkit.org/changeset/58097>
Comment 11 WebKit Commit Bot 2010-04-22 08:36:57 PDT
All reviewed patches have been landed.  Closing bug.