Correctly use the ladspa plugin folder #2190
Conversation
curlymorphic
changed the title
| { | ||
| #if defined(LMMS_BUILD_WIN32) | ||
| m_ladDir = qApp->applicationDirPath() + "/plugins/ladspa" + QDir::separator(); | ||
| #elif defined(LMMS_BUILD_APPLE) | ||
| m_ladDir = qApp->applicationDirPath() + "/../lib/lmms/ladspa/"; | ||
| #else | ||
| m_ladDir = qApp->applicationDirPath() + '/' + LIB_DIR + "/ladspa/"; | ||
| m_ladDir = qApp->applicationDirPath() + '/' + LIB_DIR + "lmms/ladspa/"; |
There was a problem hiding this comment.
I don't understand this fix well enough to critique it, but for consistency's sake, is there a reason we're using lmms/ladspa/ rather than /lmms/ladspa/ ?
There was a problem hiding this comment.
The``LIB_DIR` is terminated with a separator, I initially used /lmms/ladspa/ but this resulted in a path containing a double /.
There was a problem hiding this comment.
Fair enough, but should it? Is this where we set it?
https://github.com/LMMS/lmms/blob/master/src/CMakeLists.txt#L41
There was a problem hiding this comment.
Is this where we set it?
I belive so,
but should it?
IMO paths end with a trailing / (separator), filenames don't.
We could change LIB_DIR to not include the trailing separator, but that would involve changes across the codebase, outside the scope of this pull request. I am happy to work on that as a separate PR if required?
There was a problem hiding this comment.
FWIW, all major OSes support double (or triple, etc) -slashes in paths. There's also QDir::cleanPath to strip redundant "/", "." and ".." if desired for formatting purposes.
It may be worth concatenating with an extra "/" for consistency or just in case LIB_DIR is ever modified to not have a trailing slash and then cleaning it with cleanPath afterward. This would also fix the display on apple caused by using ".." in qApp->applicationDirPath() + "/../lib/lmms/ladspa/" (e.g. /path/to/bin/../lib/lmms/ladspa would become /path/to/lib/lmms/ladspa)
There was a problem hiding this comment.
Totally worth the extra slash IMO. It never hurts to have it, but to not have it might cause trouble.
There was a problem hiding this comment.
Will do this when im back at my pc, as this seem to be the consensus.
|
Nice! Looking forward to trying this out when I get home.
|
The default Ladspa plugin folder was incorrect under linux This pull request fixes this. Setting Dialog, Ladspa Directory Added leading / Added a leading / to the lmms/ladspa plugin Clean the path using QDir::cleanPath()
|
Added the Leading / to /lmms/ladspa and cleaned up the path using |
Correctly use the ladspa plugin folder
The config manager was not using the loaded value for the ladspa folder
The default Ladspa plugin folder was incorrect under linux
This pull request fixes this.
Fixes #2159 Master branch can't find ladspa effects
Were introduced in #1908 Re organizing of the user lmms directory