fix(editor): make tabbar and bottombar follow system font changes#446
Conversation
78ba578 to
5fde7c1
Compare
Reviewer's GuideQt6-specific fix to ensure Tabbar, BottomBar, and DDropdownMenu respond correctly to application font changes by adjusting eventFilter behavior and explicitly updating widget fonts, while keeping Qt5 behavior unchanged. Sequence diagram for ApplicationFontChange propagation with updated eventFilter behaviorsequenceDiagram
actor User
participant SystemSettings
participant QtApp as qApp
participant DDropdownMenu
participant Tabbar
participant BottomBar
User->>SystemSettings: Change system font
SystemSettings->>QtApp: Emit ApplicationFontChange
QtApp->>DDropdownMenu: eventFilter(ApplicationFontChange)
activate DDropdownMenu
DDropdownMenu->>DDropdownMenu: OnFontChangedSlot(qApp.font())
DDropdownMenu->>DDropdownMenu: m_pToolButton.setFont(m_font)
DDropdownMenu->>DDropdownMenu: m_pToolButton.setIcon(createIcon())
DDropdownMenu-->>QtApp: return false (do not block chain)
deactivate DDropdownMenu
QtApp->>Tabbar: eventFilter(ApplicationFontChange)
activate Tabbar
Tabbar->>Tabbar: setFont(qApp.font())
Tabbar-->>QtApp: return false
deactivate Tabbar
QtApp->>BottomBar: eventFilter(ApplicationFontChange)
activate BottomBar
BottomBar->>BottomBar: m_pPositionLabel.setFont(qApp.font())
BottomBar->>BottomBar: m_pCharCountLabel.setFont(qApp.font())
BottomBar->>BottomBar: m_pCursorStatus.setFont(qApp.font())
BottomBar->>BottomBar: m_scaleLabel.setFont(qApp.font())
BottomBar->>BottomBar: m_progressLabel.setFont(qApp.font())
BottomBar-->>QtApp: return false
deactivate BottomBar
Updated class diagram for Tabbar, BottomBar, and DDropdownMenu font change handlingclassDiagram
class QWidget
class BottomBar {
+BottomBar(QWidget *parent)
+DDropdownMenu *getHighlightMenu()
+void paintEvent(QPaintEvent *event)
+bool eventFilter(QObject *object, QEvent *event)
-void initFormatMenu()
-QLabel *m_pPositionLabel
-QLabel *m_pCharCountLabel
-QLabel *m_pCursorStatus
-QLabel *m_scaleLabel
-QLabel *m_progressLabel
-DDropdownMenu *m_pHighlightMenu
}
class Tabbar {
+Tabbar(QWidget *parent)
+bool eventFilter(QObject *object, QEvent *event)
+void handleDragActionChanged(Qt::DropAction action)
}
class DDropdownMenu {
+bool eventFilter(QObject *object, QEvent *event)
+void OnFontChangedSlot(const QFont &font)
-QFont m_font
-QToolButton *m_pToolButton
}
BottomBar --|> QWidget
Tabbar --|> QWidget
BottomBar --> DDropdownMenu
DDropdownMenu --> QWidget
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
5fde7c1 to
135dda9
Compare
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In BottomBar::eventFilter you ignore the watched object and call QWidget::eventFilter(nullptr, event); it would be safer to keep the QObject * parameter (e.g., as
watched) and forward it to the base implementation instead of passing nullptr. - Tabbar::eventFilter is now being used as a global application event filter, so the unconditional
qDebug() << "Enter eventFilter";will generate a lot of noisy logging; consider removing or guarding it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In BottomBar::eventFilter you ignore the watched object and call QWidget::eventFilter(nullptr, event); it would be safer to keep the QObject * parameter (e.g., as `watched`) and forward it to the base implementation instead of passing nullptr.
- Tabbar::eventFilter is now being used as a global application event filter, so the unconditional `qDebug() << "Enter eventFilter";` will generate a lot of noisy logging; consider removing or guarding it.
## Individual Comments
### Comment 1
<location path="src/widgets/bottombar.cpp" line_range="288-297" />
<code_context>
return m_pHighlightMenu;
}
+bool BottomBar::eventFilter(QObject *, QEvent *event)
+{
+#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
+ if (event->type() == QEvent::ApplicationFontChange) {
+ QFont font = qApp->font();
+ m_pPositionLabel->setFont(font);
+ m_pCharCountLabel->setFont(font);
+ m_pCursorStatus->setFont(font);
+ m_scaleLabel->setFont(font);
+ m_progressLabel->setFont(font);
+ return false;
+ }
+#endif
+ return QWidget::eventFilter(nullptr, event);
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Forward the watched object to the base implementation instead of passing nullptr, and consider using QObject::eventFilter as the base.
Passing nullptr to `QWidget::eventFilter` drops the `watched` object, which can break base-class assumptions and cause subtle bugs. Please change the signature to `bool BottomBar::eventFilter(QObject *watched, QEvent *event)` and call `return QWidget::eventFilter(watched, event);` (or `QObject::eventFilter(watched, event)`), so the event chain keeps the correct context.
</issue_to_address>
### Comment 2
<location path="src/controls/tabbar.cpp" line_range="62-63" />
<code_context>
installEventFilter(this);
+#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
+ qApp->installEventFilter(this);
+#endif
+
</code_context>
<issue_to_address>
**suggestion (performance):** Installing an application-wide event filter on Tabbar may cause excessive event traffic and noisy logging.
With `qApp->installEventFilter(this);`, `Tabbar` now sees every application event in addition to its own (you already call `installEventFilter(this);`). Combined with `qDebug() << "Enter eventFilter";`, this will produce very noisy logs and extra overhead. Please either narrow the event filter to specific objects or remove/guard this generic debug log for Qt 6 builds.
Suggested implementation:
```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)) && defined(QT_DEBUG)
// In Qt 6, only install an application-wide event filter in debug builds
// to avoid excessive event traffic and noisy logging in production.
qApp->installEventFilter(this);
#endif
```
To fully implement the review comment, you should also:
1. Locate the `eventFilter` implementation in `src/controls/tabbar.cpp` (or its corresponding header/source).
2. Wrap the generic logging line `qDebug() << "Enter eventFilter";` with a debug-only guard, e.g.:
```cpp
#if defined(QT_DEBUG)
qDebug() << "Enter eventFilter";
#endif
```
or remove it entirely if it is no longer useful.
This will ensure that even in debug builds, the extra logging is controlled, and in release builds there is no unnecessary overhead or log noise.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| bool BottomBar::eventFilter(QObject *, QEvent *event) | ||
| { | ||
| #if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)) | ||
| if (event->type() == QEvent::ApplicationFontChange) { | ||
| QFont font = qApp->font(); | ||
| m_pPositionLabel->setFont(font); | ||
| m_pCharCountLabel->setFont(font); | ||
| m_pCursorStatus->setFont(font); | ||
| m_scaleLabel->setFont(font); | ||
| m_progressLabel->setFont(font); |
There was a problem hiding this comment.
issue (bug_risk): Forward the watched object to the base implementation instead of passing nullptr, and consider using QObject::eventFilter as the base.
Passing nullptr to QWidget::eventFilter drops the watched object, which can break base-class assumptions and cause subtle bugs. Please change the signature to bool BottomBar::eventFilter(QObject *watched, QEvent *event) and call return QWidget::eventFilter(watched, event); (or QObject::eventFilter(watched, event)), so the event chain keeps the correct context.
| #if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)) | ||
| qApp->installEventFilter(this); |
There was a problem hiding this comment.
suggestion (performance): Installing an application-wide event filter on Tabbar may cause excessive event traffic and noisy logging.
With qApp->installEventFilter(this);, Tabbar now sees every application event in addition to its own (you already call installEventFilter(this);). Combined with qDebug() << "Enter eventFilter";, this will produce very noisy logs and extra overhead. Please either narrow the event filter to specific objects or remove/guard this generic debug log for Qt 6 builds.
Suggested implementation:
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)) && defined(QT_DEBUG)
// In Qt 6, only install an application-wide event filter in debug builds
// to avoid excessive event traffic and noisy logging in production.
qApp->installEventFilter(this);
#endif
To fully implement the review comment, you should also:
- Locate the
eventFilterimplementation insrc/controls/tabbar.cpp(or its corresponding header/source). - Wrap the generic logging line
qDebug() << "Enter eventFilter";with a debug-only guard, e.g.:or remove it entirely if it is no longer useful.#if defined(QT_DEBUG) qDebug() << "Enter eventFilter"; #endif
This will ensure that even in debug builds, the extra logging is controlled, and in release builds there is no unnecessary overhead or log noise.
b63feba to
4956ada
Compare
In Qt6, DDropdownMenu's eventFilter returned true for ApplicationFontChange, blocking the event filter chain and preventing Tabbar/BottomBar from receiving the event. Fix by returning false instead, and adding ApplicationFontChange handling in Tabbar and BottomBar via qApp eventFilter. Qt6下DDropdownMenu的eventFilter对ApplicationFontChange事件 返回true阻断了事件过滤器链,导致Tabbar和BottomBar无法接收 字体变化事件。修复为返回false,并在Tabbar和BottomBar中添加 ApplicationFontChange事件处理。 Log: 修复标签栏和底部工具栏不跟随系统字体变化的问题 PMS: BUG-358185 Influence: 标签栏文字、底部工具栏文字及下拉菜单文字现在能正确跟随系统字体变化,Qt5行为不受影响。
4956ada to
5771462
Compare
deepin pr auto review经过对提供的 Git diff 代码的审查,我发现这段代码主要目的是为了适配 Qt6,特别是针对字体变化事件( 1. 语法逻辑审查优点:
问题与改进建议:
2. 代码质量审查优点:
改进建议:
3. 代码性能审查
4. 代码安全审查
总结与修改建议代码核心修改建议:
以下是针对 1. src/controls/tabbar.h 和 tabbar.cpp在头文件中添加析构函数声明(如果没有的话): ~Tabbar() override;在 cpp 中实现析构函数: Tabbar::~Tabbar()
{
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
if (qApp) {
qApp->removeEventFilter(this);
}
#endif
}2. src/widgets/bottombar.h 和 bottombar.cpp在头文件中添加析构函数声明(如果没有的话): ~BottomBar() override;在 cpp 中实现析构函数: BottomBar::~BottomBar()
{
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
if (qApp) {
qApp->removeEventFilter(this);
}
#endif
}优化 private:
QList<QLabel*> m_fontAwareLabels; // 需要跟随系统字体变化的Label列表在 BottomBar::BottomBar(QWidget *parent)
: QWidget(parent)
, // ... 其他初始化
{
// ... 原有代码 ...
// 将需要更新字体的 Label 加入列表
m_fontAwareLabels << m_pPositionLabel
<< m_pCharCountLabel
<< m_pCursorStatus
<< m_scaleLabel
<< m_progressLabel;
// ...
}修改 bool BottomBar::eventFilter(QObject *watched, QEvent *event)
{
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
if (event->type() == QEvent::ApplicationFontChange) {
QFont font = qApp->font();
// 使用循环减少重复代码
for (auto label : m_fontAwareLabels) {
if (label) {
label->setFont(font);
}
}
return false;
}
#endif
return QWidget::eventFilter(watched, event);
} |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, pengfeixx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/merge |
In Qt6, DDropdownMenu's eventFilter returned true for ApplicationFontChange, blocking the event filter chain and preventing Tabbar/BottomBar from receiving the event.
Fix by returning false instead, and adding ApplicationFontChange handling in Tabbar and BottomBar via qApp eventFilter.
Qt6下DDropdownMenu的eventFilter对ApplicationFontChange事件 返回true阻断了事件过滤器链,导致Tabbar和BottomBar无法接收
字体变化事件。修复为返回false,并在Tabbar和BottomBar中添加
ApplicationFontChange事件处理。
Log: 修复标签栏和底部工具栏不跟随系统字体变化的问题
PMS: BUG-358185
Influence: 标签栏文字、底部工具栏文字及下拉菜单文字现在能正确跟随系统字体变化,Qt5行为不受影响。
Summary by Sourcery
Ensure Tabbar, BottomBar, and dropdown menus respond correctly to system font change events, particularly under Qt6.
Bug Fixes:
Enhancements: