refactor_useQCommandLineParser #2135

AHeimberger · 2020-06-29T14:30:04Z

Hey Guys, maybe you like this refactoring. It comes with a small issue. In <Windows e.g. 10> the window for showing the help information does not look identically. However I prefer using Qt-Mechanism instead of parsing it free will. Furthermore, if you say it's not direcly necessary to print an information in case the Config-Directory is not valid. It would allow to remove functions like showHint.

Best Regards

er-vin · 2020-06-29T17:06:47Z

Could you please satisfy the DCO bot? Also I think those three commits can be squashed together.

AHeimberger · 2020-06-29T19:43:29Z

Could you please satisfy the DCO bot? Also I think those three commits can be squashed together.

I was able to satisfy the "DCO Bot" but I never did a squash on a branch which I already pushed? Can you do a sqashed merge?

er-vin · 2020-06-30T06:00:03Z

Sure, I can do that.

AHeimberger · 2020-07-02T07:56:13Z

@er-vin Following lines in application.cpp can be removed as well:

    if (_parser.isSet("version") || _parser.isSet("help"))
        return;	        return;

QCommandLineParser will sys.exit() when asking for version and help, there is no more need for the questioning. I oversaw this, and will do this as well.

er-vin

Sorry for not getting your PR the attention it deserved last week. Was a bit busy. I like where this is going.

src/gui/application.cpp

AHeimberger · 2020-07-06T22:40:54Z

Sorry for not getting your PR the attention it deserved last week. Was a bit busy. I like where this is going.

No Problem, I saw there are many refactorings/redesigns ongoing.

er-vin · 2020-07-09T11:31:51Z

Looking good. I spotted another such hand rolled command line handling in cmd.cpp. Do you fancy porting it as well? If yes I'll wait a bit, if not I'll start reworking a bit the history in there since you said you didn't know how to squash commits.

AHeimberger · 2020-07-13T08:49:42Z

Looking good. I spotted another such hand rolled command line handling in cmd.cpp. Do you fancy porting it as well? If yes I'll wait a bit, if not I'll start reworking a bit the history in there since you said you didn't know how to squash commits.

Hmm, I have never used the cmd line application, but I was looking at the code and it seems to be feasible. @er-vin There is just one conflict. Option -h and --help is typically used to print help information. The cmd application use -h to Sync hidden files. Any prefrences for this argument? Shall it remain as is?

er-vin · 2020-07-13T09:51:47Z

Looking good. I spotted another such hand rolled command line handling in cmd.cpp. Do you fancy porting it as well? If yes I'll wait a bit, if not I'll start reworking a bit the history in there since you said you didn't know how to squash commits.

Hmm, I have never used the cmd line application, but I was looking at the code and it seems to be feasible. @er-vin There is just one conflict. Option -h and --help is typically used to print help information. The cmd application use -h to Sync hidden files. Any prefrences for this argument? Shall it remain as is?

Ideally it should remain as is because people using it in scripts should be expected.

AHeimberger · 2020-07-14T13:48:50Z

@er-vin Hey, do you really think that the option h is used? I am facing following Problem. ./src/cmd/cmd.cpp: bool ignoreHiddenFiles always remains false. Otherwise I do not understand the code. It is initialized as false and when the option (argument) is set it will stay false. I would introduce option i, ignore-hidden instead of h which also does something.

AHeimberger · 2020-07-14T19:30:56Z

@er-vin I missed to update documentation.

er-vin · 2020-07-15T05:52:53Z

@er-vin Hey, do you really think that the option h is used?

Well, you can't never know what user do, right? :-)

But it's a tool meant for scripting so I wouldn't underestimate what people do in said scripts.

I am facing following Problem. ./src/cmd/cmd.cpp: bool ignoreHiddenFiles always remains false. Otherwise I do not understand the code. It is initialized as false and when the option (argument) is set it will stay false. I would introduce option i, ignore-hidden instead of h which also does something.

Interesting, so it'd be broken today?

AHeimberger · 2020-07-15T11:12:33Z

Yes, I broke it, but "I can undo it". I thought I will break it because it has not done anything before and now it is more readable.

