Implement search filters. Partially closes #972 #3989

zeule · 2015-10-25T16:48:01Z

As per discussion in issue #3812, here we propose a filtering panel for the search tab. The panel's widgets control state of the search results proxy model to filter out unneeded items. The filtering currently includes seeds number, torrent size and a flag to perform matching the search pattern against torrent names only, or include file names matches as returned by some of the search engines.

This replaces filtering in the BTDigg and TorrentReactor search plugins.

Thanks to all the participants of the discussion in issue #3812 and especially to @ngosang.

Screenshot:

Pireo · 2015-10-25T20:06:18Z

Demonoid can eventually make use of this function, since it does search in description too.

DoumanAsh · 2015-10-26T05:23:44Z

Can you please also add screenshot of how it looks with current code at the head's message?

DoumanAsh · 2015-10-26T06:26:05Z

I made small review for you. This is mostly style though

glassez · 2015-10-26T09:35:09Z

src/core/utils/misc.cpp

    int i = 0;
-    while(val >= 1024. && i < 4) {
-        val /= 1024.;
+    while(rawVal >= 1024. && i < 4) {


Placing each condition in the parentheses improves the readability of the code, IMO.

while ((rawVal >= 1024.) && (i < 4))

glassez · 2015-10-26T10:19:16Z

@evsh NOTE: This PR may interfere and conflicts with #3832.

zeule · 2015-10-26T12:14:21Z

Thanks to all of you for the edits and suggestions. It seems to me that rebasing commits from PR #3832 on top of this one is easier than vice-versa.

zeule · 2015-10-26T12:29:50Z

Can you please also add screenshot of how it looks with current code at the head's message?

Yes, please. (Screenshot moved into the top post)

glassez · 2015-10-26T12:46:07Z

src/core/utils/misc.h

@@ -47,6 +47,22 @@ namespace Utils
 {
    namespace Misc
    {
+        // use Binary prefix standards from IEC 60027-2
+        // see http://en.wikipedia.org/wiki/Kilobyte
+        enum class SizeUnit


By the way, since you began to use enum class, you can even put it outside Utils::Misc namespace (Utils child namespaces are intended for the functions - that was the idea).
It will be fine. Just SizeUnit::Byte etc.

glassez · 2015-10-26T13:43:39Z

It seems to me that rebasing commits from PR #3832 on top of this one is easier than vice-versa.

I don't think so. Usually, when there is simultaneous change and move, have to manually add some things.
Okay, let maintainer decides which one of us will have to suffer.

zeule · 2015-10-26T14:19:08Z

Usually, when there is simultaneous change and move, have to manually add some things.

Yes, I meant that resolving a conflict before the files got renamed and split should be easier.

glassez · 2015-10-26T16:04:47Z

src/core/utils/misc.cpp

 // use Binary prefix standards from IEC 60027-2
 // see http://en.wikipedia.org/wiki/Kilobyte
 // value must be given in bytes
 // to send numbers instead of strings with suffixes
-QString Utils::Misc::friendlyUnit(qreal val, bool is_speed)
+bool Utils::Misc::friendlyUnit(qreal rawVal, qreal &val, Utils::Misc::SizeUnit &unit)


qreal is wrong input type for this function. We expect there some amount of bytes that shouldn't be neither fractional nor negative. So qlonglong will be right here. And return void, of course.

Did you mean quint64?

I guess there are negative sizes somewhere, and they are used to represent unknown size, because someone even created a translatable string for this case. For the same reason the function has to signal about incorrect size, so it returns bool.
The very first action of this function will be the conversion of integral input into floating point value, but since everybody want cleaner interface, let's change both function signatures to qint64.

But does double has any meaning for this function at all?
It is just being used as qint64 and so it is not really has any meaning since you're passing qint64 in all cases?

I guess qreal was used for parameter to avoid declaring a local variable

Did you mean quint64?

Yes. I made a typo. I mean qulonglong, which is the same.

But does double has any meaning for this function at all?

We can't have a fractional number of bytes, but we can have fractional number of kibibytes, etc. So return type of qreal is correct.

I guess there are negative sizes somewhere, and they are used to represent unknown size

This logic should be placed where it is required. Not here!

DoumanAsh · 2015-10-26T16:29:47Z

I looked over picture and in my opinion it looks good without extending vertically space for filter bar.
Compact and nice!
I will leave review to someone more capable :)

UPD:
But maybe you should change a bit flag Names only it looks strange a bit comparing to other fields i.e. you should follow the same style for all fields i think

zeule · 2015-10-26T18:01:49Z

But maybe you should change a bit flag Names only it looks strange a bit comparing to other fields i.e. you should follow the same style for all fields i think

@DoumanAsh can you suggest something, please? Because I can't understand what style do you mean.

DoumanAsh · 2015-10-26T22:01:01Z

@evsh Sorry.
I'll try to illustrate it with my awful Paint skills:
http://i.imgur.com/aVsRGDo.png

Green is name and red is value.
Names only flag looks off comparing to Seeds/Size filters

At least for me.

zeule · 2015-10-26T22:25:51Z

@DoumanAsh thank toy for the illustration. Do you mean that the label in "Names only" is at the right side from the value widget, while all other labels are at the left side? If so, I do not know what can I do about this. QCheckBox looks like it looks, I do not want to change it. Neither I want to move all other label to the right side. What to you want to change?

glassez · 2015-10-27T04:21:12Z

@DoumanAsh is right here. You need to put the label before the checkbox (you never seen one?).

sledgehammer999 · 2016-04-03T20:06:39Z

src/gui/search/searchsortmodel.cpp

+    }
+
+    if (m_minLeechs > 0 || m_maxLeechs >= 0) {
+        int leechs = sourceModel()->data(sourceModel()->index(sourceRow, LEECHS)).toInt();


The correct plural of the word leech is "leeches". Please correct all variations of it.

and of course add the source parent.
Btw, wouldn't it be beneficial to hold the sourceModel() in a tmp variable at the top of the function? (for optimization and readability purposes).

The correct plural of the word leech is "leeches". Please correct all variations of it.

I probably wanted to follow the enumerator identifier. Let's correct that too?

Yes. From a quick look these need correction:

LEECHS

m_minLeechs

m_maxLeechs

leechs

setLeechsFilter (and doxygen comments)

search is your friend :p

zeule · 2016-04-03T20:46:49Z

@sledgehammer999, thank you for the suggestions!

sledgehammer999 · 2016-04-03T20:53:01Z

I am done with the review. I have left some comments that are easy to fix.

@ everyone
I know there has been a shitload of discussion from my last comment in this thread. I haven't read anything. If something needs my attention for decisions(or opinion) please comment below here. Otherwise, once @evsh fixes my comments I am going to merge this.

sledgehammer999 · 2016-04-03T20:56:56Z

I forgot to mention that I didn't compile/run this. It will be my final step before merging. I hope I don't find any UI weirdness.

zeule · 2016-04-03T22:15:49Z

src/gui/search/searchsortmodel.cpp

+    m_searchTerm = searchTerm;
+    if (searchTerm.length() > 2
+        && searchTerm.at(0) == QLatin1Char('"') && searchTerm.at(searchTerm.length() - 1) == QLatin1Char('"')) {
+        m_searchTermWords.append(m_searchTerm.mid(1, m_searchTerm.length() - 2));


What a nice bug! It exists, but does not expose itself. Wish they all learn from this one.

Do you mean you should assign and not append? I admit I didn't catch too :S

Yes. Obviously, the list was flooded with the copy of this word :)

On 4 April 2016 at 00:25, sledgehammer999 notifications@github.com wrote:

In src/gui/search/searchsortmodel.cpp
#3989 (comment)
:

{
}

+void SearchSortModel::enableNameFilter(bool enable)
+{

m_isNameFilterEnabled = enable;
+}

+void SearchSortModel::setNameFilter(const QString &searchTerm)
+{

m_searchTerm = searchTerm;

if (searchTerm.length() > 2

&& searchTerm.at(0) == QLatin1Char('"') && searchTerm.at(searchTerm.length() - 1) == QLatin1Char('"')) {

m_searchTermWords.append(m_searchTerm.mid(1, m_searchTerm.length() - 2));

Do you mean you should assign and not append? I admit I didn't catch too :S

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/qbittorrent/qBittorrent/pull/3989/files/e5b9855c7860a6ea54383758200bbb7f91116e04#r58318785

zeule · 2016-04-03T23:28:38Z

@sledgehammer999
I've implemented your edits and suggestions. Seems to be working and is ready to be merged. Let me remind you to merge #5058, as the changes in the plugins code moved there.

zeule · 2016-04-03T23:31:36Z

Ups... Checks failed

Since we already have searchtab.ui, let's set up all the widgets there. Additionally, save a bit of vertical space by putting results label in a row with the filter widgets.

zeule · 2016-04-04T00:23:44Z

Ups... Checks failed

I've replaced model file include in the searchtab.h header with forward declaration. I don't know why Qt4 Travis build was failing with

#include <QMetaType>
#include <QWidget>

in searchtab.h. My local clean build was compiling fine. So, I've added an illogical include into that file, <QVariant>. The build passed OK then, but I can't understand what was wrong because the error was in moc_ file, and my local moc_ file is obviously different from the one in Travis.

zeule · 2016-04-04T00:32:55Z

If somebody wants to help to sort it out, to reproduce just replace

#include <QVariant>

with

#include <QMetaType>

in searchtab.h and compile via qmake and Qt4. CMake build compiles fine, obviously due to the fact that CMake generates a "super-moc" file, which includes all the project moc-files and compile that file instead. A required include comes from one of the other headers in this case.

zeule · 2016-04-04T00:38:41Z

@sledgehammer999, I think this can be merged as it is, with the crazy include.

ngosang · 2016-04-04T18:54:26Z

Works fine for me.

sledgehammer999 · 2016-04-05T22:17:19Z

I think everything works fine. Thank you for the work.

PS: qt4+qmake+QMetaType works for me on Windows and Debian unstable. But isn't very important to find out why travis(ubuntu trusty) fails.
PS1: The "ongoing" icon looks ridiculous but what can you do? It is the oxygen theme.

zeule · 2016-04-05T22:23:03Z

I agree with you that the icons are ugly. AFAIK they were made without designer. We can take an icon from another theme.

sledgehammer999 · 2016-04-05T22:27:58Z

No need. On linux it should use the ones from the system's theme. On Windows/OSX see #4253

zeule · 2016-04-05T22:35:36Z

Unfortunately, those names ("task-") are specific to the Oxygen style.

zeule · 2016-04-05T22:37:26Z

#5058?

zeule · 2016-04-05T22:43:01Z

Oh, I see now your question there. OK. Thank you!

zeule mentioned this pull request Oct 25, 2015

BTDigg search does not work #3812

Closed

glassez reviewed Oct 26, 2015
View reviewed changes

sledgehammer999 reviewed Apr 3, 2016
View reviewed changes

zeule reviewed Apr 3, 2016
View reviewed changes

zeule and others added 2 commits April 4, 2016 02:09

Implement search filters in the proxy model. Partially closes #972

1336cb7

Optimize widgets inside of the search tab

b6b819a

Since we already have searchtab.ui, let's set up all the widgets there. Additionally, save a bit of vertical space by putting results label in a row with the filter widgets.

sledgehammer999 merged commit b300482 into qbittorrent:master Apr 5, 2016

zeule deleted the filter-search branch April 6, 2016 21:23

ngosang mentioned this pull request Apr 24, 2016

Wishlist Filter search results and search operators #153

Closed

ngosang mentioned this pull request Jun 8, 2016

Torlock search plugin returns OR not AND results for search terms; #5354

Closed

ngosang mentioned this pull request Jan 21, 2017

Add a filter to the search tabs #36

Closed

Implement search filters. Partially closes #972 #3989

Implement search filters. Partially closes #972 #3989

Conversation

zeule commented Oct 25, 2015

Pireo commented Oct 25, 2015

DoumanAsh commented Oct 26, 2015

DoumanAsh commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassez commented Oct 26, 2015

zeule commented Oct 26, 2015

zeule commented Oct 26, 2015

Choose a reason for hiding this comment

glassez commented Oct 26, 2015

zeule commented Oct 26, 2015

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

DoumanAsh commented Oct 26, 2015

zeule commented Oct 26, 2015

DoumanAsh commented Oct 26, 2015

zeule commented Oct 26, 2015

glassez commented Oct 27, 2015

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

zeule commented Apr 3, 2016

sledgehammer999 commented Apr 3, 2016

sledgehammer999 commented Apr 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented Apr 3, 2016

zeule commented Apr 3, 2016

zeule commented Apr 4, 2016

zeule commented Apr 4, 2016

zeule commented Apr 4, 2016

ngosang commented Apr 4, 2016

sledgehammer999 commented Apr 5, 2016

zeule commented Apr 5, 2016

sledgehammer999 commented Apr 5, 2016

zeule commented Apr 5, 2016

zeule commented Apr 5, 2016

zeule commented Apr 5, 2016