sequencer64 icon indicating copy to clipboard operation
sequencer64 copied to clipboard

Drop LASH support?

Open simonvanderveldt opened this issue 9 years ago • 2 comments

There's still some LASH code in the repo (lash.cpp and the matching lash.hpp), but the way I understand it LASH basically gives similar/the same capabilities as LADISH level 1 support using the SIGUSR signal, which is already implemented (see mainwnd.cpp#L2209 and mainwnd.cpp#L2251) and working.

The only difference I've figured out is that because a level 1 application is "managed" by LADISH by sending signals there's no way for LADISH to pass where the application should save it's file. This means you'll have to pass pass a filename as argument to sequencer64 to make it work. Otherwise you'll have no way to have sequencer64 automatically open the file when restoring the session. One issue with this is that sequencer64 doesn't handle passing a file that doesn't exist yet very well in that if you run for example sequencer64 seq64.midi you'll still get the "Save file" dialog on session save and the actual saving will fail. This means you'll have make sure the seq64.midi file exists before starting sequencer64.

This is different from LASH which creates directories for every application in the session and passes that path to the application when saving and restoring the session.

All of the above was triggered by the fact that restoring a LASH session fails for me with:

floating point exception  sequencer64

The same happens when I manually open the midi file saved by sequencer64 on a LASH save. So there's something broken there, since it's pretty much deprecated it might make more sense to get rid of it.

simonvanderveldt avatar Oct 23 '16 14:10 simonvanderveldt

I'm not sure about getting rid of LASH support; I don't use it myself, but it might mess up some users to remove it. I would tend to leave the code in place, but I would consider making LASH disabled by default if there is a case for it. I know that the Arch package for sequencer64 configures the build with --disable-lash.

In any case, I did find a bug in the LASH call to the midifile constructor, and added a couple fixes. Would you be willing to try again and let me know what works and doesn't work with the latest fixups commit? Thanks!

ahlstromcj avatar Oct 23 '16 18:10 ahlstromcj

I'm not sure about getting rid of LASH support; I don't use it myself, but it might mess up some users to remove it. I would tend to leave the code in place, but I would consider making LASH disabled by default if there is a case for it. I know that the Arch package for sequencer64 configures the build with --disable-lash.

That's understandable as well. I doubt there are a lot of people of people left using liblash though, since LADISH does more, all GUI front-ends switched to LADISH and the command-line ladish_control offers a lot more options than lash_control ever did.

I haven't tried to build it but I doubt the only remaining GUI front-end for LASH, the one that's included with it, will actually work, it's gtk2 only and the code hasn't been touched in more than 7 years. (I don't think untouched code in itself is an issue, but if it relies on external libraries that get replaced it does become a problem). Anyway, it's obviously your call :) Was just thinking less code -> less stuff to maintain, especially if you don't use it yourself :)

In any case, I did find a bug in the LASH call to the midifile constructor, and added a couple fixes. Would you be willing to try again and let me know what works and doesn't work with the latest fixups commit? Thanks!

I just built the fixups branch and tried it, the LASH stuff works fine now. Thanks for the quick fix! :) There's only one thing standing out: sequencer64 doesn't show the filename in the title bar when used in a LASH session. This doesn't happen when using sequencer64 normally/within a LADISH session. image

P.S. If you reference an issue in a commit message using the # notation (like #49) GitHub will make it a link when browsing the commits and also add a link to that commit in the referenced issue. See https://guides.github.com/features/issues/#notifications and https://help.github.com/articles/closing-issues-via-commit-messages/

P.P.S. I've created an ebuild to install sequencer64 in gentoo which I'll be pushing to my overlay shortly so others can use it as well :)

simonvanderveldt avatar Oct 23 '16 20:10 simonvanderveldt