er-vin · 2020-07-15T13:50:36Z

Yes, I broke it, but "I can undo it". I thought I will break it because it has not done anything before and now it is more readable.

Sorry I was being unclear, by "it'd be broken today?" I meant, "it was broken before your changes?"

AHeimberger · 2020-07-16T12:20:09Z

Yeah, and I was happy to see that you fixed the path concatenation of makeDbName and something else. I was struggling with that while I was testing it. As I fixed it I saw you fixed or merged it a few days ago as well.

er-vin · 2020-09-16T16:05:54Z

/rebase

er-vin · 2021-01-07T10:04:32Z

@AHeimberger we're still interested into this, could you please try to rebase and solve the conflicts?

mgallien · 2021-04-20T09:48:05Z

@AHeimberger we are still interested into your work. Could you have a look at the comment from @er-vin ?

AHeimberger · 2021-04-25T20:20:13Z

@AHeimberger we are still interested into your work. Could you have a look at the comment from @er-vin ?

I will give it a try. Best Regards

AHeimberger · 2022-03-14T21:32:06Z

@er-vin, @mgallien please check. best regards

mgallien · 2022-07-18T10:12:18Z

@AHeimberger ervin is no longer part of the team
unfortunately I was very busy the last two weeks and could not really review your PR
will do my best this week
sorry again for the very long delay
I really look forward merging such improvements

er-vin

Thanks a lot for sticking around so long on that one. Overall I still think it can be a good improvement, I'm mostly pointing out tiny improvements here. The only think nagging me is the stability of nextcloudcmd CLI which shouldn't be underestimated.

er-vin · 2022-07-19T20:32:16Z

man/nextcloudcmd.1.rst

@@ -32,46 +32,56 @@ OPTIONS
 ``--path``
       Overrides default remote root folder to a specific subfolder on the server(e.g.: /Documents would sync the Documents subfolder on the server)

-``—user``, ``-u`` ``[user]``
+``--user``, ``-u`` ``[user]``


This makes me wonder, does -user still work? If not it might be a deal breaker for the change in nextcloudcmd since the exposed CLI can't change there, people are likely using this in scripts and so it'd break them.

we already had people having to adjust for even more minor workflow changes of nextcloudcmd
it would be best to still offer compatibility maybe with a deprecation warning (and I know that if run from a script nobody will read it)

the current code is already using --user and not -user so that should be fine to do

src/cmd/cmd.cpp

src/gui/application.cpp

codecov · 2022-08-01T10:57:07Z

Codecov Report

Merging #2135 (e13c0e9) into master (b718c7d) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2135      +/-   ##
==========================================
- Coverage   56.80%   56.69%   -0.12%     
==========================================
  Files         138      138              
  Lines       17142    17142              
==========================================
- Hits         9737     9718      -19     
- Misses       7405     7424      +19

Impacted Files	Coverage Δ
src/libsync/theme.cpp	`12.07% <0.00%> (ø)`
src/libsync/vfs/cfapi/hydrationjob.cpp	`52.68% <0.00%> (-3.77%)`	⬇️
src/libsync/vfs/cfapi/vfs_cfapi.cpp	`85.09% <0.00%> (-2.36%)`	⬇️
src/libsync/vfs/cfapi/cfapiwrapper.cpp	`72.89% <0.00%> (-1.37%)`	⬇️
src/libsync/syncengine.cpp	`86.65% <0.00%> (-0.55%)`	⬇️
src/libsync/discovery.cpp	`84.86% <0.00%> (+0.29%)`	⬆️

…ments Signed-off-by: aheimberger <andreasheimberger@gmx.at>

Signed-off-by: aheimberger <AndreasHeimberger@gmx.at>

nextcloud-desktop-bot · 2022-08-01T11:12:33Z

AppImage file: Nextcloud-PR-2135-e13c0e9fbcb1c7318a19fd1ddc0f884d26314b1c-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

mgallien

thanks again for keeping pushing for this
apologize for the time needed to get a review from me

mgallien · 2022-08-01T10:04:37Z

src/libsync/theme.cpp

@@ -763,8 +763,7 @@ QString Theme::versionSwitchOutput() const
 {
    QString helpText;
    QTextStream stream(&helpText);
-    stream << appName()
-           << QLatin1String(" version ")
+    stream << QLatin1String("version ")


please do not change that
I am pretty sure that having the appName can be useful in weird situation

mgallien · 2022-08-01T11:03:26Z

src/gui/application.cpp

+        { "logexpire", "Removes logs older than <hours> hours, to be used with --logdir", "hours" },
+        { "logflush", "Clears (flushes) the log output directory" },
+        { "logdebug", "Also outputs debug-level messages in the log", "", "true" },
+        { "confdir", "Uses the given configuration folder", "directory" },


implementation of this option is missing
please add it back

mgallien · 2022-08-01T11:06:30Z

src/gui/application.cpp

+    const QString logDir = _parser.value("logdir");
+    const QString logFile = _parser.value("logfile");
+    const int logExpire = _parser.value("logexpire").toInt();


please use const auto

mgallien · 2022-08-01T11:11:42Z

src/gui/application.cpp

+    const QStringList args = _parser.positionalArguments();
+    for (int i = 0; i < args.size(); i++) {
+        QString virtualFile = args.at(i);


can we use something like this

const auto args = _parser.positionalArguments(); for(const auto &virtualFile : args) {

mgallien · 2022-08-01T15:02:25Z

src/cmd/cmd.cpp

+        { "uplimit", "Limit the upload speed of files to n KB/s", "n", "0" },
+        { "downlimit", "Limit the download speed of files to n KB/s", "n", "0" },
+        { "logdebug", "More verbose logging" },
+        { "path", "Path to a folder on a remote server" }


this is taking a parameter
you can probably call it remotePath

mgallien · 2022-08-01T15:05:21Z

src/cmd/cmd.cpp

+    QString source_dir = args.at(0);
+    const QString target_url = args.at(1);
+    const QString options_user = parser.value("user");
+    const QString options_password = parser.value("password");
+    const QString proxy = parser.value("httpproxy");
+    const bool silent = parser.isSet("silent");
+    const bool trustSSL = parser.isSet("trust");
+    const bool useNetrc = parser.isSet("netrc");
+    const bool interactive = !parser.isSet("non-interactive");
+    const bool ignoreHiddenFiles = parser.isSet("ignore-hidden");
+    const QString exclude = parser.value("exclude");
+    const QString unsyncedfolders = parser.value("unsyncedfolders");
+    const int restartTimes = parser.value("max-sync-retries").toInt();
+    const int uplimit = parser.value("uplimit").toInt() * 1000;
+    const int downlimit = parser.value("downlimit").toInt() * 1000;
+    const bool logdebug = parser.isSet("logdebug");
+    const QString remotePath = parser.value("path");


please use const auto everywhere

mgallien · 2022-08-01T15:08:43Z

src/cmd/cmd.cpp

@@ -62,31 +61,6 @@ static void nullMessageHandler(QtMsgType, const QMessageLogContext &, const QStr
 {
 }

-struct CmdOptions


why removing it ?
that would have helped reduce the amount of modifications inside this file

AHeimberger force-pushed the refactor_useQCommandLineParser branch from 5291a2c to 3ebdeb2 Compare June 29, 2020 19:29

AHeimberger force-pushed the refactor_useQCommandLineParser branch 2 times, most recently from 4fc03e5 to c8f346b Compare July 6, 2020 14:13

er-vin requested changes Jul 6, 2020

View reviewed changes

src/gui/application.cpp Outdated Show resolved Hide resolved

src/gui/application.cpp Outdated Show resolved Hide resolved

AHeimberger force-pushed the refactor_useQCommandLineParser branch from 632dbc8 to 3fe74a3 Compare July 8, 2020 13:37

AHeimberger force-pushed the refactor_useQCommandLineParser branch from 655819f to 65552e7 Compare July 14, 2020 15:40

AHeimberger requested a review from er-vin July 16, 2020 09:05

er-vin added this to the 2.8 👥 Sharing features milestone Jul 20, 2020

er-vin modified the milestones: Desktop 3.1, Desktop 3.2 Dec 11, 2020

er-vin modified the milestones: Desktop 3.2, Desktop 3.3 Feb 2, 2021

mgallien modified the milestones: Desktop 3.3, Desktop 3.4 Jul 23, 2021

mgallien removed this from the 3.4.0 milestone Sep 8, 2021

camilasan approved these changes Feb 7, 2022

View reviewed changes

AHeimberger force-pushed the refactor_useQCommandLineParser branch 2 times, most recently from 176dda9 to feefed5 Compare March 14, 2022 21:30

AHeimberger force-pushed the refactor_useQCommandLineParser branch 2 times, most recently from f883b4c to 3dff96b Compare March 16, 2022 19:09

AHeimberger force-pushed the refactor_useQCommandLineParser branch from 6790090 to f5ec4b7 Compare March 24, 2022 21:30

AHeimberger force-pushed the refactor_useQCommandLineParser branch from f5ec4b7 to 2478024 Compare April 5, 2022 19:30

mgallien force-pushed the refactor_useQCommandLineParser branch from 2478024 to e1dbc55 Compare June 29, 2022 07:47

mgallien requested review from claucambra and camilasan and removed request for er-vin and FlexW June 29, 2022 09:17

AHeimberger force-pushed the refactor_useQCommandLineParser branch 2 times, most recently from c6e82dc to 4a3af3e Compare July 15, 2022 15:48

AHeimberger requested a review from er-vin July 15, 2022 19:11

mgallien removed the request for review from er-vin July 18, 2022 10:13

er-vin reviewed Jul 19, 2022

View reviewed changes

AHeimberger force-pushed the refactor_useQCommandLineParser branch from 2ad331f to a2f7474 Compare July 29, 2022 20:21

AHeimberger and others added 2 commits August 1, 2022 12:59

2135: refactoring - use qcommandlineparser to parse command line argu…

183e2fe

…ments Signed-off-by: aheimberger <andreasheimberger@gmx.at>

add const to parsed arguments

e13c0e9

Signed-off-by: aheimberger <AndreasHeimberger@gmx.at>

mgallien force-pushed the refactor_useQCommandLineParser branch from a2f7474 to e13c0e9 Compare August 1, 2022 10:59

mgallien reviewed Aug 1, 2022

View reviewed changes

refactor_useQCommandLineParser #2135

Are you sure you want to change the base?

refactor_useQCommandLineParser #2135

Conversation

AHeimberger commented Jun 29, 2020

er-vin commented Jun 29, 2020

AHeimberger commented Jun 29, 2020

er-vin commented Jun 30, 2020

AHeimberger commented Jul 2, 2020

er-vin left a comment

Choose a reason for hiding this comment

AHeimberger commented Jul 6, 2020

er-vin commented Jul 9, 2020

AHeimberger commented Jul 13, 2020 • edited Loading

er-vin commented Jul 13, 2020

AHeimberger commented Jul 14, 2020 • edited Loading

AHeimberger commented Jul 14, 2020 • edited Loading

er-vin commented Jul 15, 2020

AHeimberger commented Jul 15, 2020

er-vin commented Jul 15, 2020

AHeimberger commented Jul 16, 2020

er-vin commented Sep 16, 2020

er-vin commented Jan 7, 2021

mgallien commented Apr 20, 2021

AHeimberger commented Apr 25, 2021 • edited Loading

AHeimberger commented Mar 14, 2022

mgallien commented Jul 18, 2022 • edited Loading

er-vin 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

codecov bot commented Aug 1, 2022 • edited Loading

Codecov Report

nextcloud-desktop-bot commented Aug 1, 2022

mgallien 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

AHeimberger commented Jul 13, 2020 •

edited

Loading

AHeimberger commented Jul 14, 2020 •

edited

Loading

AHeimberger commented Jul 14, 2020 •

edited

Loading

AHeimberger commented Apr 25, 2021 •

edited

Loading

mgallien commented Jul 18, 2022 •

edited

Loading

codecov bot commented Aug 1, 2022 •

edited

Loading