Store recording directory #513

pljones · 2020-08-11T17:27:09Z

One problem. Once set, there's no way from the command line to say "clear the base recording dir". You can change it, but not clear it. (Same goes for some other strings.)

corrados · 2020-08-11T17:30:30Z

Please use recordingdir_base64 instead of recording_dir. I am only using _ to show that it is base 64.

You can change it, but not clear it.

Do you want to fix it before I merge it?

pljones · 2020-08-11T20:41:00Z

Do you want to fix it before I merge it?

I'd need to be changing main.cpp, too. Should I change all the settings that say

"If the target value is not empty, use the inifile value"
so they say
"If the target value is not empty and the command line did not set it to empty, use the inifile value"?

(I was thinking of first looping through the command line for an inifile setting, else using the default, then looping through the other known command line options, setting the target value if present else using the inifile value; then any other inifile settings - i.e. ones with no matching command line option - would get loaded. Cumbersome...)

pljones · 2020-08-11T20:47:50Z

Alternative approach:

in main.cpp, when looping over the command line arguments, a "found" list is maintained (list of strings), with each matched entry being added by its long option name (e.g. --recording)
when calling Settings.Load(), you'd pass the list, and then through to ReadSettingsFromXML(), and not use the inifile value for anything overridden on the command line

(If you wanted to stop storing the command line value in the inifile, you could do the same thing when saving...)

corrados · 2020-08-16T11:18:03Z

Good idea with the string list, thanks. I'll add some minor comments in your code now.

corrados · 2020-08-16T11:19:36Z

src/main.cpp

@@ -99,6 +100,7 @@ int main ( int argc, char** argv )
        {
            bIsClient = false;
            tsConsole << "- server mode chosen" << endl;
+            CommandLineOptions += "--server";


Should we only set the strings for command line arguments which are actually used in the settings class? I actually prefer to add them all as you did but the other solution would give less code and would make clear that the string is actually used. What is your opinion?

I think there should be "greater equality" between command line and GUI - at the moment, there's a lot that the UI can do (particular the client) that can't be set on start up from the command line. I'd also like to see most if not all of what can be set from the command line available in the UI. And then the UI settings should all get loaded / saved to the inifile.

So it seemed logical to help that process by putting this in consistently, at least.

Ok, agreed.

corrados · 2020-08-16T11:21:11Z

src/main.cpp

@@ -111,6 +113,7 @@ int main ( int argc, char** argv )
        {
            bUseGUI = false;
            tsConsole << "- no GUI mode chosen" << endl;
+            CommandLineOptions += "--nogui";


At the places in Jamulus where I have used the QStringList class, I have used the operator<< instead of the +=. Does the += give a different result? If not, for consistency, we should use << here, too.

Oh - I was actually surprised += worked. My C backgound -- << is very C++ ;). Yes, << as append seems much better! I'll fix that.

corrados · 2020-08-16T11:22:55Z

src/settings.cpp

    }

    // licence type (command line overwrites setting file)
-    if ( pServer->GetLicenceType() == LT_NO_LICENCE )
+    //if ( pServer->GetLicenceType() == LT_NO_LICENCE )
+    // TODO: command line needs to become boolean, at least...


Why should the command line be a boolean value? It is actually a boolean value. Do you want the command line to not be a boolean value instead?

Maybe I misread it -- I thought it was only checking for presence, rather than value.

corrados · 2020-08-16T11:24:10Z

src/settings.cpp

    {
        pServer->SetWelcomeMessage ( FromBase64ToString ( GetIniSetting ( IniXMLDocument, "server", "welcome" ) ) );
    }

    // window position of the main window
    vecWindowPosMain = FromBase64ToByteArray (
        GetIniSetting ( IniXMLDocument, "server", "winposmain_base64" ) );
+
+    // base recording directory (command line overwrites setting file)


Since now all settings are overwritten by the command line, maybe we should remove all the "(command line overwrites setting file)", what do you think?

It could go as a comment on the call in main.cpp - I think it needs mentioning.

Ok, let's do that later on. I'll now merge your code.

I just saw that you already put it in main.cpp -> done.

pljones mentioned this pull request Aug 11, 2020

Store recorder settings #313

Closed

corrados mentioned this pull request Aug 13, 2020

Improve interaction between use of inifile and command line parameters (edited) #120

Closed

pljones force-pushed the feature/save-recorder-settings branch from 81939fe to 502d3fe Compare August 14, 2020 19:06

corrados reviewed Aug 16, 2020

View reviewed changes

pljones added 5 commits August 16, 2020 18:30

Store recording directory

d0f8784

Use explicit _base64 suffix on new setting name

a078462

Initial commit for alternate command line overrides approach

c8580fc

Amend append

9d75e47

Move override comment

428cc83

pljones force-pushed the feature/save-recorder-settings branch from 502d3fe to 428cc83 Compare August 16, 2020 17:40

corrados merged commit 73d7fa1 into jamulussoftware:master Aug 17, 2020

pljones deleted the feature/save-recorder-settings branch August 18, 2020 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store recording directory #513

Store recording directory #513

pljones commented Aug 11, 2020

corrados commented Aug 11, 2020 •

edited

Loading

pljones commented Aug 11, 2020

pljones commented Aug 11, 2020 •

edited

Loading

corrados commented Aug 16, 2020

corrados Aug 16, 2020

pljones Aug 16, 2020

corrados Aug 17, 2020

corrados Aug 16, 2020

pljones Aug 16, 2020 •

edited

Loading

corrados Aug 16, 2020

pljones Aug 16, 2020

corrados Aug 16, 2020

pljones Aug 16, 2020

corrados Aug 17, 2020

corrados Aug 17, 2020

Store recording directory #513

Store recording directory #513

Conversation

pljones commented Aug 11, 2020

corrados commented Aug 11, 2020 • edited Loading

pljones commented Aug 11, 2020

pljones commented Aug 11, 2020 • edited Loading

corrados commented Aug 16, 2020

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

pljones Aug 16, 2020 • edited Loading

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

corrados commented Aug 11, 2020 •

edited

Loading

pljones commented Aug 11, 2020 •

edited

Loading

pljones Aug 16, 2020 •

edited

Loading