feat: add real-time FPS display#550
Merged
zccrs merged 1 commit intolinuxdeepin:masterfrom Sep 26, 2025
Merged
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Dami-star 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 |
Reviewer's GuideThis PR implements a new FPS overlay subsystem that samples frame timing via v-sync and timers, automatically adapts to display refresh rates, and integrates a toggleable QML overlay using a Meta+F11 shortcut. Sequence diagram for toggling the FPS overlay with Meta+F11sequenceDiagram
actor User
participant Helper
participant FpsDisplayManager
participant QmlEngine
participant FpsDisplay (QML)
User->>Helper: Press Meta+F11
Helper->>Helper: toggleFpsDisplay()
alt FPS overlay is visible
Helper->>FpsDisplayManager: hide()
FpsDisplayManager-->>Helper: visibilityChanged()
Helper->>FpsDisplay: setProperty("fpsVisible", false)
Helper->>FpsDisplay: deleteLater()
else FPS overlay is hidden
Helper->>QmlEngine: createFpsDisplay(parent)
QmlEngine-->>Helper: FpsDisplay (QML)
Helper->>FpsDisplayManager: show()
FpsDisplayManager-->>Helper: visibilityChanged()
Helper->>FpsDisplay: setProperty("fpsVisible", true)
FpsDisplayManager-->>Helper: fpsDataChanged(currentFpsText, maximumFpsText)
Helper->>FpsDisplay: setProperty("currentFpsText", ...)
Helper->>FpsDisplay: setProperty("maximumFpsText", ...)
end
Class diagram for FpsDisplayManager and Helper integrationclassDiagram
class Helper {
+void toggleFpsDisplay()
-QQuickItem *m_fpsDisplay
-FpsDisplayManager *m_fpsDisplayManager
}
class FpsDisplayManager {
+void setTargetWindow(QQuickWindow *window)
+void show()
+void hide()
+void toggle()
+void reset()
+bool isVisible()
+int currentFps()
+int averageFps()
+int maximumFps()
+QString currentFpsText()
+QString maximumFpsText()
+signal fpsDataChanged(QString, QString)
+signal visibilityChanged()
}
Helper --> FpsDisplayManager : manages
FpsDisplayManager --> QQuickWindow : uses
FpsDisplayManager --> QTimer : uses
FpsDisplayManager --> QElapsedTimer : uses
FpsDisplayManager --> WOutput : queries
FpsDisplayManager --> QQuickItem : for overlay
Class diagram for QmlEngine FPS display integrationclassDiagram
class QmlEngine {
+QQuickItem *createFpsDisplay(QQuickItem *parent)
-QQmlComponent fpsDisplayComponent
}
QmlEngine --> QQuickItem : creates FPS overlay
QmlEngine --> QQmlComponent : uses fpsDisplayComponent
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/utils/fpsdisplaymanager.cpp:131` </location>
<code_context>
+
+void FpsDisplayManager::updateFps()
+{
+ if (!m_visible || m_vSyncTimes.isEmpty())
+ return;
+
</code_context>
<issue_to_address>
updateFps does not update if m_vSyncTimes is empty.
When no vsync events have occurred, the display may show outdated FPS values. Consider clearing or resetting the display when m_vSyncTimes is empty.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
void FpsDisplayManager::updateFps()
{
if (!m_visible || m_vSyncTimes.isEmpty())
return;
=======
void FpsDisplayManager::updateFps()
{
if (!m_visible)
return;
if (m_vSyncTimes.isEmpty()) {
if (m_displayItem) {
// Clear or reset the FPS display when no vsync events have occurred
m_displayItem->setProperty("text", QStringLiteral("N/A"));
}
return;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/utils/fpsdisplaymanager.cpp:186` </location>
<code_context>
+{
+ m_displayRefreshRate = 60;
+ if (m_targetWindow) {
+ if (auto screen = m_targetWindow->screen()) {
+ qreal refreshRate = screen->refreshRate();
+ if (refreshRate > 0) {
</code_context>
<issue_to_address>
No handling for screens with refreshRate() returning zero.
Recommend adding a warning log when the refresh rate is zero to aid in diagnosing display issues.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
qreal refreshRate = screen->refreshRate();
if (refreshRate > 0) {
m_displayRefreshRate = qRound(refreshRate);
qCDebug(treelandFpsDisplay) << "Detected display refresh rate:" << m_displayRefreshRate << "Hz";
return;
}
=======
qreal refreshRate = screen->refreshRate();
if (refreshRate > 0) {
m_displayRefreshRate = qRound(refreshRate);
qCDebug(treelandFpsDisplay) << "Detected display refresh rate:" << m_displayRefreshRate << "Hz";
return;
} else {
qCWarning(treelandFpsDisplay) << "Screen refresh rate is zero or negative; using default value (" << m_displayRefreshRate << "Hz).";
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/utils/fpsdisplaymanager.cpp:207` </location>
<code_context>
+ }
+}
+
+class FpsDisplayItem : public QQuickPaintedItem
+{
+ Q_OBJECT
</code_context>
<issue_to_address>
Consider moving the FpsDisplayItem class to its own files and removing manual event handling, relying on Qt's built-in input disabling instead.
```suggestion
Split out the UI class and drop the huge `event()` + per‐event overrides – you can rely on Qt’s built-in “disabled” behavior and input flags instead of manually ignoring every QEvent. For example:
// In FpsDisplayItem.h
#pragma once
#include <QQuickPaintedItem>
class FpsDisplayItem : public QQuickPaintedItem {
public:
explicit FpsDisplayItem(QQuickItem *parent = nullptr)
: QQuickPaintedItem(parent)
{
// disable all input
setEnabled(false);
setAcceptedMouseButtons(Qt::NoButton);
setAcceptHoverEvents(false);
setAcceptTouchEvents(false);
setFlag(ItemAcceptsInputMethod, false);
setFlag(ItemAcceptsDrops, false);
setFlag(ItemIsFocusScope, false);
setFlag(ItemHasContents, true);
// initial geometry
setVisible(false);
setWidth(130);
setHeight(45);
}
void paint(QPainter *p) override {
// ... your existing rendering code ...
}
// no Q_OBJECT, no event()/mouse/key/focus overrides
};
---> move the above into its own FpsDisplayItem.{h,cpp}, remove
the embedded class in fpsdisplaymanager.cpp and delete
all event()/mouse*/key*/focus*/hover*/touch*/moc-related lines.
This cuts the boilerplate and makes fpsdisplaymanager.cpp much sharper
and focused purely on FPS logic.
</issue_to_address>
### Comment 4
<location> `src/utils/fpsdisplaymanager.h:17` </location>
<code_context>
+class QQuickWindow;
+class QScreen;
+
+class FpsDisplayManager : public QObject
+{
+ Q_OBJECT
</code_context>
<issue_to_address>
Consider removing unused slots and moving implementation details from the header to the source file to simplify the public interface.
```cpp
// 1) Remove the unused private slot
--- a/FpsDisplayManager.h
@@ private:
- void clampFpsToRefreshRate();
// 2) Move the two constexprs out of the public header into the .cpp
--- a/FpsDisplayManager.h
@@ private:
- static constexpr int MAX_SAMPLES = 120; // 2 seconds of samples at 60Hz
- static constexpr int UPDATE_INTERVAL_MS = 500; // UI update frequency in milliseconds
--- a/FpsDisplayManager.cpp
@@ top of file
+namespace {
+ constexpr int kMaxSamples = 120; // 2 seconds at 60Hz
+ constexpr int kUpdateIntervalMs = 500; // UI update frequency (ms)
+}
// in your code replace MAX_SAMPLES → kMaxSamples, UPDATE_INTERVAL_MS → kUpdateIntervalMs
// 3) (Optional) Group the two QColor members into a small Style struct
--- a/FpsDisplayManager.h
@@ private:
- QColor m_textColor = QColor(0, 0, 0);
- QColor m_shadowColor = QColor(255, 255, 255, 150);
+ struct Style {
+ QColor text = QColor(0, 0, 0);
+ QColor shadow = QColor(255, 255, 255, 150);
+ } m_style;
--- a/FpsDisplayManager.h
@@ public:
- void setTextColor(const QColor &color) { m_textColor = color; }
- void setShadowColor(const QColor &color) { m_shadowColor = color; }
+ void setTextColor(const QColor &c) { m_style.text = c; }
+ void setShadowColor(const QColor &c) { m_style.shadow = c; }
```
These changes remove the dead slot, shrink your public header, and push implementation details into the .cpp without altering any functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Dami-star
force-pushed
the
FPS
branch
2 times, most recently
from
976c9de to
254da9b
Compare
wineee
reviewed
Sep 10, 2025
wineee
previously approved these changes
Sep 10, 2025
Dami-star
force-pushed
the
FPS
branch
2 times, most recently
from
577f401 to
91d222a
Compare
zccrs
requested changes
Sep 11, 2025
|
TAG Bot New tag: 0.7.1 |
Dami-star
force-pushed
the
FPS
branch
2 times, most recently
from
b7e4227 to
7779098
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- All user-facing FPS strings are hardcoded in English; wrap them in tr()/qsTr() so the overlay can be localized.
- The QML item creation and signal wiring in Helper adds a lot of UI logic to that class—consider moving it into a dedicated controller to keep Helper focused on input/session handling.
- Rather than manually new/deleteLater the QQuickItem, set its parent or use a QScopedPointer/QPointer to manage the lifecycle more robustly and avoid potential leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- All user-facing FPS strings are hardcoded in English; wrap them in tr()/qsTr() so the overlay can be localized.
- The QML item creation and signal wiring in Helper adds a lot of UI logic to that class—consider moving it into a dedicated controller to keep Helper focused on input/session handling.
- Rather than manually new/deleteLater the QQuickItem, set its parent or use a QScopedPointer/QPointer to manage the lifecycle more robustly and avoid potential leaks.
## Individual Comments
### Comment 1
<location> `src/utils/fpsdisplaymanager.cpp:32-41` </location>
<code_context>
+ m_updateTimer.stop();
+ m_vsyncTimer.stop();
+
+ if (m_targetWindow) {
+ disconnect(m_targetWindow, nullptr, this, nullptr);
+ }
+}
+
+void FpsDisplayManager::setTargetWindow(QQuickWindow *window)
+{
+ if (m_targetWindow == window)
+ return;
+
+ if (m_targetWindow) {
+ disconnect(m_targetWindow, nullptr, this, nullptr);
+ }
+
+ invalidateCache();
+ m_targetWindow = window;
+
+ if (m_targetWindow) {
+ updateRefreshAndInterval();
+ connect(m_targetWindow, &QQuickWindow::screenChanged,
+ this, &FpsDisplayManager::onScreenChanged, Qt::UniqueConnection);
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling QQuickWindow destruction to avoid dangling pointers.
Since m_targetWindow is a QPointer, it will become nullptr if QQuickWindow is destroyed, but signals may remain connected. Connect to the destroyed() signal or implement a cleanup mechanism to ensure proper disconnection and prevent use-after-free issues.
Suggested implementation:
```cpp
if (m_targetWindow) {
disconnect(m_targetWindow, nullptr, this, nullptr);
// Disconnect from destroyed signal if previously connected
disconnect(m_targetWindow, &QObject::destroyed, this, &FpsDisplayManager::onTargetWindowDestroyed);
}
}
```
```cpp
void FpsDisplayManager::setTargetWindow(QQuickWindow *window)
{
if (m_targetWindow == window)
return;
if (m_targetWindow) {
disconnect(m_targetWindow, nullptr, this, nullptr);
disconnect(m_targetWindow, &QObject::destroyed, this, &FpsDisplayManager::onTargetWindowDestroyed);
}
invalidateCache();
m_targetWindow = window;
if (m_targetWindow) {
updateRefreshAndInterval();
connect(m_targetWindow, &QQuickWindow::screenChanged,
this, &FpsDisplayManager::onScreenChanged, Qt::UniqueConnection);
connect(m_targetWindow, &QObject::destroyed,
this, &FpsDisplayManager::onTargetWindowDestroyed, Qt::UniqueConnection);
}
```
```cpp
void FpsDisplayManager::onTargetWindowDestroyed()
{
if (m_targetWindow) {
disconnect(m_targetWindow, nullptr, this, nullptr);
disconnect(m_targetWindow, &QObject::destroyed, this, &FpsDisplayManager::onTargetWindowDestroyed);
}
m_targetWindow = nullptr;
invalidateCache();
}
```
You must declare the slot `void onTargetWindowDestroyed();` in the `FpsDisplayManager` class definition (likely in the header file, e.g., `fpsdisplaymanager.h`) and ensure it is marked as a slot if you use the Q_OBJECT macro.
</issue_to_address>
### Comment 2
<location> `src/utils/fpsdisplaymanager.cpp:205-209` </location>
<code_context>
+ }
+
+ // Environment variable override for debugging/testing
+ QString envRefreshRate = qgetenv("TREELAND_REFRESH_RATE");
+ if (!envRefreshRate.isEmpty()) {
+ bool ok;
+ int rate = envRefreshRate.toInt(&ok);
+ if (ok && rate > 0 && rate <= 240) {
+ m_displayRefreshRate = rate;
+ }
</code_context>
<issue_to_address>
**suggestion:** Hardcoded upper limit for refresh rate may restrict future hardware.
Consider allowing the maximum refresh rate to be configurable or increasing the limit to accommodate newer displays.
Suggested implementation:
```cpp
// Environment variable override for debugging/testing
// Allow maximum refresh rate to be configurable via environment variable
int maxRefreshRate = 240; // Default upper limit
QString envMaxRefreshRate = qgetenv("TREELAND_MAX_REFRESH_RATE");
if (!envMaxRefreshRate.isEmpty()) {
bool ok;
int rate = envMaxRefreshRate.toInt(&ok);
if (ok && rate > 0) {
maxRefreshRate = rate;
}
}
```
```cpp
QString envRefreshRate = qgetenv("TREELAND_REFRESH_RATE");
if (!envRefreshRate.isEmpty()) {
bool ok;
int rate = envRefreshRate.toInt(&ok);
if (ok && rate > 0 && rate <= maxRefreshRate) {
m_displayRefreshRate = rate;
}
```
</issue_to_address>
### Comment 3
<location> `src/seat/helper.cpp:2259-2254` </location>
<code_context>
+ m_fpsDisplay = nullptr;
+ }
+ } else {
+ if (!m_fpsDisplay) {
+ m_fpsDisplay = qmlEngine()->createFpsDisplay(m_renderWindow->contentItem());
+ if (m_fpsDisplay) {
+ connect(m_fpsDisplayManager, &FpsDisplayManager::fpsDataChanged,
+ this, [this](const QString ¤tFpsText, const QString &maximumFpsText) {
</code_context>
<issue_to_address>
**suggestion:** Consider handling errors from qmlEngine()->createFpsDisplay more robustly.
Log an error or warning if createFpsDisplay returns nullptr to improve visibility of QML loading failures.
</issue_to_address>
### Comment 4
<location> `src/core/qml/FpsDisplay.qml:16-25` </location>
<code_context>
+ property bool fpsVisible: false
+ property real scaleFactor: 1.0
+
+ Rectangle {
+ id: fpsDisplay
+ width: Math.max(200 * scaleFactor, 180)
+ height: Math.max(80 * scaleFactor, 70)
+ color: "transparent"
+ radius: 8 * scaleFactor
+ border.color: "transparent"
+ border.width: 0
+
+ anchors {
+ top: parent.top
+ right: parent.right
+ margins: 20 * scaleFactor
+ }
+
+ visible: fpsVisible
+ opacity: fpsVisible ? 1.0 : 0.0
+ scale: fpsVisible ? 1.0 : 0.8
+
</code_context>
<issue_to_address>
**suggestion:** Consider accessibility and contrast for text colors.
Since the background is transparent, text may be hard to read on varying backgrounds. Adding a semi-transparent background or shadow can improve accessibility.
Suggested implementation:
```
color: "#AA222222" // semi-transparent dark background for better contrast
```
```
radius: 8 * scaleFactor
border.color: "transparent"
border.width: 0
// Add shadow for accessibility
layer.enabled: true
layer.effect: DropShadow {
color: "#80000000"
radius: 8 * scaleFactor
samples: 16
horizontalOffset: 0
verticalOffset: 2 * scaleFactor
}
anchors {
top: parent.top
right: parent.right
margins: 20 * scaleFactor
}
visible: fpsVisible
opacity: fpsVisible ? 1.0 : 0.0
scale: fpsVisible ? 1.0 : 0.8
```
</issue_to_address>
### Comment 5
<location> `src/utils/fpsdisplaymanager.cpp:13` </location>
<code_context>
+
+FpsDisplayManager::FpsDisplayManager(QObject *parent)
+ : QObject(parent)
+ , m_updateTimer(this)
+ , m_vsyncTimer(this)
+{
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing manual vsync and refresh logic with a frame counter using QQuickWindow::frameSwapped for simpler and more maintainable FPS calculation.
Here’s a way to collapse all of the manual vsync‐jitter, queue, refresh detection, etc., into one simple “count frames between timer ticks” flow using QQuickWindow::frameSwapped:
1) Remove m_vsyncTimer, onVSyncTimer(), m_vSyncTimes, detectDisplayRefreshRate(), findBestOutput(), getOutputForWindow(), invalidateCache(), etc.
2) In setTargetWindow() connect to frameSwapped instead of screen/window timers:
```cpp
void FpsDisplayManager::setTargetWindow(QQuickWindow *window) {
if (m_targetWindow == window) return;
if (m_targetWindow) {
disconnect(m_targetWindow, nullptr, this, nullptr);
}
m_targetWindow = window;
if (m_targetWindow) {
connect(m_targetWindow,
&QQuickWindow::frameSwapped,
this,
&FpsDisplayManager::onFrameSwapped,
Qt::UniqueConnection);
}
}
```
3) Keep just one QTimer (m_updateTimer) and a simple frame counter:
```cpp
// in header:
qint64 m_lastUpdate = 0;
int m_frameCount = 0;
// slot:
void FpsDisplayManager::onFrameSwapped() {
if (!m_visible) return;
++m_frameCount;
}
// called by m_updateTimer every kUpdateIntervalMs
void FpsDisplayManager::updateFps() {
qint64 now = m_timer.elapsed();
qint64 dt = now - m_lastUpdate;
if (dt < kUpdateIntervalMs) return;
// raw fps
double rawFps = (m_frameCount * 1000.0) / dt;
m_frameCount = 0;
m_lastUpdate = now;
// optional smoothing
const double α = 0.15;
if (m_currentFps == 0) {
m_currentFps = rawFps;
} else {
m_currentFps = m_currentFps*(1-α) + rawFps*α;
}
emit fpsDataChanged(
QStringLiteral("Current FPS: %1").arg(qRound(m_currentFps)),
QStringLiteral("Maximum FPS: %1").arg(qRound(m_currentFps)) );
}
```
This:
- Drops manual vsync‐jitter and queue logic
- Drops custom refresh‐rate detection (you’ll still get whatever the GPU/OS is actually delivering)
- Keeps smoothing entirely in one place
- Preserves the existing “update every N ms” behavior
- All functionality and public API remain unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Sorry @Dami-star, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
zccrs
requested changes
Sep 19, 2025
|
TAG Bot New tag: 0.7.2 |
Dami-star
force-pushed
the
FPS
branch
3 times, most recently
from
8a998c5 to
5dbefc3
Compare
wineee
requested changes
Sep 19, 2025
zccrs
requested changes
Sep 19, 2025
zccrs
reviewed
Sep 22, 2025
Collaborator
Author
|
@sourcery-ai resolve |
Collaborator
Author
已经在src/utils/fpsdisplaymanager.h 添加TODO，为多屏每个屏幕计算FPS。 |
zccrs
previously approved these changes
Sep 22, 2025
|
TAG Bot New tag: 0.7.3 |
Introduces an on-screen FPS overlay with timer-based sampling aligned to the active screen’s refresh rate. automatic refresh-rate handling on screen changes.
zccrs
approved these changes
Sep 26, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces an on-screen FPS overlay with timer-based sampling aligned to the active screen’s refresh rate. automatic refresh-rate handling on screen changes.
Summary by Sourcery
Add a real-time, on-screen FPS display feature with a manager that samples vsync timings, a QML overlay for rendering FPS metrics, and a keyboard shortcut to toggle visibility while handling dynamic refresh rates
New Features:
Build: