surge-synthesizer / surge

Synthesizer plug-in (previously released as Vember Audio Surge)
https://surge-synthesizer.github.io/
GNU General Public License v3.0
3.09k stars 395 forks source link

Display the error recovery from the missing configuration.xml on Linux #250

Closed jarkkojs closed 5 years ago

jarkkojs commented 5 years ago

With Linux VST2 I get this:

SurgeStorage: Unable to load Surge configuration file "/home/jsakkine/.local/share/Surge/configuration.xml"

A file missing from the home folder (any file) is not an error condition. If a configuration file is missing from the home folder, the plug-in should either load or be able to create it.

What is so essential in this file that it cannot be created by the plug-in itself (the first version)?

jarkkojs commented 5 years ago

And what are snapshots and how they relate to the configuration?

Could this be instead:

if (!snapshotloader.LoadFile(snapshotmenupath.c_str())) 
   snapshotloader.SaveFile();

EDIT of course not it is just a dummy XML loader.

jarkkojs commented 5 years ago

And there is null pointer dereference here:

TiXmlElement* e = snapshotloader.FirstChild("autometa")->ToElement();

Assumes that the configuration file was found even when it wasn't. The whole error recovery scheme looks like broken.

jarkkojs commented 5 years ago

I would suggest that we create a single exception class called SurgeError that has integer codes (e.g. enum) for different errors and maybe mapping to messages for those codes. We can start with a single error CONFIG_FILE_NOT_FOUND.

I think it would make sense to always allocate SurgeStorage from heap to make the whole creation process better controlled.

esaruoho commented 5 years ago

@jsakkine can ya try and see what happens if the folder data/resources is copied manually to the path the configuration.xml error mentions?

jarkkojs commented 5 years ago

Yes, of course I tested it, but plug-in should not crash no matter what. It is in home folder so this is just broken behavior. After that it crashes in different location but I'll fix this one first. I.e. copying the file there is not a fix for this regression.

jarkkojs commented 5 years ago

Ideally plug-in should be able to create that file.

jarkkojs commented 5 years ago

@baconpaul, BTW why OS specific code is used for the error dialog? Couldn't you just derive something from CControl?

esaruoho commented 5 years ago

@jsakkine the configuration.xml missing error is pretty much the only way to know that:

jarkkojs commented 5 years ago

What are snapshots? State of controls? Could those be stripped out from the default? Can the plug-in re-create them?

jarkkojs commented 5 years ago

It would be better that plug-in layer would create SurgeStorage from heap and pass it as parameter to SurgeSynthesizer.

@baconpaul, right because SurgeGUIEditor is created after the synth you cannot use CControl at that point. That is a bummer. Maybe SurgeGUIEditor could be partially created?

jarkkojs commented 5 years ago

@esaruoho, how does configuration.xml help with that? There must be a thing called default values for anything you enumerate.

jarkkojs commented 5 years ago

Another remark. I think the config file is in a wrong place. Should be ~/.config/Surge/configuration.xml. @whydoubt, do you agree?

jarkkojs commented 5 years ago

@baconpaul, I think how display errors in this situation is cool but the errors are displayed in wrong place. We would need an VSTGUI independent class for displaying errors and it should be spawned in the plug-in host for SurgeStorage construction failure.

esaruoho commented 5 years ago

@jsakkine if configuration.xml is not in the right place, then presets, wavetables and the rest aren't there either. if configuration.xml is in the right place, then presets, wavetables and the rest are there.

configuration.xml is the canary in the goldmine, if it's dead = every asset is dead.

jarkkojs commented 5 years ago

Does not compute. It must appear anyway from the void when you install Surge. There must be defaults.

jarkkojs commented 5 years ago

I'm sorry but I just don't understand that deduction that you are trying to give me.

jarkkojs commented 5 years ago

If configuration.xml is missing, you assume default locations. Simple as that.

esaruoho commented 5 years ago

and if content in default locations is missing, you need to shoot errors. i don't see anyone writing tickets about shooting errors for "missing wavetables" "missing waveform gfx" "missing factory presets".

jarkkojs commented 5 years ago
baconpaul commented 5 years ago

I rather disagree with "create configuration.xml if missing" - that's exactly the bug that had is in mac configuration hell for the last few days. On windows and mac, the surge installer sets up the information (in %LOCALAPPDATA% on windows and in /Library/Application Suport/Surge on mac) that you need to start.

