Replace use of TriStateBool with boost::optional. #7906

tgregerson · 2017-11-26T21:53:54Z

boost::optional (and eventually std::optional in C++17) provide a generalized mechanism for specifying tristate variables of any type.

I discovered I needed tristate variables of different types than bool while scoping out the work for #5201 / #5779 so it made sense to do this cleanup first.

boost::optional (and std::optional in C++17) provide a more general mechanism for specifying tri-state values that works for types other than bool.

tgregerson · 2017-11-26T23:21:32Z

Seems like this Travis build does not work with boost features from 4/2014? What is the minimum boost version qbt is supposed to support?

older "get" methods. These are mostly functionally equivalent, and are supported on older versions of Boost.

Chocobo1 · 2017-11-27T04:48:25Z

http://www.boost.org/doc/libs/1_65_1/libs/optional/doc/html/boost_optional/tutorial/a_note_about_optional_bool_.html
What about using boost::tribool? I surveyed a while ago and find it incompatible (when doing comparisons), maybe you can work it out.

tgregerson · 2017-11-27T12:34:47Z

http://www.boost.org/doc/libs/1_65_1/libs/optional/doc/html/boost_optional/tutorial/a_note_about_optional_bool_.html
What about using boost::tribool? I surveyed a while ago and find it incompatible (when doing comparisons), maybe you can work it out.

I read that note and considered boost::tribool at the onset, but I think that optional is a better fit.

As the author of that boost note mentions, tribool treats "indeterminate" as a third valid state, whereas optional treats "none" as an invalid state indicating a missing value. The latter semantics are what we want. We are using them to indicate cases where the user has not provided a value.
I was motivated to write this PR when changing some torrent creation parameters with non-bool types (e.g. int, double) over to being boost::optional. Having a single API for dealing with optional parameters made the code cleaner than having one API for bools and a completely different API for all other types.

Optional

…opulate

Chocobo1 · 2017-12-06T14:42:49Z

What is the minimum boost version qbt is supposed to support?

IIRC version 1.35 is defined in configure.ac, what minimum boost version is in line with the namings in std::optional?

Chocobo1 · 2017-12-06T15:00:37Z

src/app/cmdoptions.cpp

-    else if (skipDialog == TriStateBool::False) {
-        result.append(QLatin1String("@skipDialog=0"));
+    if (skipDialog) {
+        if (skipDialog.get())


IMO you should decide on one: operator* or value(), I saw both are used.

We can't use value() since it requires a newer version of boost. Probably better to use operator* then, since get() is deprecated.

Chocobo1 · 2017-12-06T15:09:17Z

src/base/rss/rss_autodownloadrule.cpp

-        default: return QJsonValue();
-        }
+        if (opt)
+            return QJsonValue(*opt);


return *opt?

Chocobo1 · 2017-12-06T15:13:24Z

src/base/rss/rss_autodownloadrule.cpp

-        }
+        if (opt)
+            return QJsonValue(*opt);
+        return QJsonValue();


personal preference:

if (!opt) return QJsonValue(); return *opt;

Chocobo1 · 2017-12-06T15:14:23Z

src/base/rss/rss_autodownloadrule.cpp

        }
+        return ret;


ditch ret and inline the returns.

Chocobo1 · 2017-12-06T15:15:07Z

src/gui/addnewtorrentdialog.cpp

-        ui->startTorrentCheckBox->setChecked(true);
-    else
-        ui->startTorrentCheckBox->setChecked(!session->isAddTorrentPaused());
+    ui->startTorrentCheckBox->setChecked(m_torrentParams.addPaused.get_value_or(!session->isAddTorrentPaused()));


wrong translation

Fixed, thanks

Chocobo1 · 2017-12-06T15:22:01Z

src/gui/rss/automatedrssdownloader.cpp

