Server UI changes #2199

pljones · 2021-12-29T20:01:07Z

Short description of changes

My initial goal was to add to the server UI a way of setting (and changing in flight) the server list persistence file for a Jamulus directory.

However, during that change, it became apparent that the design of the interface between GUI and service layers was not geared to such in flight changes - the server is more expectant that it will be run with a configuration and left running "as is".

CHANGELOG: UI: Amend server registration, add server persistence

Context: Fixes an issue?

Jamulus Directory improvements: UI to set / amend / remove the server list persistence file;
Jamulus Server improvements: the server can be switched between:
- unregistered
- registered as a public server
- registered with a custom directory
- run as a Jamulus Directory

without restart, through the UI. That configuration will be saved and restored in the server inifile. If set to unregistered, the server will start up unregistered, rather than registered in the default directory. You can set the custom directory without registering, so you can set that whilst unregistered then switch to registering with the custom directory, and never register as a public server.

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

The UI has changed both in layout and behaviour. All server documentation probably needs review as a result.

Status of this Pull Request

I've been running it for months in headless mode. I've used it on Windows and Linux with the GUI and find it works much better than 3.8.1.

What is missing until this pull request can be merged?

More testing. UX feedback.

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

ann0see

Thank you. Will try soon.

src/serverdlg.cpp

src/util.h

pljones · 2022-01-03T08:46:44Z

Jamulus client - connects your sound system to the internet! Let's jam!

Jamulus server - somewhere Jamulus clients connect to for jams. A Jamulus server has an "address".

Server list - a list of Jamulus servers retrieved from a Jamulus directory (a Jamulus directory "publishes" a server list, if you like to use the word "publish" for anything at all)

Jamulus directory - a service with which a Jamulus server can choose to register and from which a Jamulus client can request a server list (list of Jamulus servers). A Jamulus directory has an "address".

Address - Jamulus client users will most likely use the server list in the Connect dialog, so only need to know the Jamulus directory and Name for the Jamulus server they want to use. However, the Conenct dialog also allows entering an Address. An Address can also be given for a Custom directory, both in client and server. The Address is a host name or IP address with an optional port number, for instance "jams.example.com:22555" or "192.168.1.54".

Public directory - all of the built in Jamulus directories, some custom directories

Custom directory - a Jamulus directory not built into the Jamulus server or client

Private directory - a custom directory that restricts access for server registration and server list retrieval

Private server - a server (optionally registered with a directory) that restricts access for client connection

NOTE: "Privacy" (i.e. running as a private directory or private server) is not directly supported by Jamulus - firewall set up is required. (When thinking about this, remember --showallservers will return the full list of servers registered at a directory, regardless of whether the client can access them; hence saying the directory must also restrict client access.)

pljones · 2022-01-03T17:13:45Z

For the "Directory" drop down:

"None" when no directory is selected (rather than "Unregistered" or "Not registered") (revert "Switch from "Unregistered" to "Not registered", some other changes instead)
One of the built in public directories
Or a "Custom" for the specified custom directory

For the status value:

Initially blank (i.e. if the user starts with "Directory" -> "None", it will remain blank)
On registering, the result of attempting to register (so, usually "Registered" but sometimes some other message - but initially, IIRC "Registering" until the successful response and then some other value on failure)
If the user chooses to switch "Directory" to "None", set the state to "Unregistered" (active - the server tells any directory it was registered in to remove it and, regardless of previous registration success, stops trying to register) -- it needs not to become blank but I'd be happy enough with "Not registered" if "Unregistered" is unpalletable.

pljones · 2022-01-06T20:46:46Z

Hm. Might have to rescind my idea of

Initially blank (i.e. if the user starts with "Directory" -> "None", it will remain blank)

The box holds a place holder of STATUS from the .ui layout, which gets overwritten with the current status -- i.e. "Not registered" if you're not registered - on start up, along with several other field updates (there's a method, CServerDlg::UpdateGUIDependencies that grabs current server values and stuffs them into the UI each time something changes - and on start up).

Currently, from start up:

$ ./Jamulus -s -i ini-server -p 56881
- server mode chosen
- initialization file name: ini-server
- selected port number: 56881
Using "192.168.1.19" as external IP.
Recording state not initialised
Recording state not initialised
libGL error: No matching fbConfigs or visuals found
libGL error: failed to load driver: swrast

(That last paragraph is there in 3.8.1 -- I'm not sure anyone looking at "What's This?" text wants to know that...)
(Also... in the second paragraph, "select ... in" should be "select ... from".)

I decided the label for the Custom Directory address had to change and, as I was making related changes (i.e. "What's This?" text on the drop down), I should do it now...

Then switch to custom...

Server Registration Status update: Registration requested

... but that wasn't a directory:

Server Registration Status update: Registration failed

Switch back to "None" until I get the right Custom Directory address:

Server Registration Status update: Not registered

Cut-n-paste the right value...

Switch to "Custom":

Server Registration Status update: Registration requested
Server Registration Status update: Registered

(with the slightest flicker of "Registration requested" before succeeding).

Switch back to "None" now:

Server Registration Status update: Not registered

pljones · 2022-01-06T21:08:02Z

We're also planning client Connect window changes:

Server list, first column, "Server Name" becomes "Name"
Server Address input box label becomes "Address"
What's This? for the label, "If you know the IP address or URL of a server..." (we don't support URL format...) becomes "Where a server isn't in any of the server lists, you can enter the address (for example, example.org:22524 or 192.168.1.54) you want to connect to here, or select one of the most recent values entered from the drop down list"
Accessibility help needs a similar update

(Client Settings/Advanced Custom directories "What's This?" looks good to me -- seems I've not changed my mind again since I wrote it... or I have but I've got back to where I was at that point...)

pljones · 2022-01-06T21:45:55Z

Hm. Can we try not running all of the builds in parallel? It might be causing the fails. If it doesn't fix it, we can switch back to parallel...

... third time lucky...

ann0see · 2022-01-09T19:38:32Z

Probably out of scope, but having the recorder directory in the Options tab while having the (by default disabled) "enable recorder" checkbox in the configuration tab seems very confusing. We don't hint the user at setting a recording directory, so by default it looks like the recording feature is not available (and this can't be fixed).

pljones · 2022-01-09T21:34:01Z

Probably out of scope, but having the recorder directory in the Options tab while having the (by default disabled) "enable recorder" checkbox in the configuration tab seems very confusing. We don't hint the user at setting a recording directory, so by default it looks like the recording feature is not available (and this can't be fixed).

The whole Server UI is a bit clumsy. Pretty much everything for a server is "set once and forget". You could almost have the list of clients filling one tab and a second tab "Settings" for everything, grouped more logically (with some more visual hinting to group related items).

pljones · 2022-01-20T07:44:10Z

Given no one's come up with an alternative that makes sense to registering when you register with a directory, can we go ahead with this?

I was hoping to get this as well as

into the coming release but it looks like that's been missed.

gilgongo · 2022-01-20T07:53:50Z

3.8.2 is supposed to be a bug fix release, but I don't suppose we need to stick to that.

hoffie · 2022-01-20T10:40:53Z

The UI has changed both in layout and behaviour. All server documentation probably needs review as a result.

Given that this PR touches many files and deliberately changes layout and behavior, I'd tend towards targeting 3.9.0. Such changes might come unexpected for users, e.g. see the unexpected (?) sandboxing side effects of the Mac signing stuff.

ann0see · 2022-02-20T09:26:22Z

Can this be merged right after the upcoming release? If yes, please do so.

pljones · 2022-02-20T17:59:24Z

I'll rebase (and squash shortly).

ann0see · 2022-02-20T18:22:08Z

Great!

pljones · 2022-02-20T18:28:21Z

Hm. I changed the commit comment. Let me try that again.

ann0see · 2022-02-20T18:37:11Z

I merged some PRs, maybe that's why - although I doubt that.

pljones · 2022-02-20T18:43:59Z

I merged some PRs, maybe that's why - although I doubt that.

No, I probably got lost somewhere locally - I had to redo what I was doing at one point.

src/util.h

pljones · 2022-02-21T21:00:25Z

It'll be easier to document this once it's stable on master. So does anyone want to approve it? :)

ann0see

Although only roughly tested a few weeks ago, I think we should merge this (and tag a nightly in a week or so)

softins

Had a look through and made some initial comments. Would really like to compile and try it out, but as I said, can't do that will Wednesday.
It looks like the "Register" checkbox has been replaced by a "None" option in the directory list. That means if the user wants to unregister their server, it can't remember where it was registered to, and the user needs to choose the directory afresh when re-registering. With an on/off checkbox, the preferred directory could be left set.

softins · 2022-02-21T22:00:26Z

src/server.cpp

@@ -684,12 +684,6 @@ void CServer::OnAboutToQuit()

    Stop();



Was it deliberate to remove the if ( GetServerRegistered() ) { Unregister(); } here? I can't see it has moved to anywhere else.

9222c20 was the original change doing this and ServerListManager.SetDirectoryType() seems to handle Unregister() if necessary: https://github.com/jamulussoftware/jamulus/blob/master/src/serverlist.cpp#L269-L275

It's called in https://github.com/jamulussoftware/jamulus/pull/2199/files#diff-c044ba02ccf7862f1e2f56436d5b92baa7bb35601791846655fb07ac253a8697R951

