Implement configurations and portable mode. Closes #465 #5214

Open
wants to merge 6 commits into
from

Projects

None yet

5 participants

@evsh
Contributor
evsh commented May 3, 2016 edited

It may be useful to have different configurations either for portable versions or for debugging purposes. To implement this we add two options, avaliable via command line switches

  1. An option to change configuration name (--configuration). The name supplied via this option is appended to QCoreApplication::applicationName() to form qBittorrent_<conf_name> name for the configuration files.
  2. An option to provide a path do directory where all the settings are stored (kind of profile directory). There is a shortcut --portable which means "use directory profile near the executable location".

In order to implement that we have to perform initialisation of the profile directories before the SettingStorage and Preferences singletones are initialised. Thus, options parsing shall be performed without defaults read from preferences.

Update:
Added command line options to implement both the different configurations and portable mode.

@evsh evsh and 1 other commented on an outdated diff May 3, 2016
src/base/utils/fs.cpp
@@ -518,3 +519,9 @@ QString Utils::Fs::cacheLocation()
locationDir.mkpath(locationDir.absolutePath());
return location;
}
+
+QString Utils::Fs::configurationName()
+{
+ QString envConfName = QString::fromLocal8Bit(qgetenv("QBT_CONF_NAME"));
@evsh
evsh May 3, 2016 Contributor

It might make sense to make this variable static. However, I can't imagine qBt changing this environment variable at run time.

@glassez
glassez May 4, 2016 Contributor

Change this in runtime will cause undefined behaviour or even crash the program. So it must be initialized at program startup and do not change further.

@sledgehammer999
Contributor

Unless I am mistaken you actually implement #465 in some way. However, under this light an environment variable isn't very desirable :p

@evsh
Contributor
evsh commented May 3, 2016

Hm... Kind of... I just tired of qBt rechecking my files over and over again.

As to me, the environment variable approach gives flexibility for developers. One can have separate configurations for different tests, check how qBt behaves with an empty configuration (by the way, it crashes with LC_COLLATE=C when opening the options dialogue) in no time.

Do you suggest to add a command line switch to set this variable for the portable version?

@evsh
Contributor
evsh commented May 3, 2016

Do you suggest to add a command line switch to set this variable for the portable version?

And, of course, a code that will check that variable contents for an absolute path, which will be used as is but not appended.

@sledgehammer999
Contributor

check how qBt behaves with an empty configuration (by the way, it crashes with LC_COLLATE=C when opening the options dialogue) in no time.

This shouldn't happen. Clean installs should just work.

Do you suggest to add a command line switch to set this variable for the portable version?

If it isn't trouble for you. Am I wrong or did the last merges greatly simplified the code for "portable version" implementation?

And, of course, a code that will check that variable contents for an absolute path, which will be used as is but not appended.

In portable versions the config could reside in the same folder as the binary. So relative paths should work too.

@evsh
Contributor
evsh commented May 3, 2016

Configuration name is appended to the paths like QDir::homePath().

@evsh
Contributor
evsh commented May 3, 2016

I'm going to pack Utils::Fs::QDesktopServices implementations into a class, add an override version with manually specified root path and instantiate one or another basing on the command line switch. What do you think?

@evsh
Contributor
evsh commented May 3, 2016

@qbittorrent/qbittorrent-frequent-contributors Can someone do me a favor, please, and make extensive tests on Windows and OS X for the portable version?

@glassez glassez commented on an outdated diff May 4, 2016
src/base/utils/fs.h
@@ -67,6 +67,10 @@ namespace Utils
/* End of Qt4 code */
QString cacheLocation();
+
+ /// Returns either default name for configuration file (QCoreApplication::applicationName())
+ /// or the value, supplied via QBT_CONF_NAME environment variable
+ QString configurationName();
@glassez
glassez May 4, 2016 Contributor

This function, like some other (cacheLocation, QDesktopServices*) , needs to be moved to another place, because they sense are not file system utilities. These are shell or environment utilities.

@glassez glassez commented on an outdated diff May 4, 2016
src/base/settingsstorage.cpp
@@ -70,9 +70,9 @@ namespace
SettingsPtr createSettings(const QString &name)
{
#ifdef Q_OS_WIN
- return SettingsPtr(new QSettings(QSettings::IniFormat, QSettings::UserScope, "qBittorrent", name));
+ return SettingsPtr(new QSettings(QSettings::IniFormat, QSettings::UserScope, Utils::Fs::configurationName(), name));
@glassez
glassez May 4, 2016 Contributor

SettingsStorage is not the only class that requires modifications. There are some other classes that store all their settings/state/data in the same place. Although it may be enough changes in QDesktopServicesDataLocation for them.

@evsh evsh changed the title from Add a code path to override configuration name to Implement configurations and portable mode. Closes #465 May 10, 2016
@evsh
Contributor
evsh commented May 10, 2016 edited

PR updated. Now it implements portable mode. Needs testing.

@sledgehammer999: support for command line switches has been added, but environment variables are supported too. I believe they are better for debugging and may be of use for users too.

Two settings are proposed:
--profile=<dir> allows to set configuration prefix. Inside of it directories cache, config, data, downloads are created. If the switch is not supplied, default system dirs are used.
Option --portable is a shortcut for --profile=<exe dir>/profile.
--configuration=<name> option allows to have different configuration inside of a configuration prefix.

If --profile option is not empty, SettingStorage serialises settings in QSettings::IniFormat to make them portable between platforms.

@evsh
Contributor
evsh commented May 10, 2016

There is a destructive change: usage help text does not contain the Web UI port number from settings anymore because options parsing needs to be done before preferences initialisation.

@glassez glassez commented on an outdated diff May 11, 2016
src/base/utils/environment.h
+#include <memory>
+
+#include <QString>
+#include <QScopedPointer>
+#include <QSettings>
+
+class Application;
+
+namespace Utils
+{
+ namespace Envrironment
+ {
+ namespace impl
+ {
+ class SpecialFoldersImpl;
+ class SpecialFoldersSingletongImpl;
@glassez
glassez May 11, 2016 Contributor

Singletong?

@glassez glassez and 1 other commented on an outdated diff May 11, 2016
src/base/utils/fs.cpp
@@ -377,137 +360,17 @@ QString Utils::Fs::expandPathAbs(const QString& path)
QString Utils::Fs::QDesktopServicesDataLocation()
@glassez
glassez May 11, 2016 Contributor

Why do you leave these ugly wrappers?
Why shouldn't we use Environment utils directly?

@evsh
evsh May 11, 2016 Contributor

OK.

@glassez glassez commented on an outdated diff May 11, 2016
src/base/utils/environment.cpp
+ * License in all respects for all of the code used other than "OpenSSL". If you
+ * modify file(s), you may extend this exception to your version of the file(s),
+ * but you are not obligated to do so. If you do not wish to do so, delete this
+ * exception statement from your version.
+ *
+ */
+
+#include "environment.h"
+
+#include <QCoreApplication>
+
+#include "environment_p.h"
+
+Utils::Envrironment::SpecialFolders*Utils::Envrironment::SpecialFolders::m_instance = nullptr;
+
+Utils::Envrironment::SpecialFolders::~SpecialFolders()
@glassez
glassez May 11, 2016 Contributor

using namespace Utils::Environment is good idea!

@evsh evsh and 1 other commented on an outdated diff May 11, 2016
src/base/utils/fs.cpp
- save_path = QDir::home().absoluteFilePath(QCoreApplication::translate("fsutils", "Downloads"));
- qDebug() << Q_FUNC_INFO << "using" << save_path << "as fallback since the XDG detection did not work";
- }
-
- return save_path;
-#endif
-
-#if defined(Q_OS_MAC)
- // TODO: How to support this on Mac OS?
-#endif
-
- // Fallback
- return QDir::home().absoluteFilePath(QCoreApplication::translate("fsutils", "Downloads"));
-#endif
-}
-
QString Utils::Fs::cacheLocation()
@evsh
evsh May 11, 2016 Contributor

I don't understand this function and thus left it as is.

@glassez
glassez May 11, 2016 Contributor

It returns the cache dir path and create this directory if it doesn't exist which is overkill. So you may drop this function and use your Environment utils directly.

@evsh
evsh May 11, 2016 Contributor

But why it was written? Apparently, somebody encountered a situation when the cache directory was missing. Should we create them all the directories when we create the SpecialFoldersImpl object?

@evsh
evsh May 11, 2016 Contributor

Should we create them all the directories when we create the SpecialFoldersImpl object?

After some thinking I like this approach. In Utils::Envrironment::SpecialFolders::initialize() we ensure that all paths exist in the same way as we do in Utils::Envrironment::impl::SpecialFoldersImplCustom::SpecialFoldersImplCustom and remove all the QDir::mkpath() calls from the individual functions. The program can not run without the config, cache, and data directories. As for the downloads directory, we already create it now anyway.

@evsh
Contributor
evsh commented May 11, 2016

This closes #4887 too

@evsh
Contributor
evsh commented May 11, 2016

@qbittorrent/qbittorrent-frequent-contributors:
This is more or less ready. With two exceptions (see below) strace qbittorrent --profile /tmp/qbt_profile does not show any app-specific file activity in ${HOME}.

The first exception is ${HOME}/.config/qBittorrentrc, but I guess it is used by KF5, and we can do nothing about it. The file keeps KFileDialog settings.

The second exception is the QIniSettings class. It is used in statistics and RSS. And my question is: may I remove the QIniSettings class? Why is it needed? May I replace it with Utils::Environment::SpecialFolders::applicationSettings() calls? We can add an overloaded applicationSettings() for that cases to pass QSettings::Format.

@Chocobo1 Chocobo1 and 1 other commented on an outdated diff May 11, 2016
src/app/options.h
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * In addition, as a special exception, the copyright holders give permission to
+ * link this program with the OpenSSL project's "OpenSSL" library (or with
+ * modified versions of it that use the same license as the "OpenSSL" library),
+ * and distribute the linked executables. You must obey the GNU General Public
+ * License in all respects for all of the code used other than "OpenSSL". If you
+ * modify file(s), you may extend this exception to your version of the file(s),
+ * but you are not obligated to do so. If you do not wish to do so, delete this
+ * exception statement from your version.
+ *
+ * Contact : chris@qbittorrent.org
+ */
+
@Chocobo1
Chocobo1 May 11, 2016 Member

Is include guard omitted on purpose?

@evsh
evsh May 11, 2016 Contributor

No, it was a mistake. Thanks!

@Chocobo1 Chocobo1 commented on an outdated diff May 11, 2016
src/base/utils/environment.h
+ {
+ Cache,
+ Config,
+ Data,
+ Downloads
+ };
+
+ class SpecialFolders
+ {
+ public:
+ QString location(SpecialFolder folder) const;
+ SettingsPtr applicationSettings(const QString &name) const;
+
+ /// Returns either default name for configuration file (QCoreApplication::applicationName())
+ /// or the value, supplied via parameters
+ QString configurationName();
@Chocobo1
Chocobo1 May 11, 2016 Member

You can add const here.

@Chocobo1 Chocobo1 commented on an outdated diff May 11, 2016
src/base/utils/environment_p.cpp
+}
+
+QString Utils::Envrironment::impl::SpecialFoldersImplCustom::dataLocation() const
+{
+ return m_rootDirectory.absoluteFilePath(dataDirName);
+}
+
+QString Utils::Envrironment::impl::SpecialFoldersImplCustom::downloadLocation() const
+{
+ return m_rootDirectory.absoluteFilePath(downloadsDirName);
+}
+
+Utils::Envrironment::SettingsPtr Utils::Envrironment::impl::SpecialFoldersImplCustom::applicationSettings(const QString &name) const
+{
+ // here we force QSettings::IniFormat format because we need it to be portable across platforms
+ return SettingsPtr(new QSettings(QDir(configLocation()).absoluteFilePath(name + QLatin1String(".conf")), QSettings::IniFormat));
@Chocobo1
Chocobo1 May 11, 2016 Member

maybe just me, you could use a temporary QString to hold the QDir::absoluteFilePath(), will be easier to read.

@Chocobo1 Chocobo1 commented on an outdated diff May 11, 2016
src/base/utils/environment_p.h
+ class SpecialFoldersImplCustom: public SpecialFoldersImpl
+ {
+ public:
+ SpecialFoldersImplCustom(const QString &rootPath, bool create, const QString &configurationName);
+
+ QString cacheLocation() const override;
+ QString configLocation() const override;
+ QString dataLocation() const override;
+ QString downloadLocation() const override;
+ SettingsPtr applicationSettings(const QString &name) const override;
+
+ private:
+ QDir m_rootDirectory;
+ static const QString cacheDirName;
+ static const QString configDirName;
+ static const QString dataDirName;
@Chocobo1
Chocobo1 May 11, 2016 Member
const QString dataDirName = QLatin1String("data");  // Why not this?
@Chocobo1 Chocobo1 commented on the diff May 11, 2016
src/app/main.cpp
- result.unknownParameter = arg;
- break;
- }
- }
- else {
- QFileInfo torrentPath;
- torrentPath.setFile(arg);
-
- if (torrentPath.exists())
- result.torrents += torrentPath.absoluteFilePath();
- else
- result.torrents += arg;
- }
+ return app->exec(params.torrents);
+ }
+ catch (CommandLineParameterError &er) {
@Chocobo1
Chocobo1 May 11, 2016 Member

This catch is quite away from the throws, is it a good idea to move up?

@evsh
evsh May 11, 2016 Contributor

Agree, the body of main() is too long, we better split it onto several functions.

@Chocobo1 Chocobo1 and 1 other commented on an outdated diff May 11, 2016
src/app/options.h
+#ifndef DISABLE_GUI
+ bool noSplash;
+#else
+ bool shouldDaemonize;
+#endif
+ int webUiPort;
+ QString profileDir;
+ bool portableMode;
+ QString configurationName;
+ QStringList torrents;
+ QString unknownParameter;
+
+ QBtCommandLineParameters();
+};
+
+class CommandLineParameterError: public std::runtime_error
@Chocobo1
Chocobo1 May 11, 2016 Member

can you share the reason of handling error by exceptions? I'm curious about it.

@evsh
evsh May 11, 2016 Contributor

Two reasons: I had to signal error from Application::Application() and main() had many repeative calls to displayBadArgMessage() with return EXIT_FAILURE (that is in catch clause now).

@Chocobo1 Chocobo1 and 2 others commented on an outdated diff May 11, 2016
src/base/utils/environment.h
+ }
+
+ using SettingsPtr = std::unique_ptr<QSettings>;
+
+ enum class SpecialFolder
+ {
+ Cache,
+ Config,
+ Data,
+ Downloads
+ };
+
+ class SpecialFolders
+ {
+ public:
+ QString location(SpecialFolder folder) const;
@Chocobo1
Chocobo1 May 11, 2016 Member

const SpecialFolder folder?

@evsh
evsh May 11, 2016 Contributor

Sorry, can not understand why it would be better with const.

@Chocobo1
Chocobo1 May 11, 2016 Member

By reading the method signature I would suspect folder will somehow change to other value, but it didn't.
Well this is a habit of mine...

@glassez
glassez May 11, 2016 Contributor

By reading the method signature I would suspect folder will somehow change to other value, but it didn't.

You read the method signature incorrectly! The folder cannot be changed since it passed by value. The method free to change its folder copy.

@Chocobo1
Chocobo1 May 11, 2016 Member

You read the method signature incorrectly!

You must have misunderstood me.
I meant I suspect the local copy folder will somehow change to other value but it didn't.

@glassez
glassez May 11, 2016 Contributor

Passing param by value as const creates hard dependency between function signature and its implementation. If in the future we will need to do some preprocessing of this param, we will either have to change the method signature or use some tricks (create additional variables, etc.).

@evsh
evsh May 11, 2016 Contributor

I hope that even with this polemic you guys will find a time to look at QIniSettings and think whether we can drop it completely, please? :)

