lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Core Refactor: Replace ``QVector`` with ``std::vector``

Open sakertooth opened this issue 3 years ago • 1 comments

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.

sakertooth avatar Aug 02 '22 17:08 sakertooth

: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:

Linux

Windows

:robot:
{"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"}

LmmsBot avatar Aug 02 '22 18:08 LmmsBot

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.

sakertooth avatar Sep 30 '22 13:09 sakertooth

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?

PhysSong avatar Oct 02 '22 06:10 PhysSong

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.

sakertooth avatar Oct 02 '22 11:10 sakertooth

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.

PhysSong avatar Oct 02 '22 12:10 PhysSong

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).

sakertooth avatar Jan 28 '23 01:01 sakertooth

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

PhysSong avatar Jul 02 '23 02:07 PhysSong

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

@PhysSong Just got done doing that :+1:

sakertooth avatar Jul 03 '23 00:07 sakertooth