Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plan for the core: Removing Qt, Decoupling, and Modernization #6472

Open
sakertooth opened this issue Jul 28, 2022 · 10 comments
Open

Plan for the core: Removing Qt, Decoupling, and Modernization #6472

sakertooth opened this issue Jul 28, 2022 · 10 comments

Comments

@sakertooth
Copy link
Contributor

sakertooth commented Jul 28, 2022

Nothing about this is definitive, and not everything will be agreed upon. Feel free to tweak the following plan and give general feedback as you see fit.

I am writing this plan as somewhat of a proposal as to what steps should be taken pertaining to the core, while also gathering the already existing ideas about it into one place. From a personal standpoint, improving the core has been an idea I have had for awhile (mainly because of the need for good foundations for potentially upcoming PRs), but that idea has been unclear in terms of how it should be approached. As a result, I have refrained from tackling it until I had a clear and strong plan to go forward. The hope is that this plan can be used as some ground to work on and develop ideas from.

Preliminary Step: Working in a distinct branch away from master

This goal of drastically improving the core will most likely be far too large to start doing real work in the master branch. I fear it will also be polluted at some point given the sheer number of changes to be made, and the chance for merge conflicts are high. Instead, it can be done on its own branch, away from master.

Goal: Removing Qt

Remove the use of Qt from the core.

Reasoning

  • It allows for easy migration to various frontends - We can use the core as a solid foundation for various frontends of our choosing, giving the application more flexibility in terms of how it can be utilized.

  • It will help assist in making the core realtime safe - Qt may not be realtime safe, so using it in places where realtime is necessary makes Qt a liability.

  • It will give space for more effective and efficient replacements to take effect - Removing Qt will allow more space for libraries such as the STL to shine through, which should be the preferred option above other libraries when possible.

  • Developers wanting to contribute don't require knowing Qt beforehand - We will most likely still be using Qt as the default and standard frontend, but for those wanting to improve the internals of LMMS specifically, removing Qt may make achieving this easier.

Approach

I propose replacing simple Qt components and data structures first, such as:

  • QVector
  • QMap
  • QHash
  • QPair

Classes such as these already have drop-in replacements from the STL, so making these changes are fairly doable. A specific and detailed way of removing these classes, say, by file, is unwanted.

Functions such as qMin, qMax, qBound, and qAbs also apply here.

Missing functionality

Qt, being as vast as it is, comes with certain functionality used widely in this codebase that doesn't have a quick and easy drop-in replacement like the STL. The general approach to address this would be to prefer using a library before writing the functionality ourselves. If the situation calls for the opposite, by all means.

The functionality that will be missing after removing Qt and will need a suitable replacement are:

  • Signals and Slots
  • XML processing and handling
  • Base64 Encoding and Decoding
  • Interacting with shared libraries at runtime
  • Universally Unique Identifiers (UUID)
  • MIME data handling
  • The event loop

Goal: Decoupling the core from the GUI

Separate the core from the GUI.

Reasoning

  • Efficiency. It may be easier to accomplish the goals described here when the core is decoupled from the GUI because of the contained workflow this will create.
  • Making the core as its own standalone library away from the frontend will be more doable.
  • A more organized repository in general, which also ties in with better productivity and development.

Approach

The core can be organized in its own directory. More specifically, the core headers can be in include/core, distinct from the GUI headers. Do note that updating the core #include's in the source files have already been done, but reviewing is still needed.

Another form of decoupling that should be considered is making the core not depend on the GUI or some arbitrary frontend. To elaborate, the core currently refers to the Qt GUI with gui::getGUI for various reasons, but this makes it less feasible to make the core as its own library and removes the advantage of having flexibility for different frontends. Therefore, it is important that the core has no knowledge of any form of a GUI, but still does what is necessary of it. A possible approach to this is to rewrite parts of the core from the bottom up, whilst still using most of the original code. I am suggesting a rewrite because trying to make the core not depend on the GUI may require completely rethinking how the code is written. For instance:

static QProgressDialog * pd = nullptr;
	bool was_null = ( pd == nullptr );
	if (!journalRestore && gui::getGUI() != nullptr)
	{
		if( pd == nullptr )
		{
			pd = new QProgressDialog( tr( "Loading project..." ),
						tr( "Cancel" ), 0,
						Engine::getSong()->getLoadingTrackCount(),
						gui::getGUI()->mainWindow());
			pd->setWindowModality( Qt::ApplicationModal );
			pd->setWindowTitle( tr( "Please wait..." ) );
			pd->show();
		}
	}

Snippet of TrackContainer::loadSettings(const QDomElement & _this).

Given this decoupling goal, how should this code be written? Evidently, the approach to writing this has to be different, with any progress dialog information handled on the GUI side, not in the core.

Goal: Modernization

It is one thing to remove Qt and make the core its own library, but I feel it is necessary to also modernize the core as well.

Reasoning

  • The core currently uses old practices that were once deemed adequate, which can leave room for mistakes, memory leaks, and bugs.
  • If we are removing Qt from the core and using a more idiomatic alternative provided in the STL, it then gives reason to make the code idiomatic as well on a larger scale.
  • Having a strong and stable "core" (pun somewhat intended) to depend on is preferable when having to ponder about problems on the GUI side of things.
  • The code may become more expressive and clear in terms of intent.
  • Idiomatic code is generally a good idea.

Approach