So @esaruoho is correct; the entire set of configuration.xml and the wavetables comes in the installer and is placed "somewhere" as a bundle; if the configuration is missing it means you didn't run the installer.

I would say about 10% of our last 30 days of mac debugging have involved coming to terms with this fact. Surge works by running an installer which installs a binary and installs configuration information in a well known system place.

jarkkojs commented 5 years ago

Well, then the home directory is wrong place for that file. How does it scale to multiple users? I don't know a single piece of good application software that isn't able to generate default settings based on known defaults.

baconpaul commented 5 years ago

@jsakkine yeah we had all the defaults in the mac bundle pre installer and moved them. That worked well until it didn't because mac has a good place to put them in /Library when you install.

The assets you need are all in resources/data at build time and need to be (wherever you want to put them) at runtime. The mechanism to accomplish that copy through an install is platform dependent. You could bake resources/data into a zip file and unzip them at runtime on linux, say. And @kurasu and I discussed doing that for all platforms in 1.7 so this problem goes away. Because it is a bit wonky. But of course that would ship the assets with every plugin as opposed to with every system.

I"m not sure how linux would do this. Maybe we will find out in the year of the linux desktop. ;) But basically the workflow you have to engineer for your users is

  1. Get the stuff from resources/data - all of it - at build time
  2. Put it somewhere so it is transmittable to end users
  3. have that transit unpack it on the filesystem - somewhere
  4. read it off the filesystem and if it isn't there (as shown by canary in coalmine configuration.xml) tell them to reinstall or
  5. read it off the filesystem and if it isn't there create it all (not just configuration.xml but all the over 40mb of content)

and then surge works.

baconpaul commented 5 years ago

and that create amounts to "copy or unzip"

jarkkojs commented 5 years ago

Actually things where stuff goes on Linux desktop has been fairly standardized thanks to freedesktop.org. No major changes for lets say a decade maybe.

Before thinking end users it is better to get this shit actually running on Linux. In the scope of this bug I will just create that CControl I've spoken of and change the path for configuration.xml. Anyway, at least we identify that there is still some plumbing to do in configuration.

One question about configuration.xml: why is snapshot information needed before Surge is used for the first time? Looking at it, it looks like default states for knobs.

baconpaul commented 5 years ago

And as to well defined app software creating defaults - while that is true this isn’t defaults. Think of this more like “an install of Gcc also includes headers”. If I copy /use/bin/gcc to another computer and run it I don’t expect it to be able to create stdio.h. Configuration.xml is like stdio.h and not like $HOME/.gccprefs or whatever

That mental model is much better than a “user defaults” mental model when thinking about this. At least it was for me!

baconpaul commented 5 years ago

Yeah yeah I know on the desktop stuff. Sorry lightly trolling linux desktop folks is a bad habit!

Configuration.xml also defines things like mappings of oscillator names to types. I think a 1.7 release would have us look at it indeed but for now the thing you need to do is “have it and all the other content in resources/data be wherever you read it from when you start somehow”

jarkkojs commented 5 years ago

I'm already happy that now with the exception Renoise deals with the error situation nicely and does not hog all my memory and stall desktop for few minutes :) Now I can actually fix stuff.

jarkkojs commented 5 years ago

Opened a topic branch https://github.com/jsakkine/surge/tree/cerrorbox. Except a PR maybe during the weekend or early next week.

jarkkojs commented 5 years ago

@baconpaul, put #linux to this issue. Thanks.

baconpaul commented 5 years ago

If you end up liking #217 I think this will just become "implement #217 for linux with control." Leaving it open but changing the title.

baconpaul commented 5 years ago

Hey @jsakkine is there something we are still doing here? Seems we now display it properly using the LInuxInteractions. Or is this implicitly now the ticket to "Write linux/LinuxUserInteractions.cpp"? (In which case I can just rename it!)

Thanks

jarkkojs commented 5 years ago

I'm still gonna do CControl for displaying GUI message on Linux but it requires some sidesteps. My other PRs are related to refactoring code so that it is possible. That is why I'm interested on moving cpu detection code to synth constructor etc.

jarkkojs commented 5 years ago

And I need to change SurgeGUIEditor singleton, because it must exist before the synth gets constructed so that the control can have a parent. Still lots of work to do.