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

Core Refactor: Replace QVector with std::vector #6477

Merged
merged 76 commits into from
Aug 22, 2023

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Aug 2, 2022

Essentially what was done in Veratil's Qt removal PR, only this time with more modernization (particularly with iterators) and testing. Changes on the GUI side were made as necessary to make the code compile. The remaining QVector's, if any, in the codebase should be in the GUI, but it is likely that I may have missed something.

The "Phase 2" is mainly because there were changes that needed to be made in order to make the code compile that I had missed on the first run.

Edit: I've decided that I will also remove the remaining QVector's in the UI part of the codebase, but as a follow-up PR to this one.

@LmmsBot
Copy link

LmmsBot commented Aug 2, 2022

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/4b9b8c46-60df-4348-b2e5-db6a17d35a8f/artifacts/0/lmms-1.3.0-alpha.1.274+gdcc49af09-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/sakertooth/lmms/428?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/d62c7e90-3e30-4146-aee5-f7dddb8e4ad8/artifacts/0/lmms-1.3.0-alpha.1.274+gdcc49af09-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/sakertooth/lmms/430?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/32f882ab-b6da-4527-9333-a1266081f0d0/artifacts/0/lmms-1.3.0-alpha.1.274+gdcc49af09-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/sakertooth/lmms/426?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "dcc49af0958c8b8a134cf56a1642a9a8e65e9bc7"}

src/core/AutomatableModel.cpp Outdated Show resolved Hide resolved
src/core/EffectChain.cpp Outdated Show resolved Hide resolved
src/core/StepRecorder.cpp Show resolved Hide resolved
src/core/AutomationClip.cpp Outdated Show resolved Hide resolved
src/core/ConfigManager.cpp Show resolved Hide resolved
src/core/Song.cpp Outdated Show resolved Hide resolved
src/core/StepRecorder.cpp Outdated Show resolved Hide resolved
src/core/StepRecorder.cpp Outdated Show resolved Hide resolved
src/core/StepRecorder.cpp Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Outdated Show resolved Hide resolved
src/core/AutomationClip.cpp Outdated Show resolved Hide resolved
src/core/AutomationClip.cpp Outdated Show resolved Hide resolved
src/core/StepRecorder.cpp Outdated Show resolved Hide resolved
@PhysSong PhysSong self-requested a review August 4, 2022 07:02
@sakertooth
Copy link
Contributor Author

I've decided that I'm just going to replace QVector with std::vector everywhere. There is no reason to leave QVector in the codebase as a whole even if this PR has a greater focus on the core.

@PhysSong
Copy link
Member

PhysSong commented Oct 2, 2022

There is no reason to leave QVector in the codebase as a whole even if this PR has a greater focus on the core.

One of the biggest differences of QVector and std::vector that I'm aware of is QVector uses copy-on-write and can implicitly share data between multiple containers. I'm not sure how this will affect our codebase, though.

One more thing worth discussing here is: do we want to changes in UI code in the same PR or not?

@sakertooth
Copy link
Contributor Author

sakertooth commented Oct 2, 2022

There is no reason to leave QVector in the codebase as a whole even if this PR has a greater focus on the core.

One of the biggest differences of QVector and std::vector that I'm aware of is QVector uses copy-on-write and can implicitly share data between multiple containers. I'm not sure how this will affect our codebase, though.

One more thing worth discussing here is: do we want to changes in UI code in the same PR or not?

I think changes to the UI code ultimately had to happen regardless because I had to make sure the code would compile. However, for the 20 most recent commits I just sent, it might be worthwhile to split this PR up because quite frankly it is huge. I've had doubts that this PR will be merged anytime soon just because of how large it is.

Let me know if you want to split this PR personally as well, and I'll do so. My only concern would be if we are going to run into merge conflicts, then I'm mostly not sure.

Edit: And on the topic of why I made QVector changes in the UI, I didn't like the idea of mixing two vector solutions together when the STL version is probably the better option. I also found it hard to make this PR because of my initial focus on just the core. I had to make sure that the core was the only part of codebase being altered (for the most part). With no kind of header separation for the core, checking to see if you are only changing the core files and changing the UI as needed is a pain. Leaving QVector also made this PR feel somewhat incomplete in my opinion.

@PhysSong
Copy link
Member

PhysSong commented Oct 2, 2022

Let me know if you want to split this PR personally as well, and I'll do so. My only concern would be if we are going to run into merge conflicts, then I'm mostly not sure.

You can backup this branch and create a new PR from it once this is merged. This won't lead to significant merge conflicts.
One more reason for splitting is to review new changes separately, if required.

src/core/ConfigManager.cpp Outdated Show resolved Hide resolved
src/core/ConfigManager.cpp Outdated Show resolved Hide resolved
src/core/ControllerConnection.cpp Outdated Show resolved Hide resolved
src/gui/clips/ClipView.cpp Outdated Show resolved Hide resolved
Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespaces added in this PR:

include/AutomationClip.h Outdated Show resolved Hide resolved
include/InstrumentFunctions.h Outdated Show resolved Hide resolved
src/core/AutomationClip.cpp Outdated Show resolved Hide resolved
src/core/Controller.cpp Outdated Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Outdated Show resolved Hide resolved
src/core/StepRecorder.cpp Outdated Show resolved Hide resolved
src/tracks/MidiClip.cpp Outdated Show resolved Hide resolved
src/tracks/MidiClip.cpp Outdated Show resolved Hide resolved
src/tracks/MidiClip.cpp Outdated Show resolved Hide resolved
src/tracks/MidiClip.cpp Outdated Show resolved Hide resolved
@sakertooth
Copy link
Contributor Author

I will try to fix the conflicts within this PR soon, but the clang-tidy PRs that occurred are creating a lot more conflicts than what it would seem. That being said, it might take some time.

I also want to get rid of those "QVector -> std::vector UI" commits I pushed and then reverted, as its just clutter (Assuming this will not cause any major problems. Otherwise, I will just leave it as is).

@PhysSong
Copy link
Member

PhysSong commented Jul 2, 2023

@sakertooth Could you resolve merge conflicts? Once you do that, I will review and test this PR.

@sakertooth
Copy link
Contributor Author

@sakertooth Could you resolve merge conflicts? Once you do that, I will review and test this PR.

@PhysSong Just got done doing that 👍

src/gui/MixerView.cpp Outdated Show resolved Hide resolved
src/gui/MixerView.cpp Outdated Show resolved Hide resolved
src/gui/clips/ClipView.cpp Outdated Show resolved Hide resolved
src/tracks/MidiClip.cpp Outdated Show resolved Hide resolved
src/core/Mixer.cpp Outdated Show resolved Hide resolved
src/core/Mixer.cpp Outdated Show resolved Hide resolved
src/core/ConfigManager.cpp Show resolved Hide resolved
sakertooth and others added 17 commits August 8, 2023 22:52
Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Some of the changes made:
+ Use auto where benefical
+ Fix bug in AutomatableModel::globalAutomationValueAt (for loop should be looping over clips variable, not clipsInRange)
+ Undo out of focus whitespace changes
Even when the clip is not found (i.e., currentClip is -1), m_steps still
gets assigned to.
Co-authored-by: Dominic Clark <mrdomclark@gmail.com>
Co-authored-by: Dominic Clark <mrdomclark@gmail.com>
src/core/AudioEngine.cpp Outdated Show resolved Hide resolved
include/AutomationEditor.h Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Show resolved Hide resolved
Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I haven't checked in-depth the places where the changed types are used, but the diff looks fine. It builds, the tests pass, and it seems to function as normal, so I'm happy for any fallout to be cleared up as we come across it in the future.

@PhysSong PhysSong merged commit 9a0add4 into LMMS:master Aug 22, 2023
9 checks passed
@sakertooth sakertooth deleted the core-refactor-qvector branch August 22, 2023 03:16
DomClark pushed a commit that referenced this pull request Aug 23, 2023
* Fix if statement in ClipView

* Move controllers.begin() to be the first argument
It was the second argument, which means it could've
returned negatives for random access iterators.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants