Lmms: 3 new minor developer features for LMMS

[JohannesLorenz]

I'm on my personal LMMS fork (it basically supports a new plugin for zyn) and would like to extract up to 3 minor features and move them over to LMMS. All features are only interesting for developers and have no "visual" effect:

• [X] Add doc/automation.md , explaining how LMMS' automation basically works (classes and widgets). (Done via 27fd8d0)
• [x] Add a simple XPMLoader class to embed.h, inheriting PixmapLoader (it only has an xpm array as member, and a pixmap() override which returns it as QPixmap) (Per #4605)
• [x] Functionality in src/core/AutomatableModel.cpp to load and save models quoted if they do not match ^[A-Za-z0-9._-]+$ (instead of <modelname ...>, they are stored as <quoted-model quoted-name="modelname" ...>). For example, this allows "save" saving of OSC string models, like "/volume". (Per #4605)

Can you please let me know which I can merge to master? I'm not sad if any/all are rejected, but I'd find it cleaner if this code is being reused by master.

[tresf]

Add doc/automation.md , explaining how LMMS' automation basically works (classes and widgets)

Please provide a Link. I can't find this onJohannesLorenz/lmms on the master branch.
My general opinion is that documentation should normally exist in the wiki but there are occasions when it makes more sense in a folder that houses that particular content. I'd like to see the wiki enhanced which is in itself a github repository. We need better tutorials on how LMMS works.

Add a simple XPMLoader class to embed.h, inheriting PixmapLoader (it only has an xpm array as member, and a pixmap() override which returns it as QPixmap)

I'm not qualified to offer feedback although anything that makes zyn 3 integration easier is going to be a 👍 from me.

Functionality in src/core/AutomatableModel.cpp to load and save models quoted if they do not match ^[A-Za-z0-9._-]+$ (instead of

I believe we discussed this on discord about the limitation of our current automatable model in XML and how they need to be enhanced to support some non-valid DOM tag elements. This seems justified although I'm not sure that the word quoted is needed, that's purely a cosmetic decision and should not influence the project's need for handling more complex names. 👍

[PhysSong]

Please provide a Link. I can't find this on JohannesLorenz/lmms on the master branch.

You need to look at the osc-plugin branch: https://github.com/JohannesLorenz/lmms/blob/osc-plugin/doc/automation.md
The branch has all those mentioned features, too.

[JohannesLorenz]

Sorry, I didn't know that this branch was even pushed in that state. Anyways, for reference:
XPMLoader: https://github.com/JohannesLorenz/lmms/blob/osc-plugin/include/embed.h#L94-L104
AutomatableModel: https://github.com/JohannesLorenz/lmms/blob/osc-plugin/src/core/AutomatableModel.cpp#L94-L229

@tresf For the AutomatableModel, I agree, "model" sounds better. I'll add some lines that find out whether the name of the model was "model", or whether it was quoted. Then it should work.

[JohannesLorenz]

Proposal from gi0e5b06 on Discord:

quoted-name -> name
quoted-model -> automatablemodel

I think that fits even better.

[tresf]

XPMLoader: https://github.com/JohannesLorenz/lmms/blob/osc-plugin/include/embed.h#L94-L104

Forgive my lack of understanding and cosmetic critique here but doesn't C++ allow multiple constructors? Is there a reason why PluginPixmapLoader( const char** xpm ) is undesired?

@tresf For the AutomatableModel, I agree, "model" sounds better. I'll add some lines that find out whether the name of the model was "model", or whether it was quoted. Then it should work.

👍

You need to look at the osc-plugin branch: https://github.com/JohannesLorenz/lmms/blob/osc-plugin/doc/automation.md The branch has all those mentioned features, too.

Thanks for the link. I still strongly feel this belongs in the wiki. A readme.md feels natural in cases where a small folder can have some documentation inside it and it would be browsable through the web. I've seen this in some other projects.

If the goal is to ship LMMS with all docs needed for development in a tarball, then we should add a source tarball that bundled the wiki docs in either as a batch or a submodule so that developers have everything needed to build it. This may make sense for plugins as well. A common complaint we get is that LMMS can't build from tarball anymore so this would fall right in alignment. It's also a requirement for some projects like Homebrew that don't want packages relying on volatile links within the build system. Related: #4100, #4229, #4400.

[JohannesLorenz]

Forgive my lack of understanding and cosmetic critique here but doesn't C++ allow multiple constructors? Is there a reason why PluginPixmapLoader( const char** xpm ) is undesired?

@tresf It wouldbe doable, but it would force merging PluginPixmapLoader and XPMloader: PluginPixmapLoader would get a const char** xpm member, and its pixmap function would return different things depending on whether xpm == nullptr. Doable, but seperating the classes looks like better design to me.

Unless you had something different in mind?

[PhysSong]

Doable, but seperating the classes looks like better design to me.

I don't know how will you use the XPMLoader, but I can't find any reason for merging them.

[JohannesLorenz]

It's indeed a bit weird to have code in master that's not used. Somehow, I think this should not be merged into master yet. I'll skip this class, it's just a few lines anyways.

[tresf]

Sorry, I meant PixmapLoader (not PluginPixmapLoader) but aren't you already adding a potential for pixmapName(...) to be undefined since you're inheriting it from PixmapLoader?

Regardless, I'm just curious why you've made a near-identical function to PixmapLoader but choses to name it XPMLoader. They appear to do near identical things and they both return a QPixmap type. I would expect the nomenclature to reflect that.

its pixmap function would return different things depending on whether xpm == nullptr

By different things, what do you mean? We get a default constructed QPixmap() in the other case. What does char * * do with a nullptr?

The source code of qpixmap.cpp shows this:

QPixmap::QPixmap(const char * const xpm[]) : QPaintDevice() {
   doInit(0, 0, QPlatformPixmap::PixmapType);
   if (!xpm)
      return;
      ... 

It's not a strong enough objection, just trying to keep our naming convention consistent.

[JohannesLorenz]

Pushed a branch for the AutomatableModel changes.

We talked about it already, but I'd appreciate if you could do a short review of the code (mostly the loading routine).

[JohannesLorenz]

I found a more elegant solution for QPixmapLoader. More elegant because it does not store the XPM inside the PixmapLoader, but (like all the other image formats) inside Qt's pixmap cache.

The complete changes are in the PR above. It would be nice if you could think about whether this should be merged to master or not.

[tresf]

1. Add doc/automation.md , explaining how LMMS' automation basically works (classes and widgets)

Would you like this added to https://github.com/LMMS/lmms/pull/4588?

[JohannesLorenz]

@tresf Already added using the github website.

[tresf]

@tresf Already added using the github website.

Thanks. Crosslinked to home page, bumped, merged and crossed off on your list above. Is there a PR open for the XPMLoader? I like the new technique but I'll defer both remaining items to @PhysSong if he has the bandwidth available for review.

[JohannesLorenz]

Thanks for adding the submodule!

There are two PRs. Sorry, I thought it would have linked them:

• AutomatableModel
• XPMs

[JohannesLorenz]

Fixed with the wiki entry and the commits above.