Use tree for feeds in RSS downloader dialog. Closes #6281. #6345

magao · 2017-02-05T05:25:18Z

This PR builds on PR #6326. It changes the RSS downloader dialog to use a tree for its feed list, as discussed in issue #6281.

This PR also introduces a new form of Natural Sort - the "case semi-sensitive" sort. This is a case insensitive sort, except where 2 strings compare equal ignoring case, in which case the strings are compared using a case-sensitive sort. This ensures a consistent ordering.

This PR also introduces subclasses of QListWidgetItem and QTreeWidgetItem that sort via natural sort (defaults to case semi-sensitive, but can be specified as case-sensitive or case-insensitive).

The RSS tab and downloader dialogs use these classes to sort, with a further enhancement that folders sort first (and for the RSS tab, the Unread item sorts before all others).

If there are no visible folders then the expander decorations are hidden, resulting in effectively the same UI as current.

--HG-- branch : magao-dev

…orrent#6281. --HG-- branch : magao-dev

--HG-- branch : magao-dev

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

--HG-- branch : magao-dev

magao · 2017-02-05T06:38:58Z

This could potentially also close issue #1805 - as I noted there, with this PR you could organise your feeds into folders (maybe a single top-level folder) so you can select multiple feeds at once.

Also #3679 as if this PR is merged then I think #3679 becomes redundant.

Chocobo1 · 2017-02-05T17:11:45Z

This PR also introduces a new form of Natural Sort - the "case semi-sensitive" sort.

That's what I've tried to achieved when I was cleaning that up a long time ago. And I didn't opt to do so before is because I thought 2-pass for doing sorting is sorta a waste of computing resource, now I doubt my views.
Also I tried to impl the same sorting after reading your idea :) https://github.com/Chocobo1/qBittorrent/commits/sort

Can you also enlighten me what is the advantage of SortableListWidgetItem (and the other one)?
AFAIK, the sorting is done in the Qt Model part so why a SortableTreeWidgetItem::operator< will benefit is not so clear for me, I mean you could just simply plug in Utils::String::naturalCompareCaseSensitive where it's needed...

Chocobo1 · 2017-02-05T17:14:13Z