So this looks fine to me.

softins · 2022-02-21T22:03:08Z

src/main.cpp

-        if ( strDirectoryServer.isEmpty() )
-        {
-            // per definition, we must be a headless server and ignoring inifile, so we have all the information
+            // by definition, when running with the GUI we always default to registering somewhere but


This comment suggests that you can't have an unregistered ("private") server when running with the GUI. That's not true, surely (maybe it used to be?). Could the comment be clarified?

No, it says what it defaults to, not what's available.

I have just tried this code for the first time (in fact it was the code for #2427 I tried, which incorporates these changes), and compared with 3.8.2, using default empty ini files to ensure all defaults were used:

/usr/local/bin/Jamulus -s -i /tmp/x.ini ./Jamulus -s -p 22125 -i /tmp/y.ini

The 3.8.2 defaulted to "Not registered", with the "make my server public" checkbox unticked, which is good.

The newer version defaulted to registering with Any Genre 1, which I think is not good. The directory drop-down should default to "None", if not set in the command line or ini file, so that a new server does not register with a directory until the user asks it to.

src/main.cpp

hoffie

The code looks good to me, but I haven't done any real-world tests so far. I'll comment again when I get time to do those. At least one additional person should do them before merging, I guess. :)

Meta: This PR is rather large and was not easy to review for me. I did most of the initial reading via git log -wp 75e4ad0c100bce605c14d9fe0f0729143fc70bc2..a50f9e2f62b3bae8acaf4bfde1b0918b53db7aad (earlier state before rebasings) as it was much simpler for me to follow the individual commits.

What I think we should do for future refactoring PRs is:

Make refactorings only do refactorings. It's way harder to judge whether code reorganizations are correct if they are mixed with new functionality.
Use and keep individual commits with a polished subject and description so that they can be merged without squashing.

Thanks for doing this though, it looks much cleaner and logical to me now.

hoffie · 2022-02-23T00:27:48Z

src/server.cpp

@@ -684,12 +684,6 @@ void CServer::OnAboutToQuit()

    Stop();



9222c20 was the original change doing this and ServerListManager.SetDirectoryType() seems to handle Unregister() if necessary: https://github.com/jamulussoftware/jamulus/blob/master/src/serverlist.cpp#L269-L275

It's called in https://github.com/jamulussoftware/jamulus/pull/2199/files#diff-c044ba02ccf7862f1e2f56436d5b92baa7bb35601791846655fb07ac253a8697R951

So this looks fine to me.

hoffie · 2022-02-23T00:33:38Z

src/settings.cpp

+        // clang-format off
+// TODO compatibility to old version < 3.8.2
+directoryAddress = GetIniSetting ( IniXMLDocument, "server", "centralservaddr", directoryAddress );
+        // clang-format on


Not a huge fan of this break-identation-to-get-attention scheme personally (I know that the original code uses this in several places).

in several places

"Consistently" might be a better way of putting it ;).

gilgongo · 2022-07-10T11:19:44Z

For website screenshot URLs. Ignore.

ann0see reviewed Dec 30, 2021

View reviewed changes

src/serverdlg.cpp Outdated Show resolved Hide resolved

src/util.h Outdated Show resolved Hide resolved

pljones force-pushed the feature/server-list-persistence-ui branch from bc5e7bc to abe3c1f Compare December 31, 2021 16:53

pljones force-pushed the feature/server-list-persistence-ui branch from abe3c1f to 596385a Compare January 6, 2022 21:17

gilgongo mentioned this pull request Jan 9, 2022

Server UI Changes jamulussoftware/jamuluswebsite#677

Closed

2 tasks

pljones force-pushed the feature/server-list-persistence-ui branch from 596385a to fafc85d Compare January 9, 2022 12:40

pljones force-pushed the feature/server-list-persistence-ui branch from fafc85d to f539230 Compare January 10, 2022 17:42

pljones force-pushed the feature/server-list-persistence-ui branch 3 times, most recently from 98311c0 to c57d3a0 Compare January 17, 2022 19:48

gilgongo added the needs documentation PRs requiring documentation changes or additions label Jan 22, 2022

gilgongo added this to the Release 3.9.0 milestone Jan 22, 2022

pljones force-pushed the feature/server-list-persistence-ui branch 2 times, most recently from 0c55a50 to b4e4937 Compare January 29, 2022 14:59

pljones force-pushed the feature/server-list-persistence-ui branch from b4e4937 to a50f9e2 Compare February 12, 2022 22:14

pljones force-pushed the feature/server-list-persistence-ui branch from a50f9e2 to 0eb4436 Compare February 13, 2022 11:21

