RSS defer matching articles for rules until idle. Closes #6166. #6177

magao · 2017-01-01T02:22:11Z

This is one of a series of pull requests designed to improve the responsiveness of the UI. This pull request addresses #6166.

Currently when editing an RSS rule each modification (e.g. each keypress in "Must Contain") results in immediately matching articles to populate the tree. This matching can take a long time - seconds is not uncommon, and can be a lot longer (one of my filters takes ~1 minute) - during which no further input is accepted.

This pull requests delays the article matching until there has been at least 1 second without any changes to the rule. This allows typing, etc without delay and makes for a much more responsive UI.

This pull request also adds a processing indicator while the articles are being matched:

Note: this pull request includes a commit (ee0359c) which has been factored out to allow all the pull requests I am submitting to merge without conflicts.

magao · 2017-01-08T02:54:23Z

Rebased on top of #6212 and uncrustified.

sledgehammer999 · 2017-01-18T18:47:49Z

src/gui/rss/automatedrssdownloader.cpp

+        // Set the loading icon and allow it to be displayed
+        ui->treeMatchingLoading->setPixmap(QIcon(":/icons/loading.png").pixmap(16, 16));
+        m_updateMatchingArticlesDeferrals.ref();
+        // Need to have a 1ms delay here or we don't get the screen update - not sure why


That's because UI updates happen after control has returned to the mainloop.
So instead of the timer+return I think you can safely do:

QCoreApplication::instance()->processEvents(QEventLoop::ExcludeUserInputEvents | QEventLoop::ExcludeSocketNotifiers);

sledgehammer999 · 2017-01-18T18:54:15Z

src/gui/rss/automatedrssdownloader.cpp