Modernization of the core can be done in a fashion somewhat similar to clang-tidy checks, or done while removing Qt given a certain commit.

For instance, replacing the use of begin() and end() iterators in for-loops with a range-based for loop provided in C++11 if applicable. Such a change may be done once throughout the codebase, or simultaneously when Qt is being removed.

These changes should not necessarily be bug fixes, but just changes that make the core more modern and idiomatic.

Methodologies

There are two main methods to achieve these goals of improving the core: either by incrementally refactoring the code, or by starting with a clean slate. Both generally should keep better principles in mind pertaining to the code itself, but they have some differences that should be considered and explicitly stated.

Incremental Refactoring

This method of improving the core is simply by updating the code as necessary in an attempt to achieve the goals described in the previous section. As such, the PRs will look more like edits rather than an introduction of better code.

Advantages:

  • Is naturally incremental and slow-paced, potentially reducing the amount of conflicting PRs and merge conflicts.
  • Less time constraints, if any at all.
  • Mimics a normal development procedure where you send multiple PRs with fairly sized changes for improvements in the code, rather than a few large ones.
  • Easier to review, which ultimately makes integrating the changes happen faster.

Disadvantages:

  • Sometimes legacy code for a particular function is written in such a way that a rewrite is superior.
  • Potentially smaller yield of modernized code since there is a greater chance parts of the code that should be refactored are missed entirely.

Clean Slate/Rewrite

This method of improving the core uses a clear canvas approach and builds up from there.

Advantages:

  • Sometimes more effective for various chunks of code, such as particular functions.
  • Potentially higher yield of modernized code since there is no legacy code to begin with.

Disadvantages:

  • Time constraints.
  • Could potentially make PRs pertaining to the original code invalid.
  • Could potentially create new bugs that haven't been seen before.
  • Developers may find the swift change in code difficult to adjust to.
  • Putting off other vital PRs in sake of a rewrite may be detrimental in the long run in terms of time.
  • Will most likely create merge conflicts for important PRs.
  • Ultimately more code, thus taking longer to review.
  • Makes tests pertaining to the original code invalid.

Current Methodology Verdict (Susceptible to be changed):

After some discussion, the current verdict I have in mind now is to incrementally refactor the code and only rewrite certain parts of functions if absolutely necessary. Such rewrites can be quickly reviewed for any conflicts with other PRs or discarded if they are just too large in general to be considered.

The goal of these rewrites, in the event that they may occur, is to mostly to make the a particular part of code in question more idiomatic but also maintain the same exact functionality as the original code.

  • Original verdict on 7/30/22
  • Updated on 8/1/22

PR Format:

Given that the number of changes will most likely be fairly high, I propose an additional PR title format that can be followed. The reason behind this is that without some format to go off of, it can make it harder to keep track of the task and thus make everything less efficient.

Given a PR that is contributing to one of the goals described in this document, a title that can be used in the PR to make it easier to keep track of them can be:

"Core Refactor: [Goal], [Specific Details]"

Starting with "Core Refactor" makes it already clear that this PR in particular is for this project, and the goal that this PR is contributing towards is also specified. Additionally, specific details about the PR should be provided as per usual.

@irrenhaus3
Copy link
Contributor

Mentioning #6452 as introducing another QT-provided type into the core: QUuid; add this to the "missing functionality" section.

@musikBear
Copy link

Just as a reminder:
#6194

@Veratil
Copy link
Contributor

Veratil commented Jul 29, 2022

Just as a reminder: #6194

This has nothing to do with core though.

@sakertooth
Copy link
Contributor Author

Added a "Methodology" section that explains the specific technical methods to alter and modify the code. The current verdict I have in summary is to both incrementally refactor the code while also having some clear canvas to start with. I find it hard to believe that one method is solely better than the other, especially when it is so dependent on the situation and the kind of code we are dealing with in the core.

@musikBear
Copy link

This has nothing to do with core though.

Oh would we keep QT for UI even if/when we outfaced it in core!
I diddent realize that was in the plan -Sorry. I hoped for a complete exit from QT.

@Veratil
Copy link
Contributor

Veratil commented Jul 31, 2022

This has nothing to do with core though.

Oh would we keep QT for UI even if/when we outfaced it in core! I diddent realize that was in the plan -Sorry. I hoped for a complete exit from QT.

Not necessarily. A frontend redesign though has nothing to do with the backend core. As we progress with this plan, we could have a Qt frontend, a Godot, a mobile, and whatever else anyone decides to make for a frontend.

@sakertooth
Copy link
Contributor Author

Updated my methodology/approach. I think I may be able to officially start now or somewhere in a couple days give or take if I have time.

@PhysSong PhysSong added the core label Jan 6, 2023
@gnudles
Copy link
Contributor

gnudles commented Aug 28, 2023

I think we should prefer Base85/Ascii85 instead of Base64. It is much more compact, and saves a lot of space.

@Veratil
Copy link
Contributor

Veratil commented Aug 28, 2023

I think we should prefer Base87 instead of Base64. It is much more compact, and saves a lot of space.

I don't know much, nor can I find much, about Base87, but I am aware of Ascii85. Is this what you meant?

@gnudles
Copy link
Contributor

gnudles commented Aug 29, 2023

@Veratil
Yeah, my bad. I probably confused because I once wrote a variant of base85, that uses two additional characters, one to encode four bytes zero, and one to encode- "repeat the last four bytes". Which gave that a little superpowers of compression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants