Surge: Make Linux UserInteractions use something other than fork or popen zenity

[jarkk0sakkinen]

[jarkk0sakkinen]

I guess I'll start working on this now.

[baconpaul]

Great!

We will almost definitely want to take the Surge::UserInteractions::promptError about unimplemented out of src/linux/DisplayInfoLinux when you do since that will over-pop error messages. We can just replace them with a silent default and a FIXME for now I think.

[baconpaul]

Over in #749 @tank-trax and @jsakkine propose a solution to this where we fork either Zenity or kdialog for now. I think both issues can close at the same time.

[baconpaul]

(@tavasti not @tank-trax sorry!)

[baconpaul]

We had a long discussion over in #749 which amounted to "we should do this right and not fork a sub process (mostly)". I made a comment about "do this like the about box" but that made me realize there's a super quick path here. Which is actually USE the about box.

So in

1. CAboutBox.h add an enum "AboutBoxMode" which has values "About", "Error", "OKCancel" and add a method "setAboutBoxMode(enum)"
2. in SurgeGUIEditor::showSettingsMenu where you do if(about box) aboutbox->show make it instead

  if(about box)
   {
     about box->setMode(CAboutBox::About)
    about box->show()
   }

(in pseudo code)

1. In UserInteractions promptError make "promptError" reach back to the GUIEditor and set the about box to error and show it. This may mean weaving an optional GUIEditor through the Surge::UserInteractions code paths, which is a bummer, since we don't always have an editor available (so there will be some errors Surge pops which still go to stderr if an editor hasn't started; and on mac and linux you will see them; the zenity solution would avoid this).

2. Modify CAboutBox::Draw to draw the right thing for prompt error

3. Repeat for OK Cancel just with the addition of a callback.

[jarkk0sakkinen]

@baconpaul, you are speaking of Zenity as it would be some kind of solution to something. It seriously isn't. For me it will break my whole development environment. I think I quite clearly showed the reasons why it should be avoided.

Examples:

• No package recipe in https://git.busybox.net/buildroot/tree/package. Impossible to fix and land my in-progress PRs unless I contribute a build recipe for Zenity.
• People who use other desktops than KDE and Gnome will hate the solution. There are Englitenment, FVWM, XFCE, i3 and whatnot. Installing GNOME libaries is too heavy hammer because the reason to start with for using those is to have a minimalist environment.

I'll give a shot and deriving an error dialog from CAboutBox to make something useful other than complaining. I try to make the point that this is not a matter of taste question for me but using Zenity is really a dead end.

[jarkk0sakkinen]

Anyway, I've spoken. Not going to comment about this as I've presented hard facts. Better to do some code now...

[baconpaul]

Yeah @jsakkine you just aren't reading my ifs correctly is all! I was never suggesting requiring zenity. Just using it if we had it and solving the immediate problem for almost all of our users.

But whatever works for you guys!

[jarkk0sakkinen]

... and I agree that my whining about Zenity is a bit over the top unless I have something else to give :) The reason being that now the coverage is zero. With Zenity it would increase from zero to something :) I'll get on work and try to feed a PR within early next week...

[tavasti]

@jsakkine do you want me to put together bash command line which calls zenity if available, and kdialog if not?

[jarkk0sakkinen]

@baconpaul, Started working on. I use CAboutBox as basis but create a new control. Less messy.

[baconpaul]

Yup that's fine too. Makes sense.

[jarkk0sakkinen]

Will put out PR during weekend (sorry for the latency).

[jarkk0sakkinen]

@baconpaul, I have a control that I would like to test with user interactions. What would be the best way to access SurgeGUIEditor instance from there. Can we make that class a singleton?

[jarkk0sakkinen]

(and sorry for the delays)

[jarkk0sakkinen]

The way it works that SurgeGUIEditor owns the CInteractionBox instance like it own the about box but UserInteractions should be able to get reference to the active SurgeGUIEditor instance somehow.

[jarkk0sakkinen]

@baconpaul, and I think it is good enough to have one gui editor and fallback to printing if it is not yet created.

[jarkk0sakkinen]

SurgeGUIEditor *SurgeGUIEditor::getInstance()
{
   return instance;
}

void SurgeGUIEditor::setInstance(SurgeGUIEditor *guiEditor)
{
   instance = guiEditor;
}

This what I'm planning to add. UserInteractions can use getInstance() to check whether to use instance or fallback to printing.

[jarkk0sakkinen]

I have something together. I wonder what would be easy use case to get either an error or prompt? @esaruoho ?

[baconpaul]

You can get a prompt by saving a patch twice. You can get an error by adding an error to the code!

That instance singleton is not going to work well though. If you have two instances of surge in one DLL which one gets picked? How do you stop the instance variable being clobbered (unless I guess you make it a std::atomic) but still if you have two tracks both with surges you have two editor instances in one memory space.

[baconpaul]

Another way to get an error is to remove ~/Library/Application Suport/Surge/patches_factory (or wherever they go on linux). That should generate an error at startup time.

[jarkk0sakkinen]

@baconpaul, OK, I get the problem, thanks. And yes, that is not easy to sort out. Is there anything making UserInteractions an object impossible? Just throwing ideas.

[baconpaul]

I think the thing to do is to add an optional SurgeGUIEditor * argument to each of the user interactions API and have it default to nullptr in the header. Mac and windows ignores it. Linux doesn’t.

Then at points where we pop an error and we have an editor instance we pass it in and linux uses it.

There are some cases for prompt error - especually early in the init path - where there is no editor instance. In that case we can use stderr or something else?

[jarkk0sakkinen]

Yeah, that sounds like a working plan!

[jarkk0sakkinen]

I was already worried about doing anything to UserInteractions given how widely it is already scattered. For some reason adding gui editor as parameter just didn't come to mind :) But yes, it is great idea..

[jarkk0sakkinen]

I'll post a screenshot as soon as I get the it visible. Then we can decide what the UX needs. Would be nice if it was kind of Surge themed optimally but anything that works is probably sufficient right now.

[jarkk0sakkinen]

Probably "save twice" is the best scenario start with as it is easy to reproduce? Now we can get this closed scenario by scenario.

[baconpaul]

I agree with that. Save Twice is the most pressing one right now. The rest of them are error paths which don’t happen as often.

[baconpaul]

If #839 builds and I merge it (which I plan to) there's one more API we need to support one day. It won't be used for core critical functions though in the near term (but will be used for the enharmonic small file reading stuff).

[baconpaul]

I stubbed in Zenity for most of the important interactions. We can take it out when the correct work contemplated here is done.