Surge OkCancel dialog not shown in Linux, preventing re-saving preset

[tavasti]

Describe the bug
When saving preset again with same name, it will fail silently. If using Surge with Ardour, I can see on console:

Surge OkCancel
Overwrite patch
The patch 'Testi' already exists in 'Taw'. Are you sure you want to overwrite it?
Returning CANCEL

To Reproduce

• Save patch
• Change some setting
• Save again with same name and category
• Reload saved patch -> getting settings of first save

Desktop (please complete the following information):

• OS: Linux (Ubuntu 18.04)
• Host: carla, ardour (in carla you can't see messages, but behaviour is identical)
• Version: git commit 6a0dae7 (latest)

[baconpaul]

This is actually kind of a dup of #562 - the linux user interactions are not implemented.

When i stubbed them in to get the port going I made OK Cancel choose “cancel” which is a conservative choice. I could be convinced to switch it to OK because of the behavior here. The save dialog is really the only place OK cancel issued in earnest. So if you want to change it to return OK in src/Linux/UserInteractionsLinux.cpp I would be OK to merge that (and we could then close this issue and focus on #562)

But the real answer is for someone to implement those user interactions. @jsakkine had threatened to look at it one day.

[tavasti]

Both options, defaulting to cancel and defaulting to ok cause data loss. Defaulting to cancel will cause them more often, but defaulting to ok will more likely to cause loosing some preset you don't remember anymore how to create it. Bit hard to pick which of these is worse.

[baconpaul]

Yeah I agree. The best thing is to implement that dialog on linux. but that's tricky.

Like I said: the only place the dialog is used right now is in the save chicken box so I'm OK for it to default to OK with a comment for now. I think it is more likely you would click OK than Cancel if the dialog appeared. But really your choice. Why not chat with @tank-trax who also has done a lot of linux testing and stuff and if you guys agree I'm fine with a change.

[jarkkojs]

I'll finish the things that I have in WiP ATM (wavetable work and ARM to be exact). If this is still undone after that, I can push it further. I know it is high priority but I do not want to cumulate anymore unfinished tasks.

[baconpaul]

I think the hard part is gonna be that since Linux kinda isn’t a desktop os api set like Mac and win are, the implied modal dialog in ok cancel is going to be hard to port. Moving ok cancel to a callback structure may be how we have to do this which could be a bit painful at this point in the code path

[jarkkojs]

@baconpaul, It could be engineered with a bit of a stretch: fork a process to show the dialog and use the exit code for that process to IPC the interaction. To make it clean we would it'd be better to have a surge_dialog type of application. Again, I can use the trick that I've already used twice to bundle this into the plugin binary. There is no direct syscall in Linux to exec from memory but by using memfd_create() one can create a virtual file to procfs that I can execve. In that helper it isn't a problem to use lets say GTK3.

So yes doable with some stretch :) But yeah, do not want to start with that before landing my previous items.

[jarkkojs]

@baconpaul, If we had surge_interaction (probably better name than dialog), what command-line arguments it should take?

[tavasti]

@jsakkine Check out https://github.com/GNOME/zenity

[baconpaul]

Title and message are the arguments - just two strings. And either present a choice or ok cancel or just an ok

[baconpaul]

https://github.com/KDE/kdialog

Seems like a kde version of same. It actually implements the curses dialog command line so is super easy to fork

Wonder if there is a way for the deb file to figure out which dependency to add. So “if you are kde add dep a; if you are gnome add dep b”

But don’t think we need to write a forkable dialog. Good thought @tank-trax

In and remember @jsakkine - it’s vfork not fork :)

[baconpaul]

If fact if you read

http://www.linux-magazine.com/Issues/2009/99/Zenity-and-KDialog

It seems that either kdialog or Zenity will be installed. So I propose we check for one then the other in the path and fork accordingly.

The fork/waitpid will hang the ui thread but the audio thread should still keep going. So it’s at least an improvement.

I’m gonna add a comment to #562 also

[baconpaul]

Oh and I typed @tank-trax and should have said @tavasti !! Oops sorry folks.

[jarkkojs]

@tavasti, @baconpaul: not supporting that solution. GTK would be a weaker dependency in the end. Practically all KDE based systems have GTK installed. If I want to scale down to a BuildRoot image, I would have to compile enormous amount of dependencies compared to plain GTK. It pulls in half of Gnome in. I would probably give up with the ARM work at this point if we do this because things don't scale nicely down anymore.

[jarkkojs]

It is like going from bad to worse: everybody has GTK installed and still we want to add KDE and/or GNOME to the stack. This is not going to work out well.

[baconpaul]

Zenity is installed in my ubuntu 18

[baconpaul]

Also why does this change buldroot?

We’re suggssting

vfork
Exec(zenity —error)

