Throttle RSS unread signal handling in MainWindow #16377
Conversation
src/gui/mainwindow.cpp
Outdated
| { | ||
| m_rssUnreadUpdateEnqueued = false; | ||
|
|
||
| if (!m_rssWidget) |
There was a problem hiding this comment.
Why not use m_rssWidget as a context to avoid this check?
There was a problem hiding this comment.
m_rssWidget is a QPointer, so I thought it's not owned by this.
There was a problem hiding this comment.
I mean QTimer::singleShot(100, m_rssWidget, [this]() so connection will be destroyed when m_rssWidget is destroyed and you don't need check it inside lambda body.
There was a problem hiding this comment.
what if this is destroyed but m_rssWidget not
m_rssWidget is a QPointer, so I thought it's not owned by this.
checking a QPointer before it's use is recommended.
I didn't look into who owns m_rssWidget
There was a problem hiding this comment.
MainWindow owns (should own) all child widgets "by design". QPointers are just misused in legacy code. However, since we've stumbled upon this problem and you're bumping into it, you can wait for me or someone else to fix it.
There was a problem hiding this comment.
you can wait for me or someone else to fix it.
extra code can be removed later on when someone will fix this problem.
There was a problem hiding this comment.
extra code can be removed later on when someone will fix this problem.
Extra code can be removed now since fixing the problem doesn't really affect it. As I said above MainWindow owns child widgets so it is destroyed after them.
jagannatharjun
force-pushed
the
throtle-rssunread
branch
from
d593d7b
to
f6b5804
Compare
| @@ -261,6 +261,8 @@ private slots: | |||
| SettingValue<bool> m_storeNotificationTorrentAdded; | |||
| CachedSettingValue<Log::MsgTypes> m_storeExecutionLogTypes; | |||
|
|
|||
| bool m_rssUnreadUpdateEnqueued = false; | |||
There was a problem hiding this comment.
Please place it out of "Setting values" section.
| @@ -681,9 +681,19 @@ void MainWindow::on_actionLock_triggered() | |||
| hide(); | |||
| } | |||
|
|
|||
| void MainWindow::handleRSSUnreadCountUpdated(int count) | |||
| void MainWindow::handleRSSUnreadCountUpdated(int) | |||
There was a problem hiding this comment.
Please remove the parameter entirely.
|
If it really doesn't make sense to process all consecutive "unread count changed" events, then why not apply throttling at a lower level? Emitting signals (having connected slots) is also not cheap in itself. |
It can be useful for other parts to get info in realtime. This PR only affects tab texts update of MainWindow. Updating Tab texts are weirdly expensive. I'm planning to open similar PRs for other tabs in the main window. |
Ok. |
| @@ -681,9 +681,19 @@ void MainWindow::on_actionLock_triggered() | |||
| hide(); | |||
| } | |||
|
|
|||
| void MainWindow::handleRSSUnreadCountUpdated(int count) | |||
| void MainWindow::handleRSSUnreadCountUpdated(int) | |||
There was a problem hiding this comment.
| void MainWindow::handleRSSUnreadCountUpdated(int) | |
| void MainWindow::handleRSSUnreadCountUpdated() |
|
@jagannatharjun are you still around? |
Improves startup time