Move core and gui classes into a 'lmms' namespace

[Wallacoloo]

This change places all classes necessary to build lmms (i.e. everything in the src directory) into a namespace named "lmms". Notably, plugins are not namespaced, which made sense to me because many plugins (e.g. zynaddsubfx) aren't specific to lmms.

The motivation for this change is to fix #2049, as well as #2053, #2035 and possibly some other bugs that arose due to having one of the plugin classes (zynaddsubfx's Engine class) sharing the same name, and therefore vtable, as an lmms class.

It's a pretty significant change, so I think it deserves some testing. Note that this does break theme compatibility (see #2049 for an explanation of why). If the default theme still appears broken for you, open settings and make sure lmms isn't accidentally using an outdated version of it (e.g. in my case, I have a stable build of lmms in /usr/local and I install dev builds to ~/dev/lmms, but the dev builds will default to using the theme in /usr/local/share unless I explicitly specify it).

[jayay]

👍 for your work!

[tresf]

I'm pinging @lukas-w for this as he authored our last major refactoring project.

Albeit noble, this may be a bit of overkill. From my understanding this was inspired by a namespace conflict with Zyn, however modifying 400+ files in reaction to poor use of a class name "Engine" on a remote process I find to be slightly drastic, given the impact this creates.

[lukas-w]

I agree this is overkill. Having a namespace around LMMS's code can prevent issues like this and perhaps it should have been done since the beginning.
But doing it now is a huge undertaking that may cause more harm than good. We should check for other less invasive ways to fix this.

[Wallacoloo]

Strictly from a design point of view, is it desirable to have LMMS's code in its own namespace? Because if it is desirable, then I believe that it should happen. I don't see much of a reason to hold off on an improvement just because it'll take a lot of work, especially when a lot of that work has already been done.

Also, moving lmms into a namespace was already discussed 2 weeks ago and nobody raised any complaints about it. Tresf even specifically called out to you @lukas-w. I don't mean to antagonize, but frankly, people had plenty of time to voice any concerns about this beforehand and nobody did.

[eagles051387]

I think there is a bit of a middle ground here. Why not do this on its own
branch instead of on the mainline development branch?

On Mon, May 25, 2015 at 6:32 AM, Colin Wallace notifications@github.com
wrote:

Strictly from a design point of view, is it desirable to have LMMS's code
in its own namespace? Because if it is desirable, then I believe that it
should happen. I don't see much of a reason to hold off on an improvement
just because it'll take a lot of work, especially when a lot of that work
has already been done.

Also, moving lmms into a namespace was already discussed 2 weeks ago
#2049 (comment) and
nobody raised any complaints about it. Tresf even specifically called out
to you @lukas-w https://github.com/Lukas-W. I don't mean to antagonize,
but frankly, people had plenty of time to voice any concerns about this
beforehand and nobody did.

—
Reply to this email directly or view it on GitHub
#2064 (comment).

Jonathan Aquilina

[Wallacoloo]

@eagles051387 Can you clarify your suggestion? This is technically already on its own branch at Wallacoloo/namespaced. Do you mean to just keep these branches permanently separate? If so, what is gained by that?

[eagles051387]

For now keep them seperate then once all the name space work is complete
merge them back into the master branch.

On Mon, May 25, 2015 at 6:55 AM, Colin Wallace notifications@github.com
wrote:

@eagles051387 https://github.com/eagles051387 Can you clarify your
suggestion? This is technically already on its own branch at
Wallacoloo/namespaced. Do you mean to just keep these branches permanently
separate? If so, what is gained by that?

—
Reply to this email directly or view it on GitHub
#2064 (comment).

Jonathan Aquilina

[Wallacoloo]

@eagles051387 all the namespace work is done - it just needs peer review and testing.

[tresf]

@Wallacoloo ignore @eagles051387 please. I personally can't offer much more on the namespace conversation and I think my opinions have been voiced. 👍

[Wallacoloo]

In that case, what would people objecting to the namespace change recommend as an alternative for fixing issue #2049 and others? The only other decent approach I can think of is renaming LMMS' Engine class to something else. But just like the approach in this PR, that's going to affect a large number of files. Maybe not 467, but it could easily be over 100 (the text "Engine" occurs in 219 source files).

[tresf]

@Wallacoloo I'm not sure. Mark did say:

One of them needs to be renamed or namespaced

One of them being Zyn's code and one of them being LMMS's code. Perhaps when he sees the amount of effort this will be, he'll offer to rename it on his end. At a glance, I think it would affect about 7 files in the Zyn repository, but that's just a quick estimate. But I'd still be in favor of a rename over the namespace change, even if it's just for readability. :)

[Spekular]

I'm not an expert by any means but I can't really see any drawbacks to namespacing LMMS aside from the work, which is already done.

[Umcaruje]

@Wallacoloo you have a typo here.

[diizy]

In any case we're under feature freeze so something like this shouldn't be merged for 1.2. Bugfixes only...

[Wallacoloo]

@diizy this is a bug fix for #2049 / #2053 / #2035. If those bug aren't fixed before shipping 1.2, then LMMS will segfault after exporting any songs that use ZynAddSubFX or during the shutdown routine after having opened an instance of ZynAddSubFX.

@Umcaruje thanks for catching that. I fixed it and reviewed the rest of style.css - should be good now.

For all opposed to the idea of large changesets, I've thought of a less-intrusive way to fix these bugs. Engine used to only be used as a static class (i.e. never instantiated). Back then, we never had the conflicting vtable issue for lmms' Engine and ZynAddSubFx's Engine class, since the lmms' Engine class didn't need a vtable.

If we revert the changes from #1915 which added virtual and non-static methods to the Engine class, we can fix this issue by only modifying about 25 lines throughout Engine.h, Engine.cpp and GuiApplication.cpp, at the expense of less descriptive splash-screen initialization messages.

I still believe namespacing lmms does more good than harm, but I'm willing to compromise here. Would those who have raised concerns over this PR be in favor of the approach outlined above?

[Spekular]

Oh right, releases. Then I vote we wait until after 1.2 to do this.

[tresf]

@curlymorphic did we determine Zyn 2.5 suffers this same bug?

Let's please remember, this change can be made in Zyn rather than LMMS (although it would make it slightly more difficult to sync with upstream changes).

-Tres

[curlymorphic]

I do think lmms should be in it's own name space, I feel improving our code base is much preferable to trying to change upstream code, whenever we get a name clash,

In regards to ZSAF I have already wrapped the Effect class in a name space and looks like i will need to do similar for Engine, but this is hacks, I feel If im putting some classes in a name space, all the classes that belong there should be moved to the name space.

The question is not one of effort, but more where our efforts should go, Our code base, and it's longevity , or into changing upstream code where the changes may or may not be merged upstream potentially causing difficulties?

[tresf]

If London has a park named after Hemingway and Reddington, called H&R Block and a global tax preparation service with the same name H&R Block moves in with a conflicting business name/street address, how is it handled?

• Does the global "H&R Block" rename for a single city?
• Does the local "H&R Block" put the name "London" in front of all of their signs/slogans, so they become "London H&R Block"?

In most cases, the smaller party will take the hit and make the adjustment.

If Zyn is a plugin whom could benefit from namespacing anyway (since by design it is intended to be called upon by 3rd party software -- whereas LMMS is not), then I still say we entertain the idea of changing it there... i.e. we ask Mark. The worst he can say is "No". :)

[Wallacoloo]

This issue needs to be dealt with. I cannot even export my songs right now because of the name conflicts (LMMS segfaults before beginning the export because of a call to ZynAddSubFxInstrument::reloadPlugin())

@tresf you mentioned asking Mark. Can you take care of that?

[tresf]

@fundamental, we're in a bit of a pickle with the namespace convo... Feedback welcome.

[fundamental]

I'd say merging this pull request should just happen as it does seem to resolve things by the looks of it. As per zyn, I can see renaming a class or two, but I'm not interested in adding a full project namespace (though I am very much interested in getting other changes needed to fixup the project in the 2.5.0 merge pull request). While it's certainly used as an app which can be embedded into others it wasn't designed for this purpose, it was mostly organically grown into what it has become.

[curlymorphic]

It looks like this Engine name clash is casing trouble with the Zyn 2.5 upgrade, when I rebase against master. I am going to add the Zyn Engine Class to the Zyn Namespace, the same as was done for effect.

I am in favour of this pull request, but I feel timing and testing needs looking at on such a large change.

[tresf]

I still think this is overkill. Name-spacing our entire project for a single plugin conflict seems like huge overkill...

[fundamental]

@tresf
It's a design style choice. With the 2.5.0 merge this issue disappears, but it could crop up again in the future. Some people really like the extra structure and organization that they can provide, though it can devolve into lmms::userInterface::graphics::roundShapedStuff::Knob as can be observed in some of the boost code out there. I'd say this sort of extra structure might be helpful to lmms, though it can be obtained through a number of different methods. With that said, those other methods aren't already implemented with a pull request.

[tresf]

as can be observed in some of the boost code out there

Right. Since boost was written with the intent of being a dependency of other projects, their decision makes perfect sense. Since LMMS historically hasn't really had this 3rd-party-dependency type of audience (on top of the volatility of our API) I just feel like this yields nearly no benefit.

Also, isn't this caused by the way we run Zyn as a remote process? We don't do that with most of our 3rd party code (we instead run it natively under the LMMS process), so what are the risks in not namespacing our code? For example, if we add LV2 support, could this become an issue again? What are the use-cases where namespacing the project will help us out long term?

[tresf]

@Wallacoloo, I took a look at our Engine class and it appears to just be a small container class which holds pointers to the rest of our GUI and DSP items. Shall we give it a new name? I can see as time goes on, more and more items are suffering from the issues described in this thread.

[curlymorphic]

@tresf sorry I missed this question

Also, isn't this caused by the way we run Zyn as a remote process? We don't do that with most of our 3rd party code (we instead run it natively under the LMMS process), so what are the risks in not namespacing our code? For example, if we add LV2 support, could this become an issue again? What are the use-cases where namespacing the project will help us out long term?

The issue is caused because we dont run ZASF as a remote process, unless the ZSAF gui is opened. There for we are linking zyn code into the lmms codebase, all in the global namespace. I am not experienced with LV2 enough to comment on that use case. In general It will only cause us issues if we link to library's that use the same class names as lmms, and are also in the global namespace.

p.s I have moved the ZASF engine class into the zyn namespace in the 2.5 upgrade

[tresf]

p.s I have moved the ZASF engine class into the zyn namespace in the 2.5 upgrade

👍

[jayay]

I think it might be the best to move both lmms and its plugins into a namespace (if possible). Only that will avoid this problem in future. Yes, we could rename our engine.cpp, but the the same issue might happen another time. I think we've wasted enough time for now, it's the best thing to make the change first over here and then try to persuade the 3rd party developers to move their plugins into another namespace too.

[Wallacoloo]

@tresf yes, we could just rename Engine to something more descriptive, like LmmsCore or something.

We could also remove the inheritance from QObject which would make it so there's no vtable conflict. In order to preserve the init status messages, we could add a QObject as a member variable, and connect slots/signals to that instead. This would touch a total of maybe 4-5 files.

@curlymorphic how close is Zyn 2.5 to being ready to merge?

[tresf]

We could also remove the inheritance from QObject which would make it so there's no vtable conflict. In order to preserve the init status messages, we could add a QObject as a member variable, and connect slots/signals to that instead. This would touch a total of maybe 4-5 files.

Creative and least impacting. 👍

[Wallacoloo]

@tresf actually, that won't work. We absolutely have to rename or namespace one of the Engines, or compile Zyn into a separate application. Having two classes with the same name in the same binary is undefined behavior. Even with removing the vtable conflict, we currently have an error in which the ZynAddSubFx NulEngine class actually calls into LMMS' Engine constructor during initialization, instead of the Zyn Engine. Here's a Valgrind trace (running it with gdb shows full pathnames that confirm that it's calling into LMMS' Engine): https://gist.github.com/Wallacoloo/f28e905edea6cb0102b1

Since constructors aren't virtual, I don't think removing the vtable would fix that.

[curlymorphic]

@curlymorphic how close is Zyn 2.5 to being ready to merge?

afaik there are 2 remaining issues, as per last couple of posts in #1991,

1. The current branch I have been working on will not squash into a single git commit.
2. There is no windows gui of ZASF.

No 2. I see as the biggest problem, as I have no idea how to progress on the matter. I have no experience of linking, remote processes or debugging windows applications, not compiled on windows. I did take a week off work, a few weeks back, that I dedicated to this problem, but In that week I made zero progress.

If we decide to change the lmms Engine class name, or put it in a namespace, then I will remove the hack I have placed in the ZASF code.

[tresf]

Just a status update on Zyn, it looks like @badosu was able to help squash the commits. Now it is just a matter of debugging the Windows Zyn GUI issues. I'll load up a version we have in Wine and see if I can get anything usable in terms of error messages on the console.

Just realized this was the wrong thread for a status update on Zyn... Sorry. :)

-Tres

[badosu]

@tresf Plot Twist: the branch that needs squashing/rebasing was not that one on that PR (zynUpgrade), but zynwin.

I am talking with @curlymorphic to see if we can solve this one.

[badosu]

@tresf That branch on that PR is outdated, a new PR using the updated branch is needed.

[curlymorphic]

@tresf sorry, im confused by your post? am I missing some context?

P.S. I tried running Zyn2.5 through wine and the crash doesn't seem to offer any valuable output, just a generic access exception: https://gist.github.com/tresf/bce71657a3768903817d

It does complain about writing to /tmp/ though but that happens just adding the Zyn instrument (well before the crash).

[tresf]

@curlymorphic I'm in the wrong thread. I keep coming back to this one. It was originally in response to item No. 2, which is not part of this PR at all. I've deleted the post and moved it over to #1991

1. [...]
2. There is no windows gui of ZASF.

[Wallacoloo]

I was able to simply rename Lmms' Engine class to LmmsCore while keeping it in Engine.h and adding typedef LmmsCore Engine to the bottom of Engine.h as a workaround for the name clashes.

This allows for the two conflicting classes to actually have different names in the binaries, which completely alleviates the issues we've been having, but the LMMS classes are unaware that Engine has been renamed and so nothing else in the source code needs to be changed.

If we want, this could be a temporary patch we could apply, and then revert once Zyn 2.5 is merged (as it's sort of a weird workaround that I wouldn't want to be a permanent part of the code base).

[tresf]

Good work. I still wonder if "Engine" is a descriptive name in this case. This class reads more like a manager or container.

[Spekular]

@tresf "controller" maybe?

[Wallacoloo]

After 2 months, this has an unholy number of merge conflicts & probably misses a few new lmms classes as well. Doesn't seem to be any consensus in namespacing LMMS either, so I'm closing this.