pljones force-pushed the feature/server-list-persistence-ui branch from 0eb4436 to be7b7c1 Compare February 20, 2022 18:20

pljones force-pushed the feature/server-list-persistence-ui branch from be7b7c1 to 6b61c23 Compare February 20, 2022 18:29

ann0see reviewed Feb 20, 2022

View reviewed changes

src/util.h Show resolved Hide resolved

ann0see approved these changes Feb 21, 2022

View reviewed changes

softins reviewed Feb 21, 2022

View reviewed changes

pljones force-pushed the feature/server-list-persistence-ui branch from 6b61c23 to a99dd6b Compare February 22, 2022 07:29

hoffie approved these changes Feb 23, 2022

View reviewed changes

UI: Amend server registration, add server persistence

1088c0a

pljones force-pushed the feature/server-list-persistence-ui branch from a99dd6b to 1088c0a Compare February 23, 2022 10:44

pljones merged commit 7d7b337 into jamulussoftware:master Feb 23, 2022

pljones mentioned this pull request Feb 23, 2022

Register My Server behaviour #2095

Closed

gilgongo mentioned this pull request Feb 24, 2022

General docs changes (v2) jamulussoftware/jamuluswebsite#714

Merged

3 tasks

softins mentioned this pull request Feb 28, 2022

Server GUI directory should default to "None" (not registered) #2441

Closed

pljones deleted the feature/server-list-persistence-ui branch February 28, 2022 17:52

gilgongo mentioned this pull request Jul 10, 2022

Screenshots and updates jamulussoftware/jamuluswebsite#772

Merged

gilgongo removed the needs documentation PRs requiring documentation changes or additions label Jul 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server UI changes #2199

Server UI changes #2199

pljones commented Dec 29, 2021 •

edited

Loading

ann0see left a comment

pljones commented Jan 3, 2022 •

edited

Loading

pljones commented Jan 3, 2022 •

edited

Loading

pljones commented Jan 6, 2022 •

edited

Loading

pljones commented Jan 6, 2022

pljones commented Jan 6, 2022 •

edited

Loading

ann0see commented Jan 9, 2022 •

edited

Loading

pljones commented Jan 9, 2022 •

edited

Loading

pljones commented Jan 20, 2022 •

edited

Loading

gilgongo commented Jan 20, 2022

hoffie commented Jan 20, 2022

ann0see commented Feb 20, 2022

pljones commented Feb 20, 2022

ann0see commented Feb 20, 2022

pljones commented Feb 20, 2022

ann0see commented Feb 20, 2022 •

edited

Loading

pljones commented Feb 20, 2022

pljones commented Feb 21, 2022

ann0see left a comment

softins left a comment

softins Feb 21, 2022

hoffie Feb 23, 2022

softins Feb 21, 2022

pljones Feb 22, 2022

softins Feb 28, 2022

hoffie left a comment

hoffie Feb 23, 2022

hoffie Feb 23, 2022

pljones Feb 23, 2022

gilgongo commented Jul 10, 2022

Server UI changes #2199

Server UI changes #2199

Conversation

pljones commented Dec 29, 2021 • edited Loading

Checklist

ann0see left a comment

Choose a reason for hiding this comment

pljones commented Jan 3, 2022 • edited Loading

pljones commented Jan 3, 2022 • edited Loading

pljones commented Jan 6, 2022 • edited Loading

pljones commented Jan 6, 2022

pljones commented Jan 6, 2022 • edited Loading

ann0see commented Jan 9, 2022 • edited Loading

pljones commented Jan 9, 2022 • edited Loading

pljones commented Jan 20, 2022 • edited Loading

gilgongo commented Jan 20, 2022

hoffie commented Jan 20, 2022

ann0see commented Feb 20, 2022

pljones commented Feb 20, 2022

ann0see commented Feb 20, 2022

pljones commented Feb 20, 2022

ann0see commented Feb 20, 2022 • edited Loading

pljones commented Feb 20, 2022

pljones commented Feb 21, 2022

ann0see left a comment

Choose a reason for hiding this comment

softins 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

hoffie 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

gilgongo commented Jul 10, 2022

pljones commented Dec 29, 2021 •

edited

Loading

pljones commented Jan 3, 2022 •

edited

Loading

pljones commented Jan 3, 2022 •

edited

Loading

pljones commented Jan 6, 2022 •

edited

Loading

pljones commented Jan 6, 2022 •

edited

Loading

ann0see commented Jan 9, 2022 •

edited

Loading

pljones commented Jan 9, 2022 •

edited

Loading

pljones commented Jan 20, 2022 •

edited

Loading

ann0see commented Feb 20, 2022 •

edited

Loading