@Chocobo1
Chocobo1 May 11, 2016 Member

@glassez
That sounds like you don't like to add const for pass-by-value parameters!?

@glassez
glassez May 11, 2016 Contributor

That sounds like you don't like to add const for pass-by-value parameters!?

Yes. Except some really special cases.

@glassez
Contributor
glassez commented May 11, 2016

This is more or less ready.

@evsh less than you think.
Your biggest error here is that you changed the order of things in main(), without knowing how it actually works.
Application instance MUST be created BEFORE we parse command line since QApplication should parse and filter the parameters destined for it. Otherwise you will get an exception if the user wants to specify such command line parameters. The second (minor) mistake is you cannot use any translations before Application is initialized.
I'm not going to dwell on other (less important) issues. First you need to fix this.

@evsh
Contributor
evsh commented May 11, 2016

Application instance MUST be created BEFORE we parse command line since QApplication should parse and filter the parameters destined for it. Otherwise you will get an exception if the user wants to specify such command line parameters.

Can't understand you, sorry. Could you explain it, please?

@glassez
Contributor
glassez commented May 11, 2016

The second exception is the QIniSettings class. It is used in statistics and RSS. And my question is: may I remove the QIniSettings class? Why is it needed?

If you look carefully the code, it becomes clear that it is completely useless. That's why I refused to use it in SettingsStorage class.

May I replace it with Utils::Environment::SpecialFolders::applicationSettings() calls?

IIRC, yes.

BTW, the last time we use the QSettings class not for its intended purpose, and only for serialization of settings into the file. It's all the same to shoot with a Bazooka on the wheel. Isn't it time for us to think about using something more suitable? @sledgehammer999, what do you say?

@glassez
Contributor
glassez commented May 11, 2016 edited

Can't understand you, sorry. Could you explain it, please?

For example, QApplication recognizes -style=<style> argument. IIRC, If user pass it your code throws an exception since it unknown option for qBittorrent. But if you pass command line to QApplication constructor first it filters all known options and modifies argc/argv. Then you can do custom parsing the rest.

@evsh
Contributor
evsh commented May 11, 2016

Oh, I get it now. Thanks! Indeed, I forgot about tanslations and Qt options completetly.

@evsh
Contributor
evsh commented May 11, 2016

If you look carefully the code, it becomes clear that it is completely useless.

I did look, did make the same conclusion and that is why I'm asking.

@glassez
Contributor
glassez commented May 11, 2016 edited

Oh, I get it now. Thanks! Indeed, I forgot about tanslations and Qt options completetly.

You should keep current order and try other solution. E.g. you may parse command line in Application constructor and then use parsed params outside it via some Application method (something like Application::commandLineArgs()).

@evsh
Contributor
evsh commented May 11, 2016

Yes, thanks. It is obvious that parameters parsing has to be done in Application::Application(), because this is the onle place between QApplication::QApplication() and Preferences initialisation.

@glassez
Contributor
glassez commented May 11, 2016

Option --portable is a shortcut for --profile=/profile.
--configuration= option allows to have different configuration inside of a configuration prefix.

What happens if the user set --portable and --profile=some_path together?

@evsh
Contributor
evsh commented May 11, 2016

Application::Application() throws an exception which is shown to the user and the program aborts.

@glassez
Contributor
glassez commented May 11, 2016

Application::Application() throws an exception which is shown to the user and the program aborts.

Ok.

@glassez
Contributor
glassez commented May 11, 2016

@evsh For some reason we need to know the original number of parameters. This can only be done before calling the QApplication constructor.

@evsh
Contributor
evsh commented May 11, 2016

For some reason we need to know the original number of parameters.

Actually, I don't understand this. If a user wants to display the dialogue with the usage text using specific style or stylesheet, why not?

@evsh
Contributor
evsh commented May 11, 2016

PR updated: options parsing was moved into the Application constructor, QIniSettings class was removed.

@evsh
Contributor
evsh commented May 11, 2016

And I would like to add that all the playing with characters in QStrings after working with boost::filesystem::path is like using C after C++.

@glassez
Contributor
glassez commented May 12, 2016

Actually, I don't understand this. If a user wants to display the dialogue with the usage text using specific style or stylesheet, why not?

Ok.

@glassez
Contributor
glassez commented May 12, 2016

@evsh, the user downloads the torrent to removable disk beside portable qBittorrent, and he continues to do this on another computer - it's real use case of portable app?
If yes your implementation is incomplete since we store absolute save paths in resume data.

@glassez
Contributor
glassez commented May 12, 2016

And I would like to add that all the playing with characters in QString s after working with boost::filesystem::path is like using C after C++.

Unfortunately it's incompatible with QString and we would often convert strings to use it. We also can implement QString/QDir based Path class.

@evsh
Contributor
evsh commented May 12, 2016

Unfortunately it's incompatible with QString and we would often convert strings to use it. We also can implement QString/QDir based Path class.

The change would make sense only together with replacing the whole Qt I/O, of course.

@evsh
Contributor
evsh commented May 12, 2016

@evsh, the user downloads the torrent to removable disk beside portable qBittorrent, and he continues to do this on another computer - it's real use case of portable app?
If yes your implementation is incomplete since we store absolute save paths in resume data.

Do you mean libtorrent and its "save_path" attribute?

@glassez
Contributor
glassez commented May 12, 2016

Do you mean libtorrent and its "save_path" attribute?

Both libtorrent save_path and qBittorrent qBt-savePath.

@evsh
Contributor
evsh commented May 12, 2016

Would it be OK if I hook up into the save/load sequences and replace both attributes with a path, relative to the $HOME or profile dir?

@evsh
Contributor
evsh commented May 12, 2016

Would it be OK if I hook up into the save/load sequences and replace both attributes with a path, relative to the $HOME or profile dir?

Oh, no! Windows might have several filesystem roots! What to do then?

@glassez
Contributor
glassez commented May 12, 2016

The change would make sense only together with replacing the whole Qt I/O, of course.

Well, in that case, I prefer not to continue further on this topic.

Oh, no! Windows might have several filesystem roots! What to do then?

We need to carefully consider it. I have no solution yet...

@glassez
Contributor
glassez commented May 12, 2016

@evsh, you can abandon claims for portable mode here, complete everything else, and then you can start to think about it.

@evsh
Contributor
evsh commented May 12, 2016 edited

We need to carefully consider it. I have no solution yet...

I see it in the following way. On saving:
<windows only> If profile_dir drive letter is the same as save_path drive letter</windows only> make save_path relative to the profile_dir.
On loading:
If save_path is relative, make it absolute using profile_dir.

profile_dir is ${HOME} or the one specified via --profile/${QBT_PROFILE}

@evsh
Contributor
evsh commented May 12, 2016

The alternative is to make path relative only if it is inside of profile_dir

@evsh
Contributor
evsh commented May 12, 2016

@evsh, you can abandon claims for portable mode here, complete everything else, and then you can start to think about it.

If we are one step from it, why dont't we make it work?

@Chocobo1
Member
Chocobo1 commented May 12, 2016 edited

@evsh, the user downloads the torrent to removable disk beside portable qBittorrent, and he continues to do this on another computer - it's real use case of portable app?

Yes.
This usage is interesting but I think its optional not mandatory.

IMO, if an app can be moved to anywhere without losing its settings, it can be called a portable app. This doesn't imply/require the artifacts that the app produces is also portable.

Here's another scenario I can think of: qbt portable was in dir C:\qbt and seed torrents in C:\seed & e:\, now the user move portable dir to d:\qbt and relaunch, the user might expect the seeding continue to work.
In this scenario, it might be best to use absolute path.

@glassez
Contributor
glassez commented May 12, 2016

The alternative is to make path relative only if it is inside of profile_dir

It is a good idea.
Then we can just set profile_dir as working directory when the application is run, if --portable is set.

@evsh
Contributor
evsh commented May 12, 2016

But that rules out interesting cases with qbt on an external drive, where it is not convenient to download inside of the qbt directory. I incline towards the first option (relative path if on the same drive).

@glassez
Contributor
glassez commented May 12, 2016

I incline towards the first option (relative path if on the same drive).

But only if --portable parameter is set.
But we could offer the user to make a choice when adding a torrent (in portable mode).

@glassez
Contributor
glassez commented May 12, 2016 edited

@evsh when you're done most of the work here, check and fix coding style. The main mistakes that I noticed a cursory glance, is naming (namespace names must start with Capital letter) and statement breaks (operators must placed not at the end of the line and at the beginning of the next one).

@evsh
Contributor
evsh commented May 12, 2016

But only if --portable parameter is set.

@glassez why? what shortcomings do you see?

@glassez
Contributor
glassez commented May 12, 2016

@glassez why? what shortcomings do you see?

I just don't want qBittorrent was doing some extra work in the normal (non-portable) mode.

@evsh
Contributor
evsh commented May 12, 2016

We can add two methods

QString toPortablePath(const QString &absolutePath);
QString fromPortablePath(const QString &portablePath);

to the Profile class (see the updated diffs). Those in the Impl::DefaultProfile will do nothing.

@glassez
Contributor
glassez commented May 12, 2016

They should do nothing even if we set --profile but not --portable.

@evsh
Contributor
evsh commented May 12, 2016

Disagree with you: current meanings of --portable and --profile are identical.

@glassez
Contributor
glassez commented May 12, 2016

current meanings of --portable and --profile are identical.

Really?

Option --portable is a shortcut for --profile=/profile

They are identical in only one case. And the rest?
When I want to use a different profile (e.g. for testing), it does not mean that I want to enable portable mode.

@evsh
Contributor
evsh commented May 12, 2016 edited

Really?

Yes, they both set profile directory.

They are identical in only one case. And the rest?
When I want to use a different profile (e.g. for testing), it does not mean that I want to enable portable mode.

For testing you may use different configurations. I'd better add an option --no-portable-fastresume, because otherwise most of the users of portable mode would need to pass two options. While a developer could use the second command line argument/environment variable.

@evsh
Contributor
evsh commented May 12, 2016

Or, we add --portable-fastresume and --portable implies --profile=x and --portable-fastresume.

@glassez glassez commented on the diff May 12, 2016
src/app/options.h
- }
+ QBtCommandLineParameters(const QProcessEnvironment&);
@glassez
glassez May 12, 2016 Contributor

Maybe remove QProcessEnvironment from constructor params and us QProcessEnvironment::systemEnvironment() in constructor body? You don't like yourself the extra things in the interface!

@evsh
evsh May 12, 2016 Contributor

I wanted to point out that default values are loaded from environment.

@glassez glassez commented on the diff May 12, 2016
src/app/options.h
-#ifdef Q_OS_WIN
- QVariant value(const QString & key, const QVariant &defaultValue = QVariant()) const {
- QString key_tmp(key);
- QVariant ret = QSettings::value(key_tmp);
- if (ret.isNull())
- return defaultValue;
- return ret;
- }
+class CommandLineParameterError: public std::runtime_error
+{
+public:
+ CommandLineParameterError(const QString &messageForUser);
+ const QString& messageForUser() const;
@glassez
glassez May 12, 2016 Contributor

There are still some messages? Why such verbosity? Why not just message?

@evsh
evsh May 12, 2016 Contributor

Usually, exception texts are for developers, that is why.

@glassez
Contributor
glassez commented May 12, 2016

Yes, they both set profile directory.

But only one set portable mode!

For testing you may use different configurations. I'd better add an option --no-portable-fastresume, because otherwise most of the users of portable mode would need to pass two options. While a developer could use the second command line argument/environment variable.
Or, we add --portable-fastresume and --portable implies --profile=x and --portable-fastresume.

What is all this? Why you need to have a separate --portable-fastresume?

otherwise most of the users of portable mode would need to pass two options

Why? They should pass only --portable. Other (normal mode users) may pass --profile=<profile_path>. And that's all!

@evsh
Contributor
evsh commented May 12, 2016

Why? They should pass only --portable.

Because portable mode user may want to keep the qBt profile not in the default location but somewhere else.

@glassez
Contributor
glassez commented May 12, 2016

Because portable mode user may want to keep the qBt profile not in the default location but somewhere else.

Ok. Then you just should allow to set --profile' together with--portable`.
But, IMO, if you go on about all the desires of the user, nothing good happens.

@evsh
Contributor
evsh commented May 12, 2016

A few moments ago you've pointed out that profile directory and relative paths in the fastresume files are independent options. Why don't we associate each with a command line parameter then? That's all. No more variations, no more desires. That would be logical, IMO. And then we introduce the --portable switch to give reasonable defaults for portable configurations.

@glassez
Contributor
glassez commented May 12, 2016

And then we introduce lot of possible undefined behaviors!

A few moments ago you've pointed out that profile directory and relative paths in the fastresume files are independent options.

Yes. But "relative paths in the fastresume files" is not separate option, it's just attribute of "portable mode" option.

@evsh
Contributor
evsh commented May 12, 2016 edited

And then we introduce lot of possible undefined behaviors!

Namely?

Yes. But "relative paths in the fastresume files" is not separate option, it's just attribute of "portable mode" option.

I begin to suspect that the name --portable for the command line switch is wrong :) "relative paths in the profile directory" is the attribute of the portable mode as well.

@glassez glassez and 1 other commented on an outdated diff May 14, 2016
src/app/application.cpp
{
+ validateCommandLineParameters();
+
+ setApplicationName("qBittorrent");
+
+ QString profileDir = m_commandLineArgs.portableMode ?
@glassez
glassez May 14, 2016 edited Contributor

Fix coding style:

QString profileDir = m_commandLineArgs.portableMode
        ? QDir(QCoreApplication::applicationDirPath()).absoluteFilePath(QLatin1String("profile"))
        : m_commandLineArgs.profileDir;
@evsh
evsh May 14, 2016 Contributor

Are you thinking that '?' on the next line adds readability?

@glassez
glassez May 14, 2016 Contributor

I am thinking we should following the coding guidelines.
I am thinking the code uniformity adds readability.

@glassez glassez commented on an outdated diff May 14, 2016
src/app/application.cpp
@@ -153,7 +173,7 @@ void Application::setFileLoggerEnabled(bool value)
QString Application::fileLoggerPath() const
{
- return settings()->loadValue(KEY_FILELOGGER_PATH, QVariant(Utils::Fs::QDesktopServicesDataLocation() + LOG_FOLDER)).toString();
+ return settings()->loadValue(KEY_FILELOGGER_PATH, QVariant(specialFolderLocation(Utils::Environment::SpecialFolder::Data) + LOG_FOLDER)).toString();
@glassez
glassez May 14, 2016 Contributor

It's better to split such long lines.

@glassez glassez commented on an outdated diff May 14, 2016
src/app/options.cpp
@@ -0,0 +1,394 @@
+/*
+ * Bittorrent Client using Qt and libtorrent.
+ * Copyright (C) 2014 Vladimir Golovnev <glassez@yandex.ru>
+ * Copyright (C) 2006 Christophe Dumez
+ * 2016 Eugene Shalygin
@glassez
glassez May 14, 2016 Contributor

Why do you love to violate the order? The copyright notice should be placed in reverse chronological order and be as follows:

Copyright (C) 2016  Eugene Shalygin <your_email>

You can see how it's done!

@glassez glassez commented on an outdated diff May 14, 2016
src/app/options.cpp
+ return QLatin1String("--") + QLatin1String(m_name);
+ }
+
+ QString shortcutParameter() const
+ {
+ return QLatin1String("-") + QLatin1Char(m_shortcut);
+ }
+
+ bool hasShortcut() const
+ {
+ return m_shortcut != 0;
+ }
+
+ QString envVarName() const
+ {
+ return QLatin1String("QBT_") +
@glassez
glassez May 14, 2016 Contributor

Fix coding style!

@glassez glassez commented on the diff May 14, 2016
src/app/options.cpp
+
+ bool value(const QProcessEnvironment &env) const
+ {
+ QString val = env.value(envVarName());
+ // we accept "1" and "true" (upper or lower cased) as boolean 'true' values
+ return (val == QLatin1String("1") || val.toUpper() == QLatin1String("TRUE"));
+ }
+ };
+
+ inline bool operator == (const QString &s, const BoolOption &o)
+ {
+ return o == s;
+ }
+
+ // Option with string value. May not have a shortcut
+ struct StringOption: protected Option
@glassez
glassez May 14, 2016 Contributor

Why protected?

@evsh
evsh May 14, 2016 Contributor

BoolOption is an exception (with its public inheritance). But probably I just need to move Option::usage() from Option to BoolOption.

@evsh
evsh May 17, 2016 Contributor

Done.

@glassez glassez commented on an outdated diff May 14, 2016
src/app/options.cpp
+ }
+
+ QString usage(const QString &valueName) const
+ {
+ return padUsageText(parameterAssignment() + QLatin1Char('<') + valueName + QLatin1Char('>'));
+ }
+
+ private:
+ QString parameterAssignment() const
+ {
+ return fullParameter() + QLatin1Char('=');
+ }
+
+ static bool isStringQuoted(const QString &s, QChar quoteChar)
+ {
+ return s.size() >= 2 && (s.startsWith(quoteChar) && s.endsWith(quoteChar));
@glassez
glassez May 14, 2016 Contributor

Please add parenthesis around s.size() >= 2 too.

@glassez glassez commented on an outdated diff May 14, 2016
src/app/options.cpp
+#endif
+ , webUiPort(WEBUI_PORT_OPTION.value(env, -1))
+ , profileDir(PROFILE_OPTION.value(env))
+ , portableMode(PORTABLE_OPTION.value(env))
+ , configurationName(CONFIGURATION_OPTION.value(env))
+{
+}
+
+QBtCommandLineParameters parseCommandLine(const QStringList &args)
+{
+ QBtCommandLineParameters result {QProcessEnvironment::systemEnvironment()};
+
+ for (int i = 1; i < args.count(); ++i) {
+ const QString &arg = args[i];
+
+ if ((arg.startsWith("--") && !arg.endsWith(".torrent")) ||
@glassez
glassez May 14, 2016 Contributor

Fix coding style!

@glassez glassez commented on an outdated diff May 14, 2016
src/app/options.cpp
+ const QString &arg = args[i];
+
+ if ((arg.startsWith("--") && !arg.endsWith(".torrent")) ||
+ (arg.startsWith("-") && (arg.size() == 2))) {
+ // Parse known parameters
+ if ((arg == SHOW_HELP_OPTION)) {
+ result.showHelp = true;
+ }
+#ifndef Q_OS_WIN
+ else if (arg == SHOW_VERSION_OPTION) {
+ result.showVersion = true;
+ }
+#endif
+ else if (arg == WEBUI_PORT_OPTION) {
+ result.webUiPort = WEBUI_PORT_OPTION.value(arg);
+ if (( result.webUiPort < 1) || ( result.webUiPort > 65535) )
@glassez
glassez May 14, 2016 Contributor

Fix coding style!

@glassez glassez commented on an outdated diff May 14, 2016
src/app/options.cpp
+
+ return result;
+}
+
+CommandLineParameterError::CommandLineParameterError(const QString &messageForUser)
+ : std::runtime_error(messageForUser.toLocal8Bit().data())
+ , m_messageForUser(messageForUser)
+{
+}
+
+const QString& CommandLineParameterError::messageForUser() const
+{
+ return m_messageForUser;
+}
+
+QString makeUsage(const QString &prg_name)
@glassez
glassez May 14, 2016 Contributor

Please fix param name (use camelCase).
I understand that most likely it got you inherited, but I hope it's not too much trouble you.

@glassez glassez and 1 other commented on an outdated diff May 14, 2016
src/app/options.h
-public:
- QIniSettings(const QString &organization = "qBittorrent", const QString &application = "qBittorrent", QObject *parent = 0 ):
-#ifdef Q_OS_WIN
- QSettings(QSettings::IniFormat, QSettings::UserScope, organization, application, parent)
+class QProcessEnvironment;
@glassez
glassez May 14, 2016 Contributor

Insert blank line after.

@evsh
evsh May 17, 2016 Contributor

Done.

@glassez glassez commented on an outdated diff May 14, 2016
src/base/utils/environment.cpp
+ * modified versions of it that use the same license as the "OpenSSL" library),
+ * and distribute the linked executables. You must obey the GNU General Public
+ * License in all respects for all of the code used other than "OpenSSL". If you
+ * modify file(s), you may extend this exception to your version of the file(s),
+ * but you are not obligated to do so. If you do not wish to do so, delete this
+ * exception statement from your version.
+ *
+ */
+
+#include "environment.h"
+
+#include <QCoreApplication>
+
+#include "environment_p.h"
+
+Utils::Environment::Profile*Utils::Environment::Profile::m_instance = nullptr;
@glassez
glassez May 14, 2016 Contributor

Add space before *.

@glassez glassez commented on an outdated diff May 14, 2016
src/base/utils/environment.cpp
+ * License in all respects for all of the code used other than "OpenSSL". If you
+ * modify file(s), you may extend this exception to your version of the file(s),
+ * but you are not obligated to do so. If you do not wish to do so, delete this
+ * exception statement from your version.
+ *
+ */
+
+#include "environment.h"
+
+#include <QCoreApplication>
+
+#include "environment_p.h"
+
+Utils::Environment::Profile*Utils::Environment::Profile::m_instance = nullptr;
+
+Utils::Environment::Profile::Profile(Impl::Profile *impl)
@glassez
glassez May 14, 2016 Contributor

This file is an implementation of the classes in the Utils::Environment namespace. To use using namespace Utils::Environment is a good idea here.

@glassez glassez and 1 other commented on an outdated diff May 14, 2016
src/base/utils/environment.h
+ {
+ public:
+ QString location(SpecialFolder folder) const;
+ SettingsPtr applicationSettings(const QString &name) const;
+
+ /// Returns either default name for configuration file (QCoreApplication::applicationName())
+ /// or the value, supplied via parameters
+ QString configurationName() const;
+
+ static const Profile& instance();
+
+ private:
+ Profile(Impl::Profile *impl);
+ ~Profile();
+
+ friend class ::Application;
@glassez
glassez May 14, 2016 Contributor

Base MUST be independent from App, GUI etc.

@evsh
evsh May 14, 2016 Contributor

Who then can initialize this singleton, if not Application?

@glassez
glassez May 14, 2016 Contributor

Application initializes (almost) all current singletons, but they are independent from it. Just do initialize() public.

@evsh
evsh May 14, 2016 Contributor

Bad idea. IMO, a friend declaration, in this case, is not a dependence. Consider the following example:

class A {
private:
   friend int main(int, char**);
   static void initialize();
};

Does A depend on main()? No, it utilises the idea that there has to be the main() function and only it can initialise the class. The same is here: everyone knows that there has to be an Application class in Qt application.

@glassez
glassez May 14, 2016 Contributor

Okay, probably you're right here...

@evsh
evsh May 14, 2016 Contributor

Glad that you agree.

@glassez glassez commented on an outdated diff May 14, 2016
src/base/utils/environment_p.cpp
+ }
+
+ return save_path;
+#endif
+
+#if defined(Q_OS_MAC)
+ // TODO: How to support this on Mac OS?
+#endif
+
+ // Fallback
+ return QDir::home().absoluteFilePath(QCoreApplication::translate("fsutils", "Downloads"));
+#endif
+}
+
+Utils::Environment::SettingsPtr
+Utils::Environment::Impl::DefaultProfile::applicationSettings(const QString &name) const
@glassez
glassez May 14, 2016 Contributor

As I mentioned above, better to remove Utils::Environment and Utils::Environment::Impl from these names, than so to be perverted here.

@glassez glassez commented on an outdated diff May 14, 2016
src/base/utils/environment_p.cpp
+#endif
+}
+
+Utils::Environment::SettingsPtr
+Utils::Environment::Impl::DefaultProfile::applicationSettings(const QString &name) const
+{
+#ifdef Q_OS_WIN
+ return SettingsPtr(new QSettings(QSettings::IniFormat, QSettings::UserScope, configurationName(), name));
+#else
+ return SettingsPtr(new QSettings(configurationName(), name));
+#endif
+}
+
+Utils::Environment::Impl::CustomProfile::CustomProfile(const QString &rootPath, const QString &configurationName)
+ : Profile(configurationName)
+ , m_rootDirectory {QDir(rootPath).absoluteFilePath(this->configurationName())}
@glassez
glassez May 14, 2016 Contributor

