Change "Central Server" to "Directory Server" in variables and settings #2079

pljones · 2021-10-21T21:23:43Z

Short description of changes

Nothing changes! At least, nothing should visibly change -- this is the follow-up to my UI changes from "Central Server" to "Directory Server/Server List" (still in flux). I've also got rid of "slave server" references, replacing with "registered server".

Context: Fixes an issue?

Jamulus isn't centralised any more.

Does this change need documentation? What needs to be documented and how?

No documentation changes required.

It's probably worth highlighting that settings files must be backed up before upgrading Jamulus, because we don't have forwards compatibilty (i.e. we don't write the old versions as well as the new versions).

Status of this Pull Request

Ready to merge after 3.8.1 release - ideally straight away so everything (else...) can break and have time to be fixed.

What is missing until this pull request can be merged?

Careful code review. Ideally someone else to confirm it works for them. Checking the settings code is particularly important.

Release 3.8.1 needs to go out.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

pljones · 2021-10-24T21:46:16Z

Consider this still in draft. I know I've some more names I want to change having done this...

softins · 2021-10-25T16:47:31Z

Consider this still in draft. I know I've some more names I want to change having done this...

is this still the case? You could click on Convert to draft to mark it if so, and then convert back when you have finished.

softins

Well it compiles for me ok. Changes look good.

One missed name: a grep finds SRR_CENTRAL_SVR_FULL in src/util.h, used in src/serverlist.cpp.

softins · 2021-10-25T16:55:16Z

src/settings.cpp

+// TODO compatibility to old version (< 3.8.2)
+QString strCurAddr = GetIniSetting ( IniXMLDocument, "client", QString ( "centralservaddr%1" ).arg ( iIdx ), "" );
+        // clang-format on
+        strCurAddr = GetIniSetting ( IniXMLDocument, "client", QString ( "directoryserveraddress%1" ).arg ( iIdx ), strCurAddr );


Not critical, but I would prefer to check for directoryserveraddress%1 first, and only look for centralservaddr%1 if that wasn't found. Also in three places further down.

That needs more conditional logic to be written. This I get for free with the defaults doing their thing.

softins · 2021-10-25T16:55:54Z

src/settings.cpp

+        eDirectoryType = static_cast<EDirectoryType> ( iValue );
+    }
+    // clang-format on
+    else if ( GetNumericIniSet ( IniXMLDocument, "client", "directorytype", 0, static_cast<int> ( AT_CUSTOM ), iValue ) )


Order of checking, as above

softins · 2021-10-25T16:56:22Z

src/settings.cpp

+    pServer->SetDirectoryType ( static_cast<EDirectoryType> ( iValue ) );
+}
+    // clang-format on
+    else if ( GetNumericIniSet ( IniXMLDocument, "server", "directorytype", 0, static_cast<int> ( AT_CUSTOM ), iValue ) )


Order of checking, as above.

softins · 2021-10-25T16:56:37Z

src/settings.cpp

+// TODO compatibility to old version
+QString directoryServerAddress = GetIniSetting ( IniXMLDocument, "server", "centralservaddr", "" );
+        // clang-format on
+        directoryServerAddress = GetIniSetting ( IniXMLDocument, "server", "directoryserveraddress", directoryServerAddress );


Order of checking, as above.

softins · 2021-10-25T17:12:22Z

I just tried running it, and it successfully picked up my custom directories from the old Jamulus.ini file. When I exited, it saved them under the new names. OK so far...

I then ran my standard 3.8.1 client, and it had (of course) forgotten its custom directory servers, because they had been saved into Jamulus.ini under names it didn't know.

Most people will only move from an older release to a newer one, but this means anyone testing a post-3.8.1 dev version will lose their custom directory servers if they go back to a 3.8.1 release or earlier.

I think the only ways to overcome that are:

Write to Jamulus.ini records under both names, or
Accept that Jamulus.ini will always use "central", not "directory".
Or else live with it and make sure people know.

pljones · 2021-10-25T17:56:59Z

I just tried running it, and it successfully picked up my custom directories from the old Jamulus.ini file. When I exited, it saved them under the new names. OK so far...

I then ran my standard 3.8.1 client, and it had (of course) forgotten its custom directory servers, because they had been saved into Jamulus.ini under names it didn't know.

Most people will only move from an older release to a newer one, but this means anyone testing a post-3.8.1 dev version will lose their custom directory servers if they go back to a 3.8.1 release or earlier.

I think the only ways to overcome that are:
1. Write to `Jamulus.ini` records under _both_ names, or

We never do that.