-        else if (m_currentRule.addPaused() == TriStateBool::False)
-            index = 2;
+        if (m_currentRule.addPaused()) {
+            if (m_currentRule.addPaused().get())


use optional::operator==:

if (m_currentRule.addPaused() == true) index = 1; else index = 2;

at least std::optional works like it, not sure about boost:optional

Unfortunately not supported

Chocobo1 · 2017-12-06T15:23:58Z

src/app/cmdoptions.cpp

-    }
-    else if (addPaused == TriStateBool::False) {
-        result.append(QLatin1String("@addPaused=0"));
+    if (addPaused) {


use operator==

There is no operator== (or any comparators) in boost's version of optional.

According to this gotcha you can do:

if (addPaused == true) result.append(QLatin1String("@addPaused=1")); else if (addPaused == false) result.append(QLatin1String("@addPaused=0"));

Its simpler. But will it confuse people? Maybe.
I don't have strong preference either way. Do you guys have any argument for or against one or the other?

I prefer @sledgehammer999 variant.

Chocobo1 · 2017-12-06T15:24:07Z

src/app/cmdoptions.cpp

-    }
-    else if (skipDialog == TriStateBool::False) {
-        result.append(QLatin1String("@skipDialog=0"));
+    if (skipDialog) {


use operator==

tgregerson · 2017-12-10T21:21:42Z

IIRC version 1.35 is defined in configure.ac, what minimum boost version is in line with the namings in std::optional?

value() and value_or() require Boost 1.56.

operator* is supported in 1.35, so that can be used in place of value(). The only difference is in exception handling.

Chocobo1 · 2017-12-11T02:18:25Z

OK, you can squash the commits (I would squash all into the first one).

Chocobo1 · 2017-12-11T02:33:10Z

@sledgehammer999
Are you OK with bumping boost minimum version to 1.56?
boost 1.56 was released 3 years ago 2014/08/07

LordNyriox · 2017-12-11T17:26:12Z

@Chocobo1:

Are you OK with bumping boost minimum version to 1.56?

IIRC qBittorrent cannot use Boost 1.56 (it was stuck at 1.55 for a while), due to a change in Boost.asio.

Perhaps use the first version to support the define workaround, instead?

boost::optional (and std::optional in C++17) provide a more general mechanism for specifying tri-state values that works for types other than bool.

tgregerson · 2017-12-12T00:09:51Z

OK, you can squash the commits (I would squash all into the first one).

Done

Chocobo1 · 2017-12-12T04:55:41Z

IIRC qBittorrent cannot use Boost 1.56 (it was stuck at 1.55 for a while), due to a change in Boost.asio.

I don't remember that... or you might be talking about #4112?
That issue is old, things should be different now.
This PR is good to merge, the boost version bump can be done in another PR.

Replace use of TriStateBool with boost::optional.

LordNyriox · 2017-12-15T15:58:11Z

@Chocobo1: No, I was referring to the issue that @sledgehammer999 solved using BOOST_ASIO_DISABLE_CONNECTEX.
That define wasn't added until Boost 1.58, but the connectex problem started in 1.56.

I do not recall if an alternative solution to this problem was discovered or not. If not, please make the minimum Boost version 1.58 rather than 1.56.

Chocobo1 · 2017-12-15T16:12:09Z

I do not recall if an alternative solution to this problem was discovered or not.

That issue is still open: arvidn/libtorrent#314 (comment)
But it's less relevant now, havn't seen a complain for some time.

Bumping to boost 1.58 which is released on 2015-04-17 (quite ancient) shouldn't pose a problem I guess.

sledgehammer999 · 2017-12-24T00:33:27Z

src/base/rss/rss_autodownloadrule.cpp

        }
+        return 0; // use default


I think here you can safely do:

if (opt == true) return 1; // always else if (opt == false) return 2; // never else return 0; // use default

A reader might scratch his head at first but afterwards he'll realize what we do here. No?

I like the @sledgehammer999 variant (if it can be compiled) except the else's after return's.

I think @glassez means:

if (opt == true) return 1; // always else if (opt == false) return 2; // never return 0; // use default

if (opt == true) return 1; // always if (opt == false) return 2; // never return 0; // use default

Sorry I'm late to this, just want to point out it might be dangerous to use this form (in this specific case optional<bool>), one might transform it to if (opt) return 1; if (!opt) return 2; which is clearly not the same thing.
To avoid this pitfall, I personally like to see it as a container type, using operator* to access it, which is also why I didn't think of using operator== for comparing with values.

Just sharing my view point.

if (opt) return 1; if (!opt) return 2;

In my opinion, it looks "too ugly" to some sane developer want to write it like this.

In fact, this "extreme case" has forced me to abandon the use of any generic classes (boost::optional etc.). My TriStateBool doesn't allow any confusing implicit conversions.
But I don't want, of course, to prevent these changes because of their further spread to other fields. I just wish we have defined some common policy for its use.

I didn't think of using operator== for comparing with values.

But it's convenient, at least for types other than bool. Otherwise boost::optional becomes not much clear than using ordinary pointers.

Using if (optional) to check for value existence is convient but it's confusing in case of bool.

sledgehammer999 · 2017-12-24T00:47:22Z

Btw, IMO we can safely bump Boost requirement to 1.56. The problem @LordNyriox mentions is valid only for Windows. And I am not going to provide Windows builds based on that old version anyway.

I left 2 inline comments. They aren't merge-blocking.

Since TriStateBool was implemented by @glassez, let's tag him in case he hasn't seen this already.

glassez · 2017-12-24T04:20:33Z

I need to make all add torrent params optional (for my further features) but I considered it in another way. I thought to implement AddTorrentParams as QVariantHash. But it also can be implemented as struct of optionals. @sledgehammer999, if you prefer this way, let me known and then I'll review this PR.

tgregerson · 2017-12-24T14:03:35Z

I need to make all add torrent params optional (for my further features) but I considered it in another way. I thought to implement AddTorrentParams as QVariantHash. But it also can be implemented as struct of optionals. @sledgehammer999, if you prefer this way, let me known and then I'll review this PR.

FWIW, I already have the conversion of the rest of the struct to optionals done in another PR that I haven't sent for review yet. I was waiting for this one to get merged before sending it out.

sledgehammer999 · 2017-12-26T22:37:09Z

@glassez since its done already here there's probably no reason to do it via QVariantHash.
So if you want proceed with review.

glassez

Also shouldn't we care about easy transition to std::optional? Maybe declare aliases for both boost::optional and boost::none?

glassez · 2017-12-27T07:17:06Z

src/base/rss/rss_autodownloadrule.cpp

 #include "../utils/fs.h"
 #include "../utils/string.h"
 #include "rss_feed.h"
 #include "rss_article.h"

 namespace
 {
-    TriStateBool jsonValueToTriStateBool(const QJsonValue &jsonVal)
+    boost::optional<bool> jsonValueToOptional(const QJsonValue &jsonVal)


jsonValueToOptionalBool or just jsonValueToBool

Done (and elsewhere)

glassez · 2017-12-27T07:18:44Z

src/base/rss/rss_autodownloadrule.cpp

    }

-    QJsonValue triStateBoolToJsonValue(const TriStateBool &triStateBool)
+    QJsonValue optionalToJsonValue(const boost::optional<bool> &opt)


optionalBoolToJsonValue or boolToJsonValue

glassez · 2017-12-27T07:19:31Z

src/base/rss/rss_autodownloadrule.cpp

    }

-    TriStateBool addPausedLegacyToTriStateBool(int val)
+    boost::optional<bool> addPausedLegacyToOptional(int val)


The same as above

addPausedLegacyToOptionalBool, as I said already.

glassez · 2017-12-27T07:19:52Z

src/base/rss/rss_autodownloadrule.cpp

        }
    }

-    int triStateBoolToAddPausedLegacy(const TriStateBool &triStateBool)
+    int optionalToAddPausedLegacy(const boost::optional<bool> &opt)


The same as above.

glassez · 2017-12-27T07:30:35Z

src/base/rss/rss_autodownloadrule.cpp

+        if (opt) {
+            if (*opt)
+                return 1; // always
+            else


In any case else is redundant after return.

glassez · 2017-12-27T07:32:41Z

src/base/rss/rss_autodownloadrule.cpp

        }
+        return 0; // use default


I like the @sledgehammer999 variant (if it can be compiled) except the else's after return's.

sledgehammer999 · 2017-12-27T13:10:58Z

Also shouldn't we care about easy transition to std::optional? Maybe declare aliases for both boost::optional and boost::none?

Sounds good, but once we replace it with std::optional we would continue using a "useless/extra" typedef/alias. So I am in favor of having before our eyes the real type*.
*Unless we want to shorten it for readability.

tgregerson · 2017-12-31T18:49:16Z

Also shouldn't we care about easy transition to std::optional? Maybe declare aliases for both boost::optional and boost::none?

I think it will probably be easier to just do a find & replace to swap types rather than having a header with aliases that needs to get included everywhere they are used and eventually deleted after the switch.

LordNyriox · 2018-10-03T16:35:12Z

@tgregerson: Any chance you can rebase / update this PR?

It has been stagnant since February.

glassez · 2018-10-03T16:42:11Z

@LordNyriox, please don't ask people to do potentially useless work. These changes are still not generally approved and may never be approved. But if we want to merge it, we'll ask to rebase the code in due time.

tgregerson · 2018-10-03T23:53:38Z

I'll reintegrate if the powers that be can come to a consensus that we should move forward with it.

As a side note, I would still like to implement the automatic category assignment feature I mentioned in the original comment on this PR, but I have not been able to make any progress since this first piece has been in limbo.

glassez

I've just re-read this topic and re-review the code. In principle, this is not bad in itself (even if we do not look at other scheduled changes). Since I don't see any blocking objections from other project members I think we can go forward with it once it is rebased and fixed.

glassez · 2018-10-04T04:05:19Z

src/app/application.cpp

@@ -426,7 +428,7 @@ void Application::processParams(const QStringList &params)
        }

        if (param.startsWith(QLatin1String("@addPaused="))) {
-            torrentParams.addPaused = param.mid(11).toInt() ? TriStateBool::True : TriStateBool::False;
+            torrentParams.addPaused = param.mid(11).toInt() != 0;


Please wrap right part with parentheses.
And in assignment below too.

glassez · 2018-10-04T04:13:23Z

src/base/rss/rss_autodownloadrule.cpp

    }

-    TriStateBool addPausedLegacyToTriStateBool(int val)
+    boost::optional<bool> addPausedLegacyToOptional(int val)


addPausedLegacyToOptionalBool, as I said already.

glassez · 2018-10-04T04:16:04Z

src/base/rss/rss_autodownloadrule.cpp

-        }
+        if (opt == true)
+            return 1; // always
+        else if (opt == false)


Don't use else after return (as well as after break, continue etc.).

glassez · 2018-10-04T04:30:59Z

src/app/cmdoptions.cpp

@@ -366,12 +366,10 @@ QStringList QBtCommandLineParameters::paramList() const
    if (!savePath.isEmpty())
        result.append(QString("@savePath=%1").arg(savePath));

-    if (addPaused == TriStateBool::True) {
+    if (addPaused == true)


IIRC, boost::optional<bool>::operator== returns false in both cases unless value is set, isn't it?

This reverts commit e2e6638, reversing changes made to 75e1ece.

boost::optional (and std::optional in C++17) provide a more general mechanism for specifying tri-state values that works for types other than bool. Replace use of the newer "value" methods from boost::optional with the older "get" methods. These are mostly functionally equivalent, and are supported on older versions of Boost. boost::optional fixup optional cmdoptions fixup Address comments Revert "Merge branch 'autopopulate' into master" This reverts commit e2e6638, reversing changes made to 75e1ece.

…ional

tgregerson · 2018-10-13T23:37:06Z

I gave up on attempting to rebase. It would be easier to start from scratch than to try to resolve all the merge conflicts.

LordNyriox · 2018-10-14T01:00:58Z

@tgregerson:

It would be easier to start from scratch

Will you do so?

tgregerson added 2 commits November 26, 2017 10:25

Snapshot

4ea1761

Replace use of TriStateBool with boost::optional.

f7dbf6a

boost::optional (and std::optional in C++17) provide a more general mechanism for specifying tri-state values that works for types other than bool.

Replace use of the newer "value" methods from boost::optional with the

a7701ca

older "get" methods. These are mostly functionally equivalent, and are supported on older versions of Boost.

tgregerson added 2 commits November 29, 2017 17:27

Merge pull request #1 from tgregerson/optional

43d1979

Optional

Merge branch 'master' of github.com:tgregerson/qBittorrent into autop…

404cbf3

…opulate

Chocobo1 reviewed Dec 6, 2017

View reviewed changes

Replace use of TriStateBool with boost::optional.

f17e075

boost::optional (and std::optional in C++17) provide a more general mechanism for specifying tri-state values that works for types other than bool.

tgregerson force-pushed the optional branch from c05455f to f17e075 Compare December 12, 2017 00:08

Chocobo1 previously approved these changes Dec 12, 2017

View reviewed changes

tgregerson added 2 commits December 12, 2017 06:29

Merge pull request #2 from tgregerson/optional

75e1ece

Replace use of TriStateBool with boost::optional.

Merge branch 'autopopulate' into master

e2e6638

sledgehammer999 reviewed Dec 24, 2017

View reviewed changes

glassez requested changes Dec 27, 2017

View reviewed changes

tgregerson dismissed Chocobo1’s stale review via 6787822 December 31, 2017 18:42

glassez requested changes Oct 4, 2018

View reviewed changes

glassez reviewed Oct 4, 2018

View reviewed changes

tgregerson added 3 commits October 13, 2018 16:13

boost::optional fixup

d0d62e4

optional cmdoptions fixup

7a3b7c9

Address comments

187d536

tgregerson force-pushed the optional branch from 976cdff to 187d536 Compare October 13, 2018 21:37

tgregerson added 4 commits October 13, 2018 16:46

Revert "Merge branch 'autopopulate' into master"

dc505a0

This reverts commit e2e6638, reversing changes made to 75e1ece.

Merge branch 'optional' of github.com:tgregerson/qBittorrent into opt…

7713318

…ional

Add parentheses around comparison

7223694

tgregerson closed this Oct 13, 2018

Replace use of TriStateBool with boost::optional. #7906

Replace use of TriStateBool with boost::optional. #7906

Conversation

tgregerson commented Nov 26, 2017

tgregerson commented Nov 26, 2017

Chocobo1 commented Nov 27, 2017

tgregerson commented Nov 27, 2017

Chocobo1 commented Dec 6, 2017

Chocobo1 Dec 6, 2017 • edited

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

Chocobo1 Dec 6, 2017 • edited

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

tgregerson commented Dec 10, 2017

Chocobo1 commented Dec 11, 2017

Chocobo1 commented Dec 11, 2017

LordNyriox commented Dec 11, 2017

tgregerson commented Dec 12, 2017

Chocobo1 commented Dec 12, 2017

LordNyriox commented Dec 15, 2017 • edited

Chocobo1 commented Dec 15, 2017

sledgehammer999 Dec 24, 2017 • edited

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

Chocobo1 Jan 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sledgehammer999 commented Dec 24, 2017

glassez commented Dec 24, 2017

tgregerson commented Dec 24, 2017

sledgehammer999 commented Dec 26, 2017

glassez left a 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

sledgehammer999 commented Dec 27, 2017 • edited

tgregerson commented Dec 31, 2017

LordNyriox commented Oct 3, 2018

glassez commented Oct 3, 2018

tgregerson commented Oct 3, 2018

glassez left a 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

tgregerson commented Oct 13, 2018

LordNyriox commented Oct 14, 2018

Chocobo1 Dec 6, 2017 •

edited

Chocobo1 Dec 6, 2017 •

edited

LordNyriox commented Dec 15, 2017 •

edited

sledgehammer999 Dec 24, 2017 •

edited

Chocobo1 Jan 1, 2018 •

edited

sledgehammer999 commented Dec 27, 2017 •

edited