Please don't mix initializer styles.

@glassez glassez and 1 other commented on an outdated diff May 14, 2016
src/base/utils/environment_p.h
+ class Profile
+ {
+ public:
+ virtual QString baseDirectory() const = 0;
+ virtual QString cacheLocation() const = 0;
+ virtual QString configLocation() const = 0;
+ virtual QString dataLocation() const = 0;
+ virtual QString downloadLocation() const = 0;
+ virtual SettingsPtr applicationSettings(const QString &name) const = 0;
+
+ virtual ~Profile() = default;
+
+ QString configurationName() const;
+
+ protected:
+ Profile(const QString &configurationName);
@glassez
glassez May 14, 2016 Contributor

As Profile is abstract class you may have public constructor here.

@evsh
evsh May 14, 2016 Contributor

I might misunderstand you, but there is no public constructor because this is an abstract class.

@glassez
glassez May 14, 2016 Contributor

I mean you can have public constructor in abstract class. No need extra (protected) section here.

@evsh
evsh May 14, 2016 Contributor

You probably mean that it is impossible to create and instance with pure virtual functions anyway. IMO, protected constructor in such case is better for documentation/hint reasons.

@glassez
glassez May 14, 2016 Contributor

IMO, protected constructor in such case is better for documentation/hint reasons.

Okay, if so.

@glassez glassez commented on an outdated diff May 14, 2016
src/base/utils/fs.cpp
#endif
-#ifndef QBT_USES_QT5
-
-#ifndef DISABLE_GUI
-#include <QDesktopServices>
-#endif
-
-#else
-#include <QStandardPaths>
-#endif
-
-
-#include "misc.h"
-#include "fs.h"
+#include "environment.h"
@glassez
glassez May 14, 2016 Contributor

This isn't needed.

@evsh evsh commented on an outdated diff May 14, 2016
src/base/bittorrent/session.cpp
@@ -2686,7 +2687,8 @@ namespace
libt::lazy_bdecode(data.constData(), data.constData() + data.size(), fast, ec);
if (ec || (fast.type() != libt::lazy_entry::dict_t)) return false;
- torrentData.savePath = Utils::Fs::fromNativePath(Utils::String::fromStdString(fast.dict_find_string_value("qBt-savePath")));
+ torrentData.savePath = Utils::Environment::Profile::instance().fromPortablePath(
@evsh
evsh May 14, 2016 Contributor

@glassez: could you check these changes, please?

@evsh evsh commented on an outdated diff May 14, 2016
src/base/bittorrent/torrenthandle.cpp
@@ -1531,7 +1532,13 @@ void TorrentHandle::handleSaveResumeDataAlert(libtorrent::save_resume_data_alert
resumeData["qBt-paused"] = isPaused();
resumeData["qBt-forced"] = isForced();
}
- resumeData["qBt-savePath"] = m_useASM ? "" : Utils::String::toStdString(m_savePath);
+ else {
@evsh
evsh May 14, 2016 Contributor

@glassez: and here too, please.

@Chocobo1 Chocobo1 and 1 other commented on an outdated diff May 14, 2016
src/base/utils/environment_p.cpp
+Utils::Environment::Impl::Converter::Converter(const QString &basePath)
+ : m_baseDir {basePath}
+{
+ m_baseDir.makeAbsolute();
+}
+
+QString Utils::Environment::Impl::Converter::toPortablePath(const QString &path) const
+{
+ if (path.isEmpty() || m_baseDir.path().isEmpty())
+ return path;
+
+#ifdef Q_OS_WIN
+ if (QDir::isAbsolutePath(path)) {
+ QChar driveLeter = path[0].toUpper();
+ QChar baseDriveLetter = m_baseDir.path()[0].toUpper();
+ bool onSameDrive = (driveLeter.category() == QChar::Letter_Uppercase) && (driveLeter == baseDriveLetter);
@Chocobo1
Chocobo1 May 14, 2016 edited Member

I wasn't going to question it anymore, but I can't hold myself :P
Please, just for once; could you explain (convince) me that this implementation will work fine in my scenario in: #5214 (comment)

I believe that is a very common usage scenario.
Or let me know why my scenario might be invalid.

@evsh
evsh May 14, 2016 Contributor

I guess you imply --portable switch set. Things will not work in this scenario (obviously). That is why we split the portable mode onto two options.

@Chocobo1
Chocobo1 May 15, 2016 Member

That is why we split the portable mode onto two options.

I am confused, how do --configuration & --profile help for this scenario?

@evsh
evsh May 15, 2016 edited Contributor

There are four options added by this PR. Two of them turn on independent features of the portable mode: --profile and --relative-fastresume. There is one option for configurations (--configuration) and there is a shortcut--profilewhich is equal to----profile=$EXE_DIR/profileand--elative-fastresume`.

For the use-case noted by @chocobo1 one needs just --profile option. Those who use a portable drive for qbt and downloads need both options.

@Chocobo1
Member

Compiled & tested, the command line switches works fine!

Some minor document issues:

  1. in command line help, the Options list should have sorted alphabetically.
  2. For example, to disable the splash screen: QBT_NO_SPLASH=1 qbittorrent, the last qbittorrent should be replaced by argv[0].
  3. Bad command line: Portable mode and explicit profile directory options are mutually exclusive, the incompatibility of 2 options should have been mentioned under --portable or --profile point.
  4. Bad command line: Portable mode and explicit profile directory options are mutually exclusive, here the Portable mode & explicit profile are mentioned and so command line help should also mention it.
@glassez glassez commented on an outdated diff May 15, 2016
src/app/options.cpp
@@ -244,6 +244,7 @@ namespace
constexpr const BoolOption NO_SPLASH_OPTION = {"no-splash"};
constexpr const IntOption WEBUI_PORT_OPTION = {"webui-port"};
constexpr const StringOption PROFILE_OPTION = {"profile"};
+ constexpr const BoolOption RELATIVE_FASTRESUME = {"relative-fastresume"};
@glassez
glassez May 15, 2016 Contributor

IMO, "relative fastresume" is wrong wording.

@glassez
Contributor
glassez commented May 15, 2016

I don't like this idea of portable mode. There are too many contradictions. Or it is necessary to apply some restrictions to make it consistent (for example, allow to store portable torrents only in a single directory, specified explicitly or by default in the subdirectory of the profile, etc.).

@glassez
Contributor
glassez commented May 15, 2016

Frankly, the use applications of this type in portable mode is generally suspicious for me.

@Chocobo1
Member

allow to store portable torrents only in a single directory, specified explicitly or by default in the subdirectory of the profile, etc

+1, thought about this. For example a folder called "Download" located at the same folder as qbt.exe or in the Profile folder is good and easy for users to follow.

@evsh
Contributor
evsh commented May 15, 2016

allow to store portable torrents only in a single directory, specified explicitly or by default in the subdirectory of the profile, etc

+1, thought about this. For example a folder called "Download" located at the same folder as qbt.exe or in the Profile folder is good and easy for users to follow.

Can not understand your concerns, but want to note that, IMO, users with portable drivers most likely would prefer to keep the qBt in a subfolder, because they use their drives for, well... portable data and qBt may be just a tool, but not a ding an sich.

@sledgehammer999
Contributor

I tried to follow the out-of-code comments and I hope I didn't miss something.

  1. There shouldn't be 2 switches for setting the profile dir. Only one. If the user chooses to override the profile path then he should specify the exact path(absolute or relative to current path).
  2. An interesting question was "What happens if user downloads something in computer A and moves to continue downloading in computer B"? And something about relative paths. Maybe we should investigate what other clients do(if they have portable mode). Saving as relative path should be done if the files are saved on the same media as the qbt binary. But this is difficult to detect on linux, right?

I haven't looked at the code, so what is the current situation on the above items? Have you come on a solution? Or is this too hard to implement correctly? I wouldn't mind having the option to only set the profile dir without any other assurances.

@glassez
Contributor
glassez commented May 16, 2016

@evsh "to store portable torrents only in a single directory" is main idea of my comment. The second thing is "specified explicitly or by default in the subdirectory of the profile" so I think it covers your case.
Also we should disable incomplete folder for portable torrents and probably apply some other restrictions.

@glassez
Contributor
glassez commented May 16, 2016

I wouldn't mind having the option to only set the profile dir without any other assurances.

+1
Since portable mode is more complex task and it requires very serious consideration to do it correctly.

@evsh
Contributor
evsh commented May 16, 2016
  1. There shouldn't be 2 switches for setting the profile dir. Only one. If the user chooses to override the profile path then he should specify the exact path(absolute or relative to current path).

There is only one.

An interesting question was "What happens if user downloads something in computer A and moves to continue downloading in computer B"?...

On Windows relative paths are written into the .fastresume files if the profile dir and the save path is on the same drive and always on the other platforms.

Also we should disable incomplete folder for portable torrents and probably apply some other restrictions.

Why not require it to be inside of the profile dir?

@evsh
Contributor
evsh commented May 17, 2016

PR update addressing the raised questions and comments.

Perhaps we should split it and merge the first five commits and think about the portable mode and fastresume later? I very much want to get configurations support for debugging purposes.

@glassez glassez commented on an outdated diff May 18, 2016
src/app/options.cpp
+#include <QFileInfo>
+#include <QProcessEnvironment>
+#include <QTextStream>
+
+#ifdef Q_OS_WIN
+#include <QMessageBox>
+#endif
+
+#include "base/utils/misc.h"
+
+namespace
+{
+ // Base option class. Encapsulates name operations.
+ class Option
+ {
+
@glassez
glassez May 18, 2016 Contributor

Remove this blank line.

@glassez glassez commented on the diff May 18, 2016
src/app/options.cpp
+namespace
+{
+ // Base option class. Encapsulates name operations.
+ class Option
+ {
+
+ protected:
+ constexpr Option(const char *name, char shortcut = 0)
+ : m_name {name}
+ , m_shortcut {shortcut}
+ {
+ }
+
+ QString fullParameter() const
+ {
+ return QLatin1String("--") + QLatin1String(m_name);
@glassez
glassez May 18, 2016 Contributor

You seem to misunderstand the QLatin1String meaning, once you use it for the wrong purpose. Re-read the documentation.

@glassez
glassez May 18, 2016 Contributor

This applies not only to this line.

@evsh
evsh May 18, 2016 Contributor

Re-read the documentation.

Done. Your turn :)

@glassez
glassez May 19, 2016 Contributor

Ok. Let's go.

Many of QString's member functions are overloaded to accept const char * instead of QString. [...] These functions are usually optimized to avoid constructing a QString object for the const char * data.

Applications that define QT_NO_CAST_FROM_ASCII don't have access to QString's const char * API. To provide an efficient way of specifying constant Latin-1 strings, Qt provides the QLatin1String, which is just a very thin wrapper around a const char *.

Note: If the function you're calling with a QLatin1String argument isn't actually overloaded to take QLatin1String, the implicit conversion to QString will trigger a memory allocation, which is usually what you want to avoid by using QLatin1String in the first place. In those cases, using QStringLiteral may be the better option.

@evsh
evsh May 19, 2016 Contributor

Sorry, I've told you already that I did read the documentation. You see, I don't understand what is wrong. Either explain that or give a hint suggesting the right code fragment, please.

@glassez
glassez May 19, 2016 Contributor

Sorry, I've told you already that I did read the documentation. You see, I don't understand what is wrong. Either explain that

Since we don't use QT_NO_CAST_FROM_ASCII using QLatin1String in our code is at least useless (really this leads to unnecessary cost).

or give a hint suggesting the right code fragment, please

Please. For this particular case:

const QString DDASH("--");

// ...

QString fullParameter() const
{
    return DDASH + m_name;
}
@glassez
glassez May 19, 2016 edited Contributor

For other case:

bool value(const QProcessEnvironment &env) const
{
    QString val = env.value(envVarName());
    // we accept "1" and "true" (upper or lower cased) as boolean 'true' values
     return ((val == "1") || (val.toUpper() == "TRUE"));
}
@glassez
glassez May 19, 2016 Contributor

Some other case:

QString envVarName() const
{
    return QString("QBT_") + QString(m_name).toUpper().replace('-', '_');
}
@evsh
evsh May 19, 2016 edited Contributor

Your code has the following shortcomings:

  1. Global QString constants lead to QString constructor calls at module loading time. Static variables in functions would be a bit better.
  2. Both local static and global QString variables need twice the memory comparing to char arrays (permanent usage, is never freed).
  3. An expression like QString("a literal") requires more computing resources as comparing to QLatin1String("the same literal") because UTF-8 to UTF-16 decoder is more complex than ASCII to UTF-16.

And what are the advantages?

Since we don't use QT_NO_CAST_FROM_ASCII using QLatin1String in our code is at least useless (really this leads to unnecessary cost).

What costs?

@glassez
glassez May 19, 2016 Contributor

Global QString constants lead to QString constructor calls at module loading time. Static variables in functions would be a bit better.

I don't mean global constant. I mean some constant, for example class-wide.

Both local static and global QString variables need twice the memory comparing to char arrays (permanent usage, is never freed).

If you feel so sorry for the two bytes, you can do so:

QString fullParameter() const
{
    return QString("--") + m_name;
}

An expression like QString("a literal") requires more computing resources as comparing to QLatin1String("the same literal") because UTF-8 to UTF-16 decoder is more complex than ASCII to UTF-16.

Agree. But you don't understand the following:
QString("str1") + "str2" = 2 QString ctor + 1 operator+
QLatin1String("str1") + QLatin1String("str2") = 2 QLatin1String ctor + 2 QString ctor + 1 operator+

What costs?

  1. Performance: additional (and redundant) QLatin1String constructor calls
  2. Visual: additional (and redundant) QLatin1String in code
@evsh
evsh May 19, 2016 Contributor

The thing is QLatin1String ctor + QString ctor is not more work than QString ctor(char literal), because we remove information about encoding. I suggest you read QLatin1String class definition, it is quite short. And the general note: it is usually not possible to make something more optimal removing the information, but the inverse case (more optimal with additional information) is more likely. I bet the compiler generates not significantly different code for QString::fromLatin1("a literal") and QString(QLatin1String("a literal")).

@glassez
glassez May 20, 2016 edited Contributor

Damn. Unfortunately Qt5 documentation is outdated and contains incorrect and conflicting information. It turns out that QString's const char * API is no longer so lightweight and optimized as described.

@evsh
evsh May 20, 2016 Contributor

I don't know what you are talking about but they say in the documentation that setCodecForCString() was removed and all const char* strings are assumed to be in UTF-8. Do they hide anything else?

@glassez
glassez May 20, 2016 Contributor

But the following is wrong:

Many of QString's member functions are overloaded to accept const char * instead of QString. [...] These functions are usually optimized to avoid constructing a QString object for the const char * data.

@glassez glassez commented on an outdated diff May 18, 2016
src/app/options.cpp
+
+ private:
+ const char *m_name;
+ const char m_shortcut;
+ };
+
+ // Boolean option.
+ class BoolOption: protected Option
+ {
+ public:
+ constexpr BoolOption(const char *name, char shortcut = 0)
+ : Option {name, shortcut}
+ {
+ }
+
+ bool operator == (const QString &arg) const
@glassez
glassez May 18, 2016 Contributor

The same issue with method names as in another PR.

@glassez glassez commented on the diff May 18, 2016
src/app/options.cpp
+ int value(const QString &arg) const
+ {
+ QString val = StringOption::value(arg);
+ bool ok = false;
+ int res = val.toInt(&ok);
+ if (!ok)
+ throw CommandLineParameterError(QObject::tr("Parameter '%1' must follow syntax '%1=%2'",
+ "e.g. Parameter '--webui-port' must follow syntax '--webui-port=<value>'")
+ .arg(fullParameter()).arg(QLatin1String("<integer value>")));
+ return res;
+ }
+
+ int value(const QProcessEnvironment &env, int defaultValue) const
+ {
+ QString val = env.value(envVarName());
+ if (val.isEmpty()) return defaultValue;
@glassez
glassez May 18, 2016 Contributor

It's better to have blank line after earlier return.

@evsh
evsh May 20, 2016 Contributor

Done.

@glassez glassez and 1 other commented on an outdated diff May 18, 2016
src/base/utils/environment.h
+ {
+ class Profile;
+ class PathConverter;
+ }
+
+ using SettingsPtr = std::unique_ptr<QSettings>;
+
+ enum class SpecialFolder
+ {
+ Cache,
+ Config,
+ Data,
+ Downloads
+ };
+
+ class Profile
@glassez
glassez May 18, 2016 Contributor

When I suggested Utils::Environment, I proceeded from the sense of the old functions. Environment is something external to the application, but the Profile is not. So you shouldn't put it to Utils::Environment namespace. Let it be in global namespace and its files in base/ directly.

@evsh
evsh May 18, 2016 Contributor

While I certainly agree with you that the code does not fall exactly into the "environment" group, it makes an individual unit and I think it would be nice to highlight that by using a proper place in the files hierarchy. Can not find a word right away.

@evsh
evsh May 19, 2016 Contributor

Sorry, @LordNyriox, those are good names for the entity which is currently named class Profile, but we are looking for a name for the namespace (and the corresponding directory), which contains this entity, its various implementations and other things which control how the qBt is localised within a filesystem at run-time.

@evsh
evsh May 21, 2016 Contributor

With this PR we are adding options to control were qBt stores its configuration and data files (like the fast resume files or the RSS cache) and we called this entity "Profile". Thank you for the willingness to help.

@glassez
glassez May 23, 2016 Contributor

@LordNyriox Profile is usual name for such a thing. Why do you propose changing it?

@glassez
glassez May 23, 2016 Contributor

Things under Utils namespace are helpers, wrappers (and so on) for different purposes. But Profile is more important, so I see it as the application component (albeit small). Therefore, I propose to put it in the global namespace.

@evsh
evsh May 24, 2016 Contributor

Maybe it will be better to leave implementation classes in namespace Impl? Perhaps we can create base/private or base/impl directory and put those files there?

@glassez
glassez May 24, 2016 Contributor

Agree.
base/private and Private namespace (as done in BitTorrent).

@evsh
Contributor
evsh commented May 24, 2016

PR updated: moved the Profile class into the global namespace, implementation classes into namespace Private. Moved files to base/ and base/private/ respectively.

@evsh
Contributor
evsh commented Jun 20, 2016

@qbittorrent/qbittorrent-frequent-contributors:
I really-really want this to be merged (at least partially). This would make debugging so much simpler! What can I do to make it happen?

@glassez
Contributor
glassez commented Jun 21, 2016

For me, there is no portable mode should be here. I'm really not sure it's implemented correctly now. It requires more thinking about so it's another task.

@evsh
Contributor
evsh commented Jun 21, 2016

@glassez, thank you for the opinion, but this is not an answer to my question. Let me remind the initial motivation: to make development easier it makes sense to load different list of torrents in the debugging mode than that of a regular run.
Attempts to implement a full-fledged portable mode did not only improved the initial implementation, but also raised requirements quite high. And the latter complicates any efforts to merge this code into the master branch. That is why my questions is: what can be done to merge at least the configurations support?

@glassez
Contributor
glassez commented Jun 21, 2016

@evsh I don't understand what does not suit you in my answer... You say that "portable mode" prevents merging this request:

Attempts to implement a full-fledged portable mode did not only improved the initial implementation, but also raised requirements quite high. And the latter complicates any efforts to merge this code into the master branch.

So you just need to remove it from here and leave only "configuration" part.

@Owyn
Owyn commented Nov 13, 2016

command line

so no support for file association?

I mean when you associate .torrent file with qbittorrent - it won't use any command lines switches, would it?

@evsh
Contributor
evsh commented Dec 17, 2016

@sledgehammer999: would you accept this PR without the very last commit? Or, better, with it, please :)

@evsh
Contributor
evsh commented Dec 30, 2016
evsh added some commits May 3, 2016
@evsh evsh Add support for different configurations. Partially closes #465
It may be useful to have different configurations either for portable
versions or for debugging purposes. To implement this we add two
options, avaliable via command line switches
1. An option to change configuration name ("--configuration"). The name
supplied via this option is appended to
QCoreApplication::applicationName() to form "qBittorrent_<conf_name>"
name for the configuration files.
2. An option to provide a path do directory where all the settings are
stored (kind of profile directory). There is a shortcut "--portable"
which means "use directory 'profile' near the executable location".

In order to implement that we have to perform initialisation of the
profile directories before the SettingStorage and Preferences singletones
are initialised. Thus, options parsing shall be performed without defaults
read from preferences.
95e8ab6
@evsh evsh Initialise QBtCommandLineParameters members from environment
This allows to pass options via environment variables. The variable name
is constructed from parameter name by transforming the name to upper
case and prefixing "QBT_".
0e672e9
@evsh evsh Add environment variables usage description to the help text 314b469
@evsh evsh Replace wrappers in base/utils/fs.h with Profile::SpecialFolders::loc…
…ation()
8c9005f
@evsh evsh Refactor parameters parsing
Introduce classes that encapsulate parameter names and parsing schemes
from command line and from environment variables.
7a6f54e
@evsh evsh Save relative paths in fastresume files
Conditionally change absolute paths to relative in the fastresume data files.
The condition is specified by user via a command line parameter and
paths are relative to the profile dir.

On Windows the convertion to relative path is performed if the path and
the profile are on the same drive only.
1d1e675
@evsh
Contributor
evsh commented Jan 19, 2017

@sledgehammer999: ping? Don't you agree that profiles would make debugging so much simpler?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment