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

Make Linux UserInteractions use something other than fork or popen zenity #562

Closed
jarkkojs opened this issue Feb 10, 2019 · 30 comments
Closed
Labels
Feature Request New feature request Linux Issues which only occur on Linux

Comments

@jarkkojs
Copy link
Collaborator

No description provided.

@jarkkojs
Copy link
Collaborator Author

I guess I'll start working on this now.

@baconpaul baconpaul added the Linux Issues which only occur on Linux label Feb 10, 2019
@baconpaul
Copy link
Collaborator

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

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

(@tavasti not @tank-trax sorry!)

@baconpaul
Copy link
Collaborator

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.

baconpaul added a commit to baconpaul/surge that referenced this issue Mar 10, 2019
OKCancel is only used to chicken box overwritign a patch.
Neither default is great, but chosing OK means at least
the user action is not silently ignored.

Closes surge-synthesizer#749
Will be greatly improved by the correct implementation of surge-synthesizer#562
baconpaul added a commit that referenced this issue Mar 10, 2019
OKCancel is only used to chicken box overwritign a patch.
Neither default is great, but chosing OK means at least
the user action is not silently ignored.

Closes #749
Will be greatly improved by the correct implementation of #562
@jarkkojs
Copy link
Collaborator Author

jarkkojs commented Mar 10, 2019

@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.

@jarkkojs
Copy link
Collaborator Author

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

@baconpaul
Copy link
Collaborator

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!

@jarkkojs
Copy link
Collaborator Author

... 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
Copy link
Contributor

tavasti commented Mar 12, 2019

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

@jarkkojs
Copy link
Collaborator Author

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

@baconpaul
Copy link
Collaborator

Yup that's fine too. Makes sense.

@jarkkojs
Copy link
Collaborator Author

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

@jarkkojs
Copy link
Collaborator Author

@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?

@jarkkojs
Copy link
Collaborator Author

(and sorry for the delays)

@jarkkojs
Copy link
Collaborator Author

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.

@jarkkojs
Copy link
Collaborator Author

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

@jarkkojs
Copy link
Collaborator Author

jarkkojs commented Mar 20, 2019

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.

@jarkkojs
Copy link
Collaborator Author

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

@baconpaul
Copy link
Collaborator

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<SurgeGUIEditor*>) but still if you have two tracks both with surges you have two editor instances in one memory space.

@baconpaul
Copy link
Collaborator

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.

@jarkkojs
Copy link
Collaborator Author

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

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?

@jarkkojs
Copy link
Collaborator Author

Yeah, that sounds like a working plan!

@jarkkojs
Copy link
Collaborator Author

jarkkojs commented Mar 21, 2019

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..

@jarkkojs
Copy link
Collaborator Author

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.

@jarkkojs
Copy link
Collaborator Author

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

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

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 baconpaul modified the milestones: 1.6.0, 1.6.n Jun 10, 2019
baconpaul added a commit to baconpaul/surge that referenced this issue Jun 17, 2019
Surge UserInteractions on Linux should really use
the vst event loop and vstgui and be hand written because
otherwise you have to popen zenity which can cause audio
pauses and pops.

But audio pauses and pops on anciliary actions are better than
nothing, so stick in a crudy zenity implementation which is
a start, and someone inspired to remove it at least has a spec
in the future.

Addresses surge-synthesizer#562
Closes surge-synthesizer#637 and surge-synthesizer#917
@baconpaul baconpaul changed the title Make Linux UserInteractions use something other than std::cerr Make Linux UserInteractions use something other than fork or popen zenity Jun 17, 2019
@baconpaul baconpaul added this to the 1.6.n milestone Jun 17, 2019
@baconpaul
Copy link
Collaborator

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

baconpaul added a commit that referenced this issue Jun 17, 2019
Surge UserInteractions on Linux should really use
the vst event loop and vstgui and be hand written because
otherwise you have to popen zenity which can cause audio
pauses and pops.

But audio pauses and pops on anciliary actions are better than
nothing, so stick in a crudy zenity implementation which is
a start, and someone inspired to remove it at least has a spec
in the future.

Addresses #562
Closes #637 and #917
baconpaul added a commit to baconpaul/surge that referenced this issue Jul 10, 2019
OKCancel is only used to chicken box overwritign a patch.
Neither default is great, but chosing OK means at least
the user action is not silently ignored.

Closes surge-synthesizer#749
Will be greatly improved by the correct implementation of surge-synthesizer#562

Former-commit-id: ce10a4c1ba842ddc8fa92654c9505f53133db58d [formerly ee8dcca]
Former-commit-id: 3d535d44f84c4fd994dbd5696470b259bc276af9
Former-commit-id: bb6df1148a16c417842da807e30636e3c8f5b626
baconpaul added a commit to baconpaul/surge that referenced this issue Jul 10, 2019
Surge UserInteractions on Linux should really use
the vst event loop and vstgui and be hand written because
otherwise you have to popen zenity which can cause audio
pauses and pops.

But audio pauses and pops on anciliary actions are better than
nothing, so stick in a crudy zenity implementation which is
a start, and someone inspired to remove it at least has a spec
in the future.

Addresses surge-synthesizer#562
Closes surge-synthesizer#637 and surge-synthesizer#917

Former-commit-id: dd198acb7a6802ea285ad6e320e6720be85c8ebe [formerly 42b70bb]
Former-commit-id: 1140ae12866cf67a4bee6059949054862f9c13d8
Former-commit-id: 520cb368af46dad04c8b890f202014ef116c3561
@baconpaul baconpaul modified the milestones: 1.6.n, Currently Unscheduled Oct 4, 2019
@mkruselj mkruselj added the Feature Request New feature request label Nov 9, 2020
@mkruselj mkruselj removed this from the Currently Unscheduled milestone Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature request Linux Issues which only occur on Linux
Projects
None yet
Development

No branches or pull requests

4 participants