ZynAddSubFX can't edit preset

[tresf]

A few symptoms are present in ZynAddSubFX when running from .AppImage.

I'm assigning @lukas-w based on my code review below.

• Zyn exports as sine-wave (originally reported by seven#9849 on Discord here)
• Cannot see or edit banks (reproduced here: Add AppImage support to Linux builds #3688 (comment))

There's a chance we are creating this blank zyn bank bug with the new data:/ syntax. Suspected because .zynaddsubfxXML.cfg shows a line in it that says:

<BANKROOT id="0">
   <string name="bank_root">~/banks</string>
  </BANKROOT>
  <BANKROOT id="1">
   <string name="bank_root">./</string>
  </BANKROOT>
  <BANKROOT id="2">
   <string name="bank_root">/usr/share/zynaddsubfx/banks</string>
  </BANKROOT>
  <BANKROOT id="3">
   <string name="bank_root">/usr/local/share/zynaddsubfx/banks</string>
  </BANKROOT>
  <BANKROOT id="4">
   <string name="bank_root">../banks</string>
  </BANKROOT>
  <BANKROOT id="5">
   <string name="bank_root">banks</string>
  </BANKROOT>
  <BANKROOT id="6">
-   <string name="bank_root">data:/presets//ZynAddSubFX</string>
  </BANKROOT>

I can confirm that patching ZynAddSubFX helps. Here's my proof of concept code, which I believe fixes a regression from #1719.

diff --git a/plugins/zynaddsubfx/ZynAddSubFx.cpp b/plugins/zynaddsubfx/ZynAddSubFx.cpp
index 47da514..a4ab71b 100644
--- a/plugins/zynaddsubfx/ZynAddSubFx.cpp
+++ b/plugins/zynaddsubfx/ZynAddSubFx.cpp
@@ -28,6 +28,7 @@
 #include <QDomDocument>
 #include <QTemporaryFile>
 #include <QDropEvent>
+#include <QFileInfo>
 #include <QGridLayout>
 #include <QPushButton>
 
@@ -440,13 +441,13 @@ void ZynAddSubFxInstrument::initPlugin()
                        RemotePlugin::message( IdZasfLmmsWorkingDirectory ).
                                addString(
                                        QSTR_TO_STDSTR(
-                                               QString( ConfigManager::inst()->workingDir() ) ) ) );
+                                               QFileInfo(QString( ConfigManager::inst()->workingDir() ) ).absoluteFilePat
                m_remotePlugin->sendMessage(
                        RemotePlugin::message( IdZasfPresetDirectory ).
                                addString(
                                        QSTR_TO_STDSTR(
-                                               QString( ConfigManager::inst()->factoryPresetsDir() +
-                                                               QDir::separator() + "ZynAddSubFX" ) ) ) );
+                                               QFileInfo(QString( ConfigManager::inst()->factoryPresetsDir() +
+                                                               QDir::separator() + "ZynAddSubFX" ) ).absoluteFilePath() )
 
                m_remotePlugin->updateSampleRate( Engine::mixer()->processingSampleRate() );

The bank selection is still a bit buggy like #1001, but this fixes it so that it works with the checkbox workaround.

Related: #1001, #3688
Related in description: #3855, #2865, #2662

[lukas-w]

The fix appears to be fine. 👍

A couple of notes though:

• I can't create a new bank with or without this patch. An error dialog pops up saying

  Error: Could not make a new bank (directory)..

  The error text appears truncated (can't make a screenshot due to a VirtualBox bug)

• workingDir() should return a proper path, so I think the first change is unnecessary.

• This is not your fault, but the use of QDir::separator() is incorrect. Qt functions expect / as separator on all OSs, not the native one.

• In theory, data:/presets/ZynAddSubFx could resolve to more than one absolute file paths. If we want to respect that, we'd need to loop through QDir::searchPaths("data") and add all preset paths one by one. But this is a situation that will probably never occur in a normal installation, especially not in an AppImage. So I'd say it's too much work, but it may be worth keeping it in mind.

• Albeit being equivalent, QDir(…).absolutePath() instead of QFileInfo(…).absoluteFilePath() is probably a bit more readable (since we're handling directories here, not files).

Sorry for the late feedback. In the future, feel free to just send a couple more mails my way if I don't answer soon enough. Chances are I just forgot.

[musikBear]

Just FYI(win32 xp3
rc3 crashed when zasfx UI was opened, but rc4 does not

nb deepest setting AbstrSine will still crash zasfx, but not lmms!
After this component-crash the zasfx instrument only need to be refreshed with a new preset, and it works again, so in fact the only thing that does crash, is the actual zasfx-internal preset!
Imo, that point at zasfx-dev. eg a not our bug situation

[tresf]

@lukas-w how do you want to proceed with this? I can patch Zyn in this one area as a stop-gap, but I feel a full review of #1719 may be more proper, and this review is not something I'm prepared to make.

[lukas-w]

Well as long as we don't revert #1719 we'll have to patch Zyn. I'll do that and open a PR in a minute.

There could be other places that were broken by #1719, potentially all places where non-Qt functions are used to access "data:" paths. We could see if we can find any more of them by searching for uses of ConfigManager functions such as factoryPresetsDir().

[tresf]

We could see if we can find any more of them by searching for uses of ConfigManager functions such as factoryPresetsDir().

I did a quick check on some other plugins and can't find another offender. For example, VSTs use some String values, but I couldn't find any use of foo: style paths. Plugins seemed ok at a glance too . We'll just open a new bug report if this surfaces somewhere else.

[tresf]

As @pikachuk reported on Discord, new banks still cannot be created through Zyn with the AppImage. Reopening.

Steps:

1. Drag Zyn instrument into song editor
2. Open instrument, click "Show GUI"
3. Click "Advanced"
4. From the top menu, click "Instrument, Show Instrument Bank"
5. Click "New Bank" button in upper right.

[tresf]

Just tested 1.1.3 from Ubuntu mirrors and same issue occurs, so the "New Bank" issue is not a regression with the AppImage. @pikachuk please open a new bug report so that we can milestone this appropriately.