Bug 33967 – [GTK] Mute/unmute button on <video> elements are backwards

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mike Hommey]

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 ?

[Mike Hommey]

Created attachment 47138 [details]
Patch

With the patch

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 47138 [details]
Patch

Every change requires a ChangeLog.  see http://webkit.org/coding/contributing.html

[Mike Hommey]

(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.)

[Eric Carlson]

Or you could change the name to indicate the action they will take: m_willMuteButton, m_willPauseButton, etc.

[Xan Lopez]

(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.

[Eric Carlson]

(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.

[Diego Escalante Urrelo]

Created attachment 54030 [details]
2010-04-21  Diego Escalante Urrelo  <[e-mail]>

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):

[Diego Escalante Urrelo]

Created attachment 54031 [details]
2010-04-21  Diego Escalante Urrelo  <[e-mail]>

review!

2010-04-21  Diego Escalante Urrelo  <[e-mail]>

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):

[WebKit Commit Bot]

Comment on attachment 54031 [details]
2010-04-21  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 54031

Committed r58097: <http://trac.webkit.org/changeset/58097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.