Type thing

[baconpaul]

Basically: I think @tavasti was suggesting fork this for the implementation of promptError and then waitpid it rather than writing our own thing. And if zenity isn't installed, kdialog will be; and if neither is installed fall back to cerr.

[baconpaul]

So: Do exactly your proposal; but don't roll our own surge_dialog; use one of the two which will be installed instead.

[jarkkojs]

Vim is installed on my system but we do not require Vim do we? Being installed in your system is not an argument.

[jarkkojs]

@baconpaul, Just look at ldd /usr/bin/zenity output. It would be insane to use it. Should be obvious from that why it does not play well with building custom Linux images. You have compile all those dependencies (and their dependencies) every time you create a BuildRoot image.

How have you verified that XFCE, Enlightenment, i3 etc. desktops have either installed? You rely on a random application based on a 10 year old article instead of a library that exists practically in all Linux installations.

[jarkkojs]

I'm not saying my solution is great but at least it is sustainable. If you want to look for alternatives, maybe they can be found from other plugins. Would not hurry with the implementation. Doesn't VSTGUI have any support for dialogs?

[jarkkojs]

There is no even build recipe for BuildRoot for Zenity, which does not come to me as a surprise. BuildRoot does not support the use of Zenity at all right now. If I this easily bump into environment that does not support the approach, how likely do you think that others will not?

[baconpaul]

Whatever you guys like. Right now linux users can’t save a patch twice. So we can improve that immediately. If zenity isn’t there you could fall back to current behavior and have basically ever active user get desired behavior in 20 lines of code.

I didn’t say require zenity. I said if it is there use it.

But hey I use Mac. You guys figure out if saving patches is a feature Linux users need or not. I’m happy to review changes if you have em and would merge an if zentity fork else stderr diff

[baconpaul]

OH and tp amswer upper other question: VSTGUI doesn't have dialogs. Has some modal support but it is flaky.What you could do is copy CAboutBox to cover the entire UI with a dialog, draw it with vstgui, and then turn the code into callback style rather than modal style. That's probably better than writing your own subordinate process from scratch - less work I think. Basically just follow the CAboutBox example.

[jarkkojs]

@baconpaul, this is not really a question what I like or do not like. Using Zenity will not work at all. I would have to stop ARM work completely if we choose that just to name an example.

I guess CControl is worth of pursuing...

[jarkkojs]

OK, it seems I have to postpone my other PRs. because this cannot really go like this. I'll try to get something available with next couple of days.

[baconpaul]

I’m not sure why checking if Zenity is in your path would break ARM. ARM machines can check their path for an executable right?

[baconpaul]

Also: Tjhere’s no rush here. If we just change the default to “OK” then it means you can save but you don’t get a chicken box at overwrite.

I think rather than dropping stuff I would change to OK for now. Moving to CDialog is going to have changes which modify code on mac and win also. It’s not easy (again because of modality).

[baconpaul]

Oooh actually I just had a clever idea about implementing this using exactly the about box. I'll update the other issue.

Here's what I propose

1. I'm going to change the linux OK Cancel default to "OK". This means at least you overwrite patches if you save them twice. The only place OK Cancel is currently used is in this. Then I will close this issue

2. I'm gonna write up my clever note on using exactly the about box in Make Linux UserInteractions use something other than fork or popen zenity #562. Then someone can code it if they want!

[baconpaul]

Oh and my first item takes some of the time pressure off this issue, @jsakkine

[jarkkojs]

@baconpaul, well I build my test images straight from the source code and build recipe for Zenity does not exist in BuildRoot. I cannot build binary for Zenity.

And by requiring Zenity you enfornce people who use lightweight desktops to install half of GNOME libraries rather being fine with the GTK DLL that they already have installed. Do we want to enforce people to install huge portion of the desktop that they don't use?

Especially in audio production I'd go for lightweight desktop to minimize the side-effect in a studio environment, and if Surge required me to pull enormous GNOME dependencies I would ignore the whole plugin. You are looking at just Zenity and not its dependencies. That is where the dilation comes from.

[baconpaul]

I understand your point fully @jsakkine - just you are missing mine!

We could - right now - write code which looks like this

  if( vfork() )
     if( excelp( "zenity", ) )
         exit(254)
  res = waitpid
  if (res == 254 )
     std::err << "You don't have zenity- I made this choice for you"
  else
     return (result consistent with zenity)

then you don't have to install it at all. All the conversation about "if zenity else if KDialog else if fall back" I feel like either I'm not saying loudly enough or you aren't hearing! I was never suggesting requiring gnome. Just using it if you happen to have it! (and KDE if you happen to have that).

And I was assuming that all of our users today have either gnome or KDE so the only person who falls into the cerr if is your ARM port.

So everything would just work.

See?