HEnquist / camillagui-backend

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

Should not set active config when saving to file #22

Closed HEnquist closed 3 years ago

HEnquist commented 3 years ago

When the config is saved to file, it always wants to set this new config as the active. Don't do this, we might not want it as active. Also, if the "active_config" setting is empty it should never try to make links (which will allow this to work on windows).

JWahle commented 3 years ago

When the config is saved to file, it always wants to set this new config as the active. Don't do this, we might not want it as active.

What's the problem with setting it as active? This behavior is similar to basic text editors on windows/linux(gedit): if you "Save as" a file, it becomes the current file of the editor and the editor displays, that there are no unsaved changes. I plan to implement the "no unsaved changes" functionality similarly for CamillaGUI.

Also, if the "active_config" setting is empty it should never try to make links (which will allow this to work on windows).

Does that mean, that on windows creating symbolic links doesn't work? According to the python docs, this should not be a problem - so there probably is a bug somewhere in the backend code. Is there a backend stacktrace, that you can post here?

HEnquist commented 3 years ago

When the config is saved to file, it always wants to set this new config as the active. Don't do this, we might not want it as active.

What's the problem with setting it as active? This behavior is similar to basic text editors on windows/linux(gedit): if you "Save as" a file, it becomes the current file of the editor and the editor displays, that there are no unsaved changes. I plan to implement the "no unsaved changes" functionality similarly for CamillaGUI.

It's fine to set it as active for the gui, but if you run camilladsp with the "active_config" symlink as config file, then it also becomes the default one for the dsp. That part feels very non-intuitive, as soon as I save to some name, that one becomes the default for the dsp. Instead, setting the config as default should be a separate action.

Also, if the "active_config" setting is empty it should never try to make links (which will allow this to work on windows).

Does that mean, that on windows creating symbolic links doesn't work? According to the python docs, this should not be a problem - so there probably is a bug somewhere in the backend code. Is there a backend stacktrace, that you can post here?

For some completely random reason you can't create symlinks on windows without special permissions. To do it the user that runs the backend needs to have the "Create Symbolic Links" privilege: https://community.perforce.com/s/article/3472 So while symlnks on windows are possible, I think this will never work well in practice.

I haven't thought through the details yet, but I think something along these lines is needed:

JWahle commented 3 years ago
  • if the active_config setting is empty, then never create any symlink (windows mode)

This is implemented in 29492ad018e3383cde002d5c6444c4dc8449d690 in the develop branch.

  • saving the config to a file sets that config as active for the gui, but does not update the symlink. The frontend should keep track of what is the active config (the backend should not since you could have the gui open in several browser windows).
  • a new button is added to the gui, to make the config the default for the dsp. Then the symlink is updated.

I created PR https://github.com/HEnquist/camillagui/pull/39 and https://github.com/HEnquist/camillagui-backend/pull/24 for this, please try this out. It is a little more convenient, but I also find it harder to understand, what is happening. Not sure, if this is the way to go.

HEnquist commented 3 years ago

Alright! I hope to have have time tomorrow to try it.

HEnquist commented 3 years ago

I have tried this. I'm happy with how the new version works, and for me this is more logical. I can for example open some config, edit it and save it under a new name, then change samplerate and fir coeffciient, and save it under yet another name. And noee of it changes the active config until I tell it to.

What is it that you don't like?

From my side, I think this is ready now to publish a new release.

JWahle commented 3 years ago

What is it that you don't like?

The user now has to understand the difference between a file being set to active and a file being opened in the editor. I'm not sure, how obvious it will be for users. You can for example apply a config, and it will be used by CDSP, but the changes won't persist when it restarts.

The model before was pretty simple - after you "Load" or "Apply", the state that you see in the GUI is the state your CDSP will be in.

I'd say for now, we should stick with the new functionality, but we might need to change it, if it creates too much confusion.

JWahle commented 3 years ago

I merged the PRs and updated the master branches of frontend and backend. So everything should be ready for the new release.

HEnquist commented 3 years ago

The user now has to understand the difference between a file being set to active and a file being opened in the editor. I'm not sure, how obvious it will be for users. You can for example apply a config, and it will be used by CDSP, but the changes won't persist when it restarts.

The model before was pretty simple - after you "Load" or "Apply", the state that you see in the GUI is the state your CDSP will be in.

I'd say for now, we should stick with the new functionality, but we might need to change it, if it creates too much confusion.

Yes I see your point. Both the new and the old can be confusing, depending on how you look at the gui. Is it a front panel that directly controls the dsp? Or is an editor for configs, that can upload a config to the dsp?

The previous version was more suited for the first viewpoint, but I found the behaviour when saving to files confusing. Perhaps we could add a toggle switch for "live mode" / "offline mode", that switches between the two?

I agree, let's leave it like it is now until we have some feedback. I'll make new releases of everything as soon as I can.