Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manage settings with structs #609

Open
pljones opened this issue Sep 20, 2020 · 14 comments
Open

Manage settings with structs #609

pljones opened this issue Sep 20, 2020 · 14 comments
Labels
refactoring Non-behavioural changes, Code cleanup

Comments

@pljones
Copy link
Collaborator

pljones commented Sep 20, 2020

(Yes, I'm still trying to sort this out...)

My mind's eye view is that the server has a set of settings: SServerSettings. This is everything the server needs to know.

On start up, the structure is created (in main.cpp, defined in server.h).

Then, if GUI-enabled, CSettings populates it with values from the inifile - hence CSettings doesn't get passed a server instance any more - the server doesn't yet exist.

Next, all the values collected from the command line overwrite the appropriate entry in the settings structure.

And then finally, CServer gets instantiated with the SServerSettings instance reference.

To change a server setting, you call the server as you do now and it records that value in SServerSettings&.

When the server terminates, if GUI-enabled, CSettings is passed the SServerSettings reference to store to the inifile.


It's actually a fairly straight forward change in the server. The only concern I have when it comes to the client code... it isn't anywhere near as simple a model. Much of what's in the inifile isn't even for the client. So I'd keep the SClientSettings struct "unpolluted", free of GUI settings, perhaps having a separate SClientGUISettings for those.


One benefit is dropping all that code I added checking for command line options to prevent the inifile being used. The logic above handles that without any effort.

It could also help make it clear on what will persist to the inifile -- if it's not in one of the structs, it's not getting saved.

@corrados
Copy link
Contributor

corrados commented Jan 3, 2021

Should we really change anything with the settings? It just works as it is right now.

@pljones
Copy link
Collaborator Author

pljones commented Jan 6, 2021

It's a "wait and see" -- the setting / command line haven't changed recently but, when I raised this a lot of change was happening.

@pljones
Copy link
Collaborator Author

pljones commented Jan 23, 2021

OK, some more thoughts.

A couple of people have wanted to decouple of client GUI from the client itself and have a more programmer-friendly way of controlling the client - effectively, all the calls to the client from the GUI and the emits from the client to the GUI would need replacing with some other asynchronous interface.

Ideally - and what one of the people wanted - this would allow easy custom GUI construction, on top of that interface.

The other requirement was that remote control would be possible.

Whilst the server has some support for running without a GUI, it doesn't provide a full runtime control interface, either.

So, how does this relate to my ideas on the settings? Well, this was a kind of first step down the path - get the settings clearly separated out and managed by specific calls to update them.

I'm still half thinking "it's not going to happen"...

@gene96817
Copy link

gene96817 commented Jan 23, 2021

One caution about enabling custom client UI. It will become a nightmare to support. It will get complicated for users to help each other. The only way to avoid this complexity is if a custom client UI is in a closed ecosystem because they may be having different UI experiences. The proliferation of closed ecosystems will fragment the Jamulus user community.

The most important reason to have a custom client UI (that I can imagine) would be for
1- branding purposes for commercial projects
2- a school seeking to brand their program (perhaps with proprietary enhancements)

@atsampson
Copy link
Contributor

have a more programmer-friendly way of controlling the client

I've wondered occasionally if it'd be worth making Jamulus understand OSC - it's used by lots of other audio applications already (e.g. Ardour, which uses the liblo library), and it'd be a pretty good fit for controlling things like recording and the mixer remotely.

@WolfganP
Copy link

WolfganP commented Jan 24, 2021

have a more programmer-friendly way of controlling the client

I've wondered occasionally if it'd be worth making Jamulus understand OSC - it's used by lots of other audio applications already (e.g. Ardour, which uses the liblo library), and it'd be a pretty good fit for controlling things like recording and the mixer remotely.

A bit off topic...
We already have MIDI control for the mixer. What will OSC bring extra in terms of functionality?

@pljones
Copy link
Collaborator Author

pljones commented Jan 24, 2021

One caution about enabling custom client UI. It will become a nightmare to support.

That's my biggest concern. My default position on this is simple: "No, it's a very bad idea". But this is open source, so it's not up to me. Enablement (why isn't that a word...) and provision are two different things.

@gilgongo gilgongo added this to Triage in Tracking (old) via automation Mar 13, 2021
@gilgongo gilgongo moved this from Triage to Backlog in Tracking (old) Mar 13, 2021
@ann0see
Copy link
Member

ann0see commented Mar 4, 2022

@pljones I assume you're working on this (next-big-thing) branch on your repo? Can we close this?

@pljones
Copy link
Collaborator Author

pljones commented Mar 4, 2022

Well, this sort of is "the issue" my branch will be the PR to once I get it anywhere near finished... It's going to take time... (and that's been scarce in terms of enough at the same time to make progress).

@ann0see
Copy link
Member

ann0see commented Apr 22, 2022

@pgScorpio, I think this is somehow also related to the settings redesign?

Note: I will unsubscribe from this issue and won’t receive responses from any new comments. If you have any questions concerning maintenance, feel free to ping me.

@pgScorpio
Copy link
Contributor

Should we really change anything with the settings? It just works as it is right now.

Yes ! It works, but is far from ideal!

I'm working on settings too (See my branch) with the initial idea already implemented.... (But still far from complete)
And after the sound-redesign there could be even more improvements...

@pgScorpio
Copy link
Contributor

@pljones

I'm still half thinking "it's not going to happen"...

I'm still thinking "This HAS to happen" (Too much problems now)
Please let's work on this together.

@pgScorpio
Copy link
Contributor

@pljones

Well, this sort of is "the issue" my branch will be the PR to once I get it anywhere near finished... It's going to take time... (and that's been scarce in terms of enough at the same time to make progress).

Well I started working on this also (while waiting to get all my other PR's going.) and I think I'm already quite far in the right direction with settings. So let's work on this together.

@pgScorpio
Copy link
Contributor

@ann0see

@pgScorpio, I think this is somehow also related to the settings redesign?

Yes, for 100%

@pljones pljones moved this from Backlog to Triage in Tracking (old) Oct 5, 2022
@ann0see ann0see added the refactoring Non-behavioural changes, Code cleanup label Jul 1, 2023
@pljones pljones removed this from Triage in Tracking (old) Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Non-behavioural changes, Code cleanup
Projects
Status: Triage
Development

When branches are created from issues, their pull requests are automatically linked.

8 participants