LMMS
Remove some Qt from core
I am opening this to start conversation on what we want to do for our path to remove Qt from the core files (and if we agree, to merge it of course 😁).
So, I have a few questions for the group:
- How should commits for removing/replacing/adding functionality be done?
- One commit per removal/replacement/addition? (I'm combining two things here actually: moving the Memory*.h files and creating the common.h file)
- Are partial replacements okay? (I didn't change all
qWarningsfor instance, since my quick and dirtylmms_warningfunction isn't a 1:1 drop-in replacement; and I only targeted files insrc/core) - Do we think this PR shows a route we actually want to go?
- Moving header files into a
include/corefolder similar to thesrcfolder now - creating
core/common.hfor common core functions, macros, etc
- Moving header files into a
Oh, and to note: I did not update areas that are Mac or Windows in this.
:robot: Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! :tophat:
Windows
- Windows 32-bit:
lmms-1.3.0-alpha-msvc2017-win32.exe(build link) - Windows 64-bit:
lmms-1.3.0-alpha-msvc2017-win64.exe(build link)
:robot:
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/j0u34c8h0nm8p2t4/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43842203"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/7lwf1urhdcoa07jn/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43842203"}]}, "commit_sha": "1a61bb9a471d143cb7a6b4180954616ea7b6ae75"}
How should commits for removing/replacing/adding functionality be done?
I personally think with the sheer number of changes to be made, the commits can be done in chunks, with each chunk signifying the part of the core where Qt was removed/reduced. This is subjective, ultimately. If you feel like having a commit for the removal of Qt per component in the core would be easier, then by all means.
Are partial replacements okay? (I didn't change all qWarnings for instance, since my quick and dirty lmms_warning function isn't a 1:1 drop-in replacement; and I only targeted files in src/core)
I feel like if we are removing Qt from the core, there should be a full removal. It just makes more sense that way I suppose. Assume we wanted to work towards using a new frontend. A qWarning inside the core would be quite the surprise. However, I feel like the main focus should be on removing the usage of Qt and replacing it with, say, the C++ standard or some other reasonable alternative. If there isn't a reasonable alternative, then I suppose it can be left alone for the time being.
This is a developing topic after all, so I expect there to be solid decisons made in the future about this 😆.
Do we think this PR shows a route we actually want to go?
In my opinion, yes. I see many benefits in the future for removing Qt from the core:
- More flexiblity for various frontends.
-
lmmslibwill most likely be small in size thanks to having no Qt dependency. - We can catch up on improving the code overall by using more of the standard where applicable, making it more friendly to newer C++ developers wanting to improve the code.
(I might've misunderstood that last point of yours. You were asking how this PR will be a beneficial route correct?)
(I might've misunderstood that last point of yours. You were asking how this PR will be a beneficial route correct?)
Yep, seeing if we agree this would be a good step forward.
I feel like if we are removing Qt from the core, there should be a full removal. It just makes more sense that way I suppose.
Keep in mind a sweeping change like that would be a large PR, which would be difficult to test all facets of what changed. Smaller increments are easier to test and review.
Keep in mind a sweeping change like that would be a large PR, which would be difficult to test all facets of what changed. Smaller increments are easier to test and review.
Ah, very true. In that case, I think the commits can be made detailing the changes to each component in the core, similar to how irrenhaus did them in the lmms namespace PR, Ultimately resulting in small, incremental commits for the PR. For instance:
- Remove Qt from SampleClip
With that commit signifying the removal of Qt from SampleClip's interface and implementation, respectively. The only thing I fear is that there will ultimately have to be multiple changes to remove Qt from a single component of the core due to how intertwined the core may be. Using the example above, SampleClip, SampleTrack, SamplePlayHandle may all be connected in some way using Qt, so the necessary changes may naturally have to expand to different components of the core. I believe this to be the case for the parts of the core that use, say, signals & slots and other central Qt functionality.
This brings up a new problem where we would need new implementations to replace certain Qt functionality, such as message passing.
Scratch that. A more incremental approach can be done like this:
- Removal of Qt classes and objects from a specific part of the core
- Removal of Qt helper functions from a specific part of the core
- Removal of Qt includes from a specific part of the core
- Implement
<certain implementation>as a replacement for<Qt functionality to be replaced> - Replace
<Qt functionality>in<core component>for<new implementation> - Organize core component
<core component>'s interface insideinclude/coredirectory
What I suppose I meant was that there would ultimately be a complete removal of Qt from the entire core at the final stage of this PR, with incremental changes along the way.
In general:
I am in favor for removing Qt from the core, especially because many Qt things are not realtime-safe, so it's a good preparation. E.g., due to the copy-on-write operations, even const operations like m_qVector[0] can cause allocations in the realtime thread. Aside from that, if we split core and GUI, then this will allow you to run LMMS with an alternative GUI/CLI without requiring Qt.
"One commit per removal/replacement/addition?"
Here I'm for the usual approach that I apply for arbitrary commits: Try to make commits atomic, but don't force/overdo it. For example, if you do a bugfix and see 1 or 2 spelling errors in comments, you will usually put them into the bugfix. If you see 100 spelling errors (maybe always the same word?), put it in an extra commit (because otherwise, no one will see the bugfix among all the spelling fixes). In your example, at least one of the changes seems minor (moving files isn't that crucial), so I'm fine with combining it (as long as both is in the commit message, like in your case).
"Are partial replacements okay?"
Like usual, not perfect, some reviewers will ask about it, but acceptable. We usually want to reduce the number of commits on master.
"Do we think this PR shows a route we actually want to go?"
If we will finally have LMMS realtime safe or split into GUI/core, then warnings from core and from GUI have to call different functions. So it's reasonable to use different headers for this.
Whether this should go into different subfolders below "include/" or not is IMO a more general question than removing Qt from the core. It's more a discussion like #6374. But as long as there is no discussion about whether splitting "include/"-files in general, I would (personally) not start creating a subfolder for one file, but rather use "core-common.h".
If we will finally have LMMS realtime safe or split into GUI/core, then warnings from core and from GUI have to call different functions. So it's reasonable to use different headers for this.
Whether this should go into different subfolders below "include/" or not is IMO a more general question than removing Qt from the core. It's more a discussion like #6374. But as long as there is no discussion about whether splitting "include/"-files in general, I would (personally) not start creating a subfolder for one file, but rather use "core-common.h".
To me our current "every header file in existence under include/" is just lazy. If we're organizing the src/ folder then we should organize the include/ folder as well, otherwise why are we even moving files around in src? :)
Because much of Qt dates back to the time before good standard c++ functions, I think these kind of replacements could be done pretty easily and as a first step. QVector to std::vector etc...
It would be great to be able to dive down in one single file and clean it of Qt, because it gives you an opportunity to wrap your head around that piece of code. But that file may be a dependency of 20 others, so that's virtually impossible without creating our own monkey patched drop in replacement for Qt...
I think we might need to replace Qt one aspect at a time. The PRs will be more monotonic to review - for the better or worse. You'll get the hang of it quickly, but some things may slip through the cracks.
Problem with this is we'll end up in a re-org situation again where any useful work is hindered by a thousand merge conflicts.
For this reason please don't create too many commits if the change is the same. E.g. don't "remove QVector from Note.cpp"; "remove QVector from Track.cpp".... Because that makes rebasing terrible. (Well for your own sake maybe because when the PR is merged the commits may be squashed...) But I may be totally wrong on this point.
And then at some point in time we'll need to replace the signal/slot system and event loop and that can't be down gradually. But that's for a late time.
TL;DR I don't know what's best but let's do this.
Replace QT function with standard C++ function across entire codebase "one by one" if C++ function is avalaible? I see Allejok write basically the same proposition.
Because much of Qt dates back to the time before good standard c++ functions, I think these kind of replacements could be done pretty easily and as a first step. QVector to std::vector etc...
I agree, QVector might be a good first target.
It would be great to be able to dive down in one single file and clean it of Qt, because it gives you an opportunity to wrap your head around that piece of code. But that file may be a dependency of 20 others, so that's virtually impossible without creating our own monkey patched drop in replacement for Qt...
I think we might need to replace Qt one aspect at a time. The PRs will be more monotonic to review - for the better or worse. You'll get the hang of it quickly, but some things may slip through the cracks.
Yeah, this PR I sort of went the route of a "single" (double in this case, plus one new) file. I wanted to find a very simple and small file that I could remove Qt from entirely or almost entirely. I saw MemoryManager and figured it would be a good case study.
Problem with this is we'll end up in a re-org situation again where any useful work is hindered by a thousand merge conflicts.
For this reason please don't create too many commits if the change is the same. E.g. don't "remove QVector from Note.cpp"; "remove QVector from Track.cpp".... Because that makes rebasing terrible. (Well for your own sake maybe because when the PR is merged the commits may be squashed...) But I may be totally wrong on this point.
We'd probably squash those into a single commit though. ;) And working on the core is a reorg but not part of the main reorg. It's unfortunately impossible to prevent rebases as we replace Qt with the STL. I want to do what's best for progress forward, and it's going to inevitably step on some toes along the way.
And then at some point in time we'll need to replace the signal/slot system and event loop and that can't be down gradually. But that's for a late time.
TL;DR I don't know what's best but let's do this.
I don't even want to think about the signal slot system at this time. 😂
I don't worry much that this would be another reorg, because I wouldn't expect too many merge conflicts with existing PRs (but maybe I'm wrong). E.g. there are not that many QVector in our headers (git grep -c QVector include/), and most of them are likely private... And even then, usage like operator[] would be unchanged, so...
It would be great to be able to dive down in one single file and clean it of Qt, because it gives you an opportunity to wrap your head around that piece of code. But that file may be a dependency of 20 others, so that's virtually impossible without creating our own monkey patched drop in replacement for Qt...
This is main fear that I think is just inevitable at some point. The core as of right now is a mix of cables with Qt binding it together. The act of detangling specific cables, I believe, adheres to the expectation that other cables have to be moved around as well.
Given this, my curiosity is how the new implementations will look like and how to rethink current functionality such as the signal & slot system and QDomElement and QDomNode, among other things. Will new, standalone libraries be introduced?
Simple aspects such as QVector and QString are undoubtedly easier to replace, but I tend to seek complete removal in each core component. With that ideal case, I can just check it off my mental checklist of core components that are clean of Qt usage and move on to another one. But alas, that obviously is not the case for a majority of them.
The only other route I can think of is, like allejok has already stated, removing Qt from the core by starting with the simplest aspects first, like QVector, QString, etc.
E.g. there are not that many QVector in our headers and most of them are likely private... And even then, usage like operator[] would be unchanged, so...
There's the silliness of append vs push_back and isEmpty vs empty and so on... And that goes for NoteVector and TrackList and well... we'll discover them along the way. Better just start doing it. But there will be conflicts.
I have removed more QVector from the core in this PR here. Do note that it is missing ConfigManager because of unusual behavior when switching to std::vector ~and InstrumentFunctionNoteStacking::ChordTable as mentioned in #6434.~
The InstrumentFunctionNoteStacking::ChordTable switch has been made with various adjustments. It does not inherit anymore and stores a std::vector<Chord> instead.
I think there needs to be more preliminary steps like a reformat and file reorg of the core though. This will make the task a lot easier and more efficient.