src/base/utils/string.cpp

            int posL = 0;
            int posR = 0;
            while (true) {
                while (true) {
                    if ((posL == left.size()) || (posR == right.size()))
-                        return (left.size() < right.size());  // when a shorter string is another string's prefix, shorter string place before longer string
+                        return (left.size() < right.size()) ? -1 : 1;  // when a shorter string is another string's prefix, shorter string place before longer string


I think you should just use subtraction here and below.

I thought about that ... probably better. Although in theory that does potentially suffer underflow errors if right.size() is large enough. Would need to first cast both to long, then the result back to int. Not sure if that would be more or less efficient than a branch.

You lost me here, how could the same type with only >=0 values, subtraction causes an integer underflow? care to provide an example here?

Not sure if that would be more or less efficient than a branch.

In human perspective on computer, should be faster; in reality, depends on how good the compiler optimization is...

D'oh! For some reason it totally slipped my mind that they're both always positive values ... so no chance of underflow.

Without casting subtraction should definitely be more efficient.

Chocobo1 · 2017-02-05T17:26:12Z

src/base/utils/string.cpp

-    thread_local NaturalCompare nCmp(true);
-    return nCmp(left, right);
-#endif
+    return NaturalCompare::naturalCompareCaseSensitive(left, right) < 0;


You don't really have to create this function in order to let Utils::String::naturalCompareCaseSemiSensitive looks nice... or you could just propagate the return int up as I've done here: Chocobo1@20964ce
Just some ideas though...

It wasn't to make the functions look nice, but to avoid:

Duplicating the code again;

Having extra thread-locals when we only need the 2.

magao · 2017-02-05T20:25:39Z

The cases where it would need to fall back to a case-sensitive sort are likely to be rare - I don't see the extra sort in those cases to be a performance killer. There may be cases where we genuinely want a case-insensitive sort e.g. a case-insensitive container, so having the case-insensitive sort automatically fall back to case-sensitive as a tie-breaker may not be correct in all cases (hence why I created a separate function).

There are some widgets where you can't change the model (setModel() is private) - including QTreeWidget and QListWidget. The recommended way to sort items of these widgets is to override operator<() on the *WidgetItem (I've seen some code which directly calls like ui.treeWidget->QTreeWidget::setModel(proxyModel); but that's always followed by a "don't do that" ...

Since I was going to use the behaviour in multiple places, I thought it best to create utility classes.

Chocobo1 · 2017-02-06T01:36:36Z

so having the case-insensitive sort automatically fall back to case-sensitive as a tie-breaker may not be correct in all cases

I'm curious, for example?

but that's always followed by a "don't do that" ...

This one too, a link to somewhere that explains this?

magao · 2017-02-06T01:56:13Z

so having the case-insensitive sort automatically fall back to case-sensitive as a tie-breaker may not be correct in all cases

I'm curious, for example?

I gave one - a case-insensitive container. Another example would be anything on a case-insensitive filesystem. In both of these cases, multiple entries with differing cases should compare equal.

I guess it comes down to if I have a function that claims to do case-insensitive comparisons, then that's exactly what I expect it to do. The definition of "case-sensitive" might need explaining (in particular, whether it's locale-aware or not, follows unicode case definitions, etc).

OTOH, if the functions were named in such a way as to make it clear that they're specifically for sorting (and commented that case-insensitive uses a tie-breaker), I wouldn't have a problem with it.

I should note that currently all uses of naturalCompareCaseInsensitive are sorting, so changing the behaviour shouldn't have any negative consequences to the existing codebase.

Re: sorting items in a QListWidget or QTreeWidget:

https://stackoverflow.com/questions/8493671/sorting-a-qlistwidget-using-sortitems
https://stackoverflow.com/questions/11335602/sort-a-qtreewidgetitem-by-a-data-value#11764852
http://www.qtcentre.org/threads/2904-how-to-use-QSortFilterProxyModel-with-QtreeWidget
https://forum.qt.io/topic/61239/how-does-qtreewidgetitem-sortchildren-sort
http://www.qtforum.org/article/25281/customize-sorting-of-qtreewidget.html

Chocobo1 · 2017-02-06T09:55:20Z

OTOH, if the functions were named in such a way as to make it clear that they're specifically for sorting (and commented that case-insensitive uses a tie-breaker),

I see your point, it's because the functions is called *Compare instead of *LessThan or *Sort, my head is twisting now, maybe you could come up with a good name...

Re: sorting items in a QListWidget or QTreeWidget:

OK, good point, just one more thing, to keep things tidy, you should create a new folder src/gui/widgets/ (for reusable components) and place sortablewidgetitems.* in it, there was some discussions about this before you come along.

Chocobo1 · 2017-02-09T08:28:20Z

src/gui/rss/automatedrssdownloader.h

-    void handleFeedCheckStateChange(QListWidgetItem *feed_item);
-    void updateRuleDefinitionBox();
+    void handleFeedCheckStateChange(QTreeWidgetItem *feed_item, int column);
+    void updateRuleDefinitionBox(QListWidgetItem *selected = 0);


use nullptr instead of 0 (and everywhere else).

Chocobo1 · 2017-02-09T08:33:31Z

src/gui/rss/rss_imp.cpp

@@ -647,20 +649,26 @@ void RSSImp::updateItemInfos(QTreeWidgetItem *item)
 void RSSImp::updateFeedIcon(const QString &url, const QString &iconPath)
 {
    QTreeWidgetItem *item = m_feedList->getTreeItemFromUrl(url);
-    item->setData(0, Qt::DecorationRole, QVariant(QIcon(iconPath)));
+
+    if (item != 0)


for consistency: if (!item) return;

Chocobo1 · 2017-02-09T08:33:43Z

src/gui/rss/rss_imp.cpp

-    if (item->parent())
-        updateItemInfos(item->parent());
+
+    if (item != 0) {


for consistency: if (!item) return;

Chocobo1 · 2017-02-09T08:36:44Z

src/gui/rss/rss_imp.cpp

+
+    if (item != 0) {
+        Rss::FeedPtr stream = qSharedPointerCast<Rss::Feed>(m_feedList->getRSSItem(item));
+        item->setText(0, display_name + QString::fromUtf8("  (") + QString::number(nbUnread) + QString(")"));


can you do us a favor:

(0, display_name + " (" + QString::number(nbUnread) + ")");

Chocobo1 · 2017-02-09T08:39:15Z

src/gui/sortablewidgetitems.cpp

@@ -0,0 +1,129 @@
+/*
+ * Bittorrent Client using Qt4 and libtorrent.


remove the 4

Chocobo1 · 2017-02-09T08:39:58Z

src/gui/sortablewidgetitems.cpp

@@ -0,0 +1,129 @@
+/*
+ * Bittorrent Client using Qt4 and libtorrent.
+ * Copyright (C) 2017  Christophe Dumez


I guess he doesn't wrote this part, put your name here or "The qBittorrent team".

Chocobo1 · 2017-02-09T08:40:11Z

src/gui/sortablewidgetitems.h

@@ -0,0 +1,77 @@
+/*
+ * Bittorrent Client using Qt4 and libtorrent.
+ * Copyright (C) 2017  Christophe Dumez


same the above 2 lines.

Chocobo1 · 2017-02-09T08:40:40Z

src/gui/sortablewidgetitems.h

+    virtual bool operator<(const QTreeWidgetItem &other) const;
+
+private:
+


redundant empty line

Chocobo1 · 2017-02-09T08:41:38Z

src/gui/sortablewidgetitems.cpp

+ */
+
+#include "base/utils/string.h"
+#include "sortablewidgetitems.h"


#include "sortablewidgetitems.h" // empty line #include "base/utils/string.h"

Chocobo1 · 2017-02-09T08:45:58Z

src/base/utils/string.cpp

+            return 0;
+        }
+
+        static int naturalCompareCaseSensitive(const QString &left, const QString &right)


I'm not sure if there is this rule: for each local function, you should create a function prototype

Chocobo1 · 2017-02-09T08:50:11Z

src/gui/rss/rss_imp.cpp

@@ -255,6 +262,10 @@ void RSSImp::deleteSelectedItems()
        }
    }
    m_rssManager->saveStreamList();
+
+    foreach (const QString &feed_id, deleted)
+        m_rssManager->forwardFeedInfosChanged(feed_id, "", 0);


"" -> QString() or {}

Chocobo1 · 2017-02-09T08:54:09Z