@@ -521,12 +522,36 @@ void AutomatedRssDownloader::handleFeedCheckStateChange(QListWidgetItem *feed_it
 void AutomatedRssDownloader::updateMatchingArticles()
 {
    ui->treeMatchingArticles->clear();
+    saveEditedRule();
+    m_updateMatchingArticlesDeferrals.ref();
+    QTimer::singleShot(1000, this, SLOT(deferredUpdateMatchingArticles()));


This will create a multitude of temporary QTimers. If I read it correctly it will create one for each keystroke.
Instead do this. In the class constructor create a member Timer. Set its property to singleshot and its interval to 1000ms.
Then in this function/slot, just call start() on the timer. If it is already running it will be restarted.

Thanks - I'll change it to work that way. Qt newbie ...

--HG-- branch : magao-dev

…6166. --HG-- branch : magao-dev

magao · 2017-01-22T06:45:25Z

@sledgehammer999 Rebased on top of master. Changed to use a single timer for the 1-second delay before matching, and to use processEvents() to show the processing icon.

Also added a few closely-related changes:

Don't select the first rule in the list when opening the dialog (otherwise it starts matching on that immediately).
Sort matched articles ascending.
Don't have duplicates in the tree (e.g. if multiple rules are selected and an article matches more that one).

sledgehammer999 · 2017-01-25T21:41:56Z

src/gui/rss/automatedrssdownloader.cpp

+
+    foreach (const QListWidgetItem *rule_item, ui->listRules->selectedItems()) {
+        Rss::DownloadRulePtr rule = m_editableRuleList->getRule(rule_item->text());
+        if (rule) {


curly brackets aren't needed here

Same - must not have re-run uncrustify.

sledgehammer999 · 2017-01-25T21:44:18Z

src/gui/rss/automatedrssdownloader.cpp

@@ -47,10 +50,35 @@
 #include "ui_automatedrssdownloader.h"
 #include "automatedrssdownloader.h"

+class AutomatedRssDownloader::DownloadRuleListMatchState


I can't see the usefulness of this class. Why not just make the QTimer and QSet privatemembers of the AutomatedRssDownloader class?
I read the next commit too, but I still don't see the benefit.

It's more useful for PR #6178 (200a815) which has to hold a lot more state. By including it in these commits it makes for more straightforward diffs IMO. But I'm happy to move them to AutomatedRssDownloader if you'd prefer.

OK, I'll probably respond tomorrow. Today, I am not in a state where I can read complex PRs.

No problems - I can rework it on the weekend if needed. Working on things today because it's a public holiday here today (Australia Day).

sledgehammer999 · 2017-01-25T21:46:17Z

src/gui/rss/automatedrssdownloader.cpp

 AutomatedRssDownloader::AutomatedRssDownloader(const QWeakPointer<Rss::Manager> &manager, QWidget *parent)
    : QDialog(parent),
    ui(new Ui::AutomatedRssDownloader),
-    m_manager(manager), m_editedRule(0)
+    m_manager(manager), m_editedRule(0),
+    m_ruleMatcher(new AutomatedRssDownloader::DownloadRuleListMatchState())


Something went wrong with uncrustify.
The initialization list should look like this:

AutomatedRssDownloader::AutomatedRssDownloader(const QWeakPointer<Rss::Manager> &manager, QWidget *parent) : QDialog(parent) , ui(new Ui::AutomatedRssDownloader) , m_manager(manager) , m_editedRule(0) , m_ruleMatcher(new AutomatedRssDownloader::DownloadRuleListMatchState()) { }

Must not have re-run uncrustify on this - I'll do so.

Take note that the lines that you didn't chagne in this diff are also wrong in the initialization list.
(comma goes in the start to reduce noise in the diff when adding new fields)

Turns out that uncrustify doesn't fix that initialisation list - you might want to look at the uncrustify.cfg.

I'll fix it here manually.

--HG-- branch : magao-dev

magao · 2017-01-28T22:51:01Z

@sledgehammer999 Rebased on top of master, added an uncrustify commit before all the other commits and ran uncrustify for each commit.

magao · 2017-02-05T05:27:50Z

This PR is on hold until #6326 and #6345 have been reviewed and (preferably) merged (there are merge conflicts due to changing some of the same code).

magao mentioned this pull request Jan 1, 2017

Incremental/interruptible RSS article match tree updates. Closes #6169. #6178

Closed

magao force-pushed the issue6166 branch from 894842e to c3609d8 Compare January 8, 2017 02:46

sledgehammer999 added the RSS RSS-related issues/changes label Jan 18, 2017

sledgehammer999 reviewed Jan 18, 2017

View reviewed changes

sledgehammer999 mentioned this pull request Jan 18, 2017

Improve UI responsiveness during RSS downloading. Closes #873, #1089, #1235, #5423 #6176

Merged

magao added 3 commits January 22, 2017 15:36

RSS sort matched articles ascending in downloader dialog.

0356d38

--HG-- branch : magao-dev

RSS prevent duplicate matched articles in downloader dialog.

fcd10bb

--HG-- branch : magao-dev

RSS defer matching articles for rules until idle. Closes qbittorrent#…

bd643b4

…6166. --HG-- branch : magao-dev

magao force-pushed the issue6166 branch from c3609d8 to 90a62e5 Compare January 22, 2017 06:41

sledgehammer999 requested changes Jan 25, 2017

View reviewed changes

magao added 2 commits January 29, 2017 09:24

Follow project coding style. Issue qbittorrent#2192.

d2818fe

--HG-- branch : magao-dev

Don't select first RSS rule when opening downloader dialog.

9d5e723

--HG-- branch : magao-dev

magao force-pushed the issue6166 branch from 90a62e5 to bd643b4 Compare January 28, 2017 22:49

magao mentioned this pull request Mar 11, 2017

[WIP] Redesign RSS subsystem #6505

Closed

FranciscoPombal added the PR abandoned label Mar 7, 2020

FranciscoPombal closed this Mar 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSS defer matching articles for rules until idle. Closes #6166. #6177

RSS defer matching articles for rules until idle. Closes #6166. #6177

magao commented Jan 1, 2017 •

edited

Loading

magao commented Jan 8, 2017

sledgehammer999 Jan 18, 2017

sledgehammer999 Jan 18, 2017

magao Jan 18, 2017

magao commented Jan 22, 2017 •

edited

Loading

sledgehammer999 Jan 25, 2017

magao Jan 25, 2017

sledgehammer999 Jan 25, 2017

magao Jan 25, 2017

sledgehammer999 Jan 25, 2017

magao Jan 25, 2017

sledgehammer999 Jan 25, 2017

magao Jan 25, 2017

sledgehammer999 Jan 25, 2017

magao Jan 25, 2017

magao commented Jan 28, 2017

magao commented Feb 5, 2017

RSS defer matching articles for rules until idle. Closes #6166. #6177

RSS defer matching articles for rules until idle. Closes #6166. #6177

Conversation

magao commented Jan 1, 2017 • edited Loading

magao commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magao commented Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magao commented Jan 28, 2017

magao commented Feb 5, 2017

magao commented Jan 1, 2017 •

edited

Loading

magao commented Jan 22, 2017 •

edited

Loading