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

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

Closed
jarkkojs opened this issue Jan 10, 2019 · 33 comments
Closed
Labels
Linux Issues which only occur on Linux

Comments

@jarkkojs
Copy link
Collaborator

jarkkojs commented Jan 10, 2019

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
Copy link
Collaborator Author

jarkkojs commented Jan 10, 2019

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
Copy link
Collaborator Author

jarkkojs commented Jan 10, 2019

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
Copy link
Collaborator Author

  • I think SurgeStorage should throw an exception on error and not do GUI at all. Now it continues on error, which is just wrong.
  • Whoever creates SurgeSynthesizer should catch this exception and display an error message.

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
Copy link
Collaborator

@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
Copy link
Collaborator Author

jarkkojs commented Jan 10, 2019

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
Copy link
Collaborator Author

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

@jarkkojs
Copy link
Collaborator Author

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

@esaruoho
Copy link
Collaborator

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

  • factory presets not present
  • waveform display will be empty
  • no wavetables present.

@jarkkojs
Copy link
Collaborator Author

jarkkojs commented Jan 10, 2019

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

@jarkkojs
Copy link
Collaborator Author

jarkkojs commented Jan 10, 2019

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
Copy link
Collaborator Author

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

@jarkkojs
Copy link
Collaborator Author

jarkkojs commented Jan 10, 2019

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

@jarkkojs
Copy link
Collaborator Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

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

@jarkkojs
Copy link
Collaborator Author

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

@jarkkojs
Copy link
Collaborator Author

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

@esaruoho
Copy link
Collaborator

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
Copy link
Collaborator Author

jarkkojs commented Jan 10, 2019

  • Add SurgeErrorDisplay class for displaying error messages (e.g. Uniform way to show errors to users #217)
  • Add error codes for missing wavetables, graphics and presets to SurgeError.
  • Catch exceptions in the plug-in layer and use SurgeErrorDisplay to display them (e.g. strip off error display from SurgeStorage.
  • Create configuration.xml if it is missing.
  • On Linux, the correct path is ~/.config/Surge/configuration.xml

@baconpaul
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

@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
Copy link
Collaborator

and that create amounts to "copy or unzip"

@jarkkojs
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@jarkkojs
Copy link
Collaborator Author

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

@jarkkojs jarkkojs changed the title Error message from a missing configuration file Fix the error recovery from missing configuration.xml. Jan 10, 2019
@jarkkojs jarkkojs changed the title Fix the error recovery from missing configuration.xml. Fix the error recovery from the missing configuration.xml. Jan 10, 2019
@baconpaul baconpaul added the Linux Issues which only occur on Linux label Jan 11, 2019
@baconpaul
Copy link
Collaborator

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 baconpaul changed the title Fix the error recovery from the missing configuration.xml. Display the error recovery from the missing configuration.xml on Linux Jan 15, 2019
@baconpaul
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux Issues which only occur on Linux
Projects
None yet
Development

No branches or pull requests

3 participants