2. Accept that `Jamulus.ini` will always use "central", not "directory".

We don't need to do that.

3. Or else live with it and make sure people know.

Like I said in the PR:

Does this change need documentation? What needs to be documented and how?

No documentation changes required.

It's probably worth highlighting that settings files must be backed up before upgrading Jamulus, because we don't have forwards compatibilty (i.e. we don't write the old versions as well as the new versions).

It's always been the case (since before server registration existed, indeed) that the inifile is not forwards compatible, but is backwards compatible.

pljones · 2021-10-25T18:02:40Z

is this still the case? You could click on Convert to draft to mark it if so, and then convert back when you have finished.

I would if I could see how to.

softins · 2021-10-25T18:28:13Z

is this still the case? You could click on Convert to draft to mark it if so, and then convert back when you have finished.

I would if I could see how to.

It’s a small link just below reviewers and above assignees.

softins · 2021-10-25T18:32:01Z

Like I said in the PR:

It's always been the case (since before server registration existed, indeed) that the inifile is not forwards compatible, but is backwards compatible.

ah yes, I should have read more carefully ;)

pljones · 2021-10-25T19:32:14Z

It’s a small link just below reviewers and above assignees.

No wonder I couldn't find it - I thought that section was about reviewers, not changing the state of the PR to draft ... 🙄

pljones · 2021-10-25T22:09:02Z

Sorry for the messing around on this one... My generally mildly chaotic approach seemed to get the better of me and I'd got changes being made in later branches that should be here...

Some things probably still don't make sense -- like I now have "DirectoryAddress" consistently in variables but the UI shows "Directory Server Address". As to what's in comments... That's not a big issue, though.

I now try to think of it like this:

+=========================+=============+
| Run as...               | Term        |
+=========================+=============+
| Jamulus                 | "client"    |
| Jamulus -s              | "server"    |
| Jamulus -s -e localhost | "directory" |
+-------------------------+-------------+

If a server uses a -e other than localhost, it's registered, otherwise it's unregistered or not registered.

Whether it's "public" or "private" is down to whether the directory it registers with is "public" or "private". All the built in ones are public. Custom directories can choose but there's nothing to indicate this choice.

pljones · 2021-11-01T08:39:36Z

Anyway, this has been stable for a while (at least 24 hours...) and I don't plan further changes.

Yet to come (I wish it was easy on Github to say "diff this against that")

bugfix/custom-serverlist-keyboard-control https://github.com/pljones/jamulus/tree/bugfix/custom-serverlist-keyboard-control
This allows deleting entries from the custom directory address list just using the keyboard more easily.
patch/ocd-reorder-of-settings https://github.com/pljones/jamulus/tree/patch/ocd-reorder-of-settings
patch/ocd-reorder-of-serverdlg-bits https://github.com/pljones/jamulus/tree/patch/ocd-reorder-of-serverdlg-bits
These two are preparatory "me just unable to leave things well alone" changes -- I needed to get them organised to see what was happening - or at least keep it in my head clearly.
feature/server-list-persistence-ui https://github.com/pljones/jamulus/tree/feature/server-list-persistence-ui
And this is where I was trying to get to (from this PR).

All broken into nice small commits currently, so the individual commit can be looked at. These are still sort of work in progress but I'm now pretty happy with them.

pljones · 2021-11-06T10:23:20Z

@softins

One missed name: a grep finds SRR_CENTRAL_SVR_FULL in src/util.h, used in src/serverlist.cpp.

Eek, missed you mentioning that. Done.

softins · 2021-11-06T10:40:46Z

@softins

One missed name: a grep finds SRR_CENTRAL_SVR_FULL in src/util.h, used in src/serverlist.cpp.

Eek, missed you mentioning that. Done.

Ok, will re-review and approve today.

softins

Further suggestions:
src/connectdlg.cpp:48 - change "Use the List dropdown" to "Use the Directory dropdown".
src/connectdlg.cpp:74-75 - reword to use "directory" terminology.
src/serverdlgbase.ui:73 - change "Register My Server in the Server List" to "Register My Server in the chosen Directory" or something.
src/serverdlgbase.ui:82 - change "List" to "Directory", to match the client's connect dialog.

src/serverdlg.cpp

pljones · 2021-11-11T19:46:56Z

OK...

Connect Dialog done on
pljones/jamulus@feature/Central-to-Directory-in-variables...pljones:feature/connect-dialog-help-improvements

Server Dialog done on
pljones/jamulus@patch/ocd-reorder-of-settings...pljones:patch/ocd-reorder-of-serverdlg-bits
(which maybe could do with the actual changes splitting from the OCD reordering bits...)

pljones · 2021-11-12T07:15:08Z

@jamulussoftware/maindevelopers Anyone else able to review?

pljones force-pushed the feature/Central-to-Directory-in-variables branch 4 times, most recently from e5ca3a4 to 201fd0d Compare October 23, 2021 22:17

gilgongo added this to the Release 3.9.0 milestone Oct 24, 2021

pljones force-pushed the feature/Central-to-Directory-in-variables branch 4 times, most recently from b8ffb8b to 14c9c46 Compare October 24, 2021 21:42

pljones force-pushed the feature/Central-to-Directory-in-variables branch from 14c9c46 to b54b1b8 Compare October 25, 2021 07:48

softins reviewed Oct 25, 2021

View reviewed changes

pljones marked this pull request as draft October 25, 2021 19:29

pljones force-pushed the feature/Central-to-Directory-in-variables branch 2 times, most recently from 9ceddbf to 9ea13f0 Compare October 25, 2021 21:54

pljones marked this pull request as ready for review October 25, 2021 21:57

pljones force-pushed the feature/Central-to-Directory-in-variables branch from 9ea13f0 to 38d8518 Compare October 30, 2021 10:14

pljones force-pushed the feature/Central-to-Directory-in-variables branch from b4bbd35 to 6d6bbd0 Compare November 8, 2021 20:29

softins reviewed Nov 11, 2021

View reviewed changes

src/serverdlg.cpp Show resolved Hide resolved

src/serverdlg.cpp Show resolved Hide resolved

softins approved these changes Nov 11, 2021

View reviewed changes

pljones added 2 commits November 13, 2021 10:02

Rename "Central" to "Directory" in source code

997fdd5

Certain other rewording

2d2b0ae

pljones force-pushed the feature/Central-to-Directory-in-variables branch from 6d6bbd0 to 2d2b0ae Compare November 13, 2021 10:03

pljones merged commit d0393c4 into jamulussoftware:master Nov 13, 2021

pljones deleted the feature/Central-to-Directory-in-variables branch November 13, 2021 12:12

pljones modified the milestones: Release 3.9.0, Release 3.8.2 Feb 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change "Central Server" to "Directory Server" in variables and settings #2079

Change "Central Server" to "Directory Server" in variables and settings #2079

pljones commented Oct 21, 2021 •

edited

Loading

pljones commented Oct 24, 2021

softins commented Oct 25, 2021

softins left a comment

softins Oct 25, 2021

pljones Oct 25, 2021

softins Oct 25, 2021

softins Oct 25, 2021

softins Oct 25, 2021

softins commented Oct 25, 2021

pljones commented Oct 25, 2021 •

edited

Loading

pljones commented Oct 25, 2021

softins commented Oct 25, 2021

softins commented Oct 25, 2021

pljones commented Oct 25, 2021

pljones commented Oct 25, 2021 •

edited

Loading

pljones commented Nov 1, 2021

pljones commented Nov 6, 2021

softins commented Nov 6, 2021

softins left a comment

pljones commented Nov 11, 2021 •

edited

Loading

pljones commented Nov 12, 2021

Change "Central Server" to "Directory Server" in variables and settings #2079

Change "Central Server" to "Directory Server" in variables and settings #2079

Conversation

pljones commented Oct 21, 2021 • edited Loading

Checklist

pljones commented Oct 24, 2021

softins commented Oct 25, 2021

softins left a comment

Choose a reason for hiding this comment

softins Oct 25, 2021

Choose a reason for hiding this comment

pljones Oct 25, 2021

Choose a reason for hiding this comment

softins Oct 25, 2021

Choose a reason for hiding this comment

softins Oct 25, 2021

Choose a reason for hiding this comment

softins Oct 25, 2021

Choose a reason for hiding this comment

softins commented Oct 25, 2021

pljones commented Oct 25, 2021 • edited Loading

pljones commented Oct 25, 2021

softins commented Oct 25, 2021

softins commented Oct 25, 2021

pljones commented Oct 25, 2021

pljones commented Oct 25, 2021 • edited Loading

pljones commented Nov 1, 2021

pljones commented Nov 6, 2021

softins commented Nov 6, 2021

softins left a comment

Choose a reason for hiding this comment

pljones commented Nov 11, 2021 • edited Loading

pljones commented Nov 12, 2021

pljones commented Oct 21, 2021 •

edited

Loading

pljones commented Oct 25, 2021 •

edited

Loading

pljones commented Oct 25, 2021 •

edited

Loading

pljones commented Nov 11, 2021 •

edited

Loading