HEnquist / camillagui-backend

Backend server for camillagui
GNU General Public License v3.0
19 stars 4 forks source link

Implemented save to/load from current_config #9

Closed JWahle closed 3 years ago

JWahle commented 3 years ago

This is part of the implementation of #8.

This includes:

HEnquist commented 3 years ago

I have looked at this and would propose a few changes. The name "current" indicates that this is the file that is active right now, but there is no guarantee that this is the case. It also means you have to define the path of the current file in two places (in the backend and when starting camilladsp). I would instead call it "default", and get the current config by querying the running CamillaDSP process. I also don't think the default should be overwritten.

These are the steps I would propose:

Thoughts?

JWahle commented 3 years ago
  • Rename the current_config option to default_config.

I agree that current_config is not a really fitting name and I'd gladly change the name, once we settled on the exact semantics of this config.

  • Add a new option update_current_config, true/false if the active config should be updated when /api/setconfig is called (using the path provided by the camilladsp process).
  • When the gui starts, let it load the current config if possible, if not fall back to the default file, and if even that fails use the one built into the gui.

I also thought about using config paths from CamillaDSP. This seems like a really clean approach. However, in the case of moOde, we use the ALSA CDSP plugin and thus, there is no guarantee, that there is a running CamillaDSP instance. We need a solution that behaves (save/load) the same whether CamillaDSP is running or not - hence the current_config file with its slightly awkward semantics.

If we keep the semantics of the current_config file, maybe more fitting names would be temporary_config or working_config. Then it would make sense to load it initially and save it automatically. So, do you think I should just rename the config property and call it a day, or do you see a cleaner way of implementing this?

HEnquist commented 3 years ago

Good point about the cdsp plugin. I see a big value in providing a separate default config that isn't overwritten. This would then replace the one hardcoded in the gui, and make it user customizable.

For the current_config, maybe rename it to working_config (not sure about the name, I'll think a little more).

How about this:

JWahle commented 3 years ago

I wonder, if all this complexity is really needed. These are the use cases for real world setups, that I currently see:

Use case 1: CamillaDSP as system service (https://github.com/HEnquist/camilladsp-config) Configure working_config to the same file as CamillaDSP (or when implemented leave working_config empty and let the GUI ask CamillaDSP for its config location, as you suggested). Then you see the active config when the GUI is opened and you can change and save it.

Use case 2: CamillaDSP is started by ALSA CDSP plugin Configure working_config to the same file as CamillaDSP in the ALSA CDSP plugin config. Then you see the active config when the GUI is opened and you can change and save it.

What additional use cases do you want to support with the default_config and update_working_config options?

HEnquist commented 3 years ago

I would add a few more that I'm aware of: 3) CamillaDSP processes input from SPDIF. A separate script monitors the incoming sample rate and switches camlladsp to a different configuration file when it changes. 4) The gui is used stand-alone just as an editor, camilladsp isn't running at all (this is possible, but not very well handled by the gui currently). 5) CamillaDSP is running in wait mode, and is given configs generated by a script - there is no current_config file.

The default_config would be set to point to a standard file included with the backend, as a replacement for the ugliness of having the default hard-coded in the frontend. After thinking a little more I agree that the update_working_config setting isn't needed. It should be enough to let the existence of the working_config setting decide.

For this PR then, that would only mean:

JWahle commented 3 years ago

For lack of a better name, I went with working_config. I tried to implement the version with the default_config, but then I realized, it is equivalent to the working_config with save_working_config = false. Since the latter was easier to implement, document and made more sense in the default camillagui.yaml, I went with the save_working_config approach. I also updated my PR for camillagui to work with these changes.

What do you think?

JWahle commented 3 years ago

This is mainly a reminder for myself. After we close this PR, I would like to talk to you about

I'll open new issues for these discussions.

JWahle commented 3 years ago

You're right, that was misleading.

HEnquist commented 3 years ago

I'm happy with this now but it's still marked as a draft. Should I merge it now, or do you want to do something more?

JWahle commented 3 years ago

Yes, please merge.