From 3e659b10f042bfa6fd969c4bcd1e84be6ce58c7c Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Sat, 30 Mar 2024 18:36:26 -0400 Subject: [PATCH] Ladybird/Qt: Reference the correct tab when handling the audio icon We were errantly always referring to the active tab when the audio play state changed, and when clicking a tab's audio state button, by way of BrowserWindow::view(). It turns out we also can't copy / rely on the tab index provided to the signal in any asynchronous context. If the tabs are rearranged, so are their indices. Instead, capture a pointer to the tab of interest - this should be safe as we wouldn't be able to click a tab's audio button if that tab no longer exists. With this change, we can click the audio button from any tab in the Qt chrome, and re-arrange tabs at will. The AppKit and Serenity chromes do not have this issue. --- Ladybird/Qt/BrowserWindow.cpp | 27 +++++++++++++++------------ Ladybird/Qt/BrowserWindow.h | 4 ++-- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/Ladybird/Qt/BrowserWindow.cpp b/Ladybird/Qt/BrowserWindow.cpp index 4dcf372f954..4b617c0542f 100644 --- a/Ladybird/Qt/BrowserWindow.cpp +++ b/Ladybird/Qt/BrowserWindow.cpp @@ -649,29 +649,32 @@ void BrowserWindow::tab_favicon_changed(int index, QIcon const& icon) void BrowserWindow::tab_audio_play_state_changed(int index, Web::HTML::AudioPlayState play_state) { + auto* tab = verify_cast(m_tabs_container->widget(index)); + switch (play_state) { case Web::HTML::AudioPlayState::Paused: - if (view().page_mute_state() == Web::HTML::MuteState::Unmuted) + if (tab->view().page_mute_state() == Web::HTML::MuteState::Unmuted) m_tabs_container->tabBar()->setTabButton(index, QTabBar::LeftSide, nullptr); break; case Web::HTML::AudioPlayState::Playing: - auto* button = new QPushButton(icon_for_page_mute_state(), {}); - button->setToolTip(tool_tip_for_page_mute_state()); + auto* button = new QPushButton(icon_for_page_mute_state(*tab), {}); + button->setToolTip(tool_tip_for_page_mute_state(*tab)); button->setFlat(true); button->resize({ 20, 20 }); - connect(button, &QPushButton::clicked, this, [this, index]() { - view().toggle_page_mute_state(); + connect(button, &QPushButton::clicked, this, [this, tab]() { + tab->view().toggle_page_mute_state(); + auto index = tab_index(tab); - switch (view().audio_play_state()) { + switch (tab->view().audio_play_state()) { case Web::HTML::AudioPlayState::Paused: m_tabs_container->tabBar()->setTabButton(index, QTabBar::LeftSide, nullptr); break; case Web::HTML::AudioPlayState::Playing: auto* button = m_tabs_container->tabBar()->tabButton(index, QTabBar::LeftSide); - verify_cast(button)->setIcon(icon_for_page_mute_state()); - button->setToolTip(tool_tip_for_page_mute_state()); + verify_cast(button)->setIcon(icon_for_page_mute_state(*tab)); + button->setToolTip(tool_tip_for_page_mute_state(*tab)); break; } }); @@ -681,9 +684,9 @@ void BrowserWindow::tab_audio_play_state_changed(int index, Web::HTML::AudioPlay } } -QIcon BrowserWindow::icon_for_page_mute_state() const +QIcon BrowserWindow::icon_for_page_mute_state(Tab& tab) const { - switch (view().page_mute_state()) { + switch (tab.view().page_mute_state()) { case Web::HTML::MuteState::Muted: return style()->standardIcon(QStyle::SP_MediaVolumeMuted); case Web::HTML::MuteState::Unmuted: @@ -693,9 +696,9 @@ QIcon BrowserWindow::icon_for_page_mute_state() const VERIFY_NOT_REACHED(); } -QString BrowserWindow::tool_tip_for_page_mute_state() const +QString BrowserWindow::tool_tip_for_page_mute_state(Tab& tab) const { - switch (view().page_mute_state()) { + switch (tab.view().page_mute_state()) { case Web::HTML::MuteState::Muted: return "Unmute tab"; case Web::HTML::MuteState::Unmuted: diff --git a/Ladybird/Qt/BrowserWindow.h b/Ladybird/Qt/BrowserWindow.h index ad67f098f2f..4d364a4788c 100644 --- a/Ladybird/Qt/BrowserWindow.h +++ b/Ladybird/Qt/BrowserWindow.h @@ -128,8 +128,8 @@ private: } } - QIcon icon_for_page_mute_state() const; - QString tool_tip_for_page_mute_state() const; + QIcon icon_for_page_mute_state(Tab&) const; + QString tool_tip_for_page_mute_state(Tab&) const; QScreen* m_current_screen; double m_device_pixel_ratio { 0 };