-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
🤖 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"} |
I've decided that I'm just going to replace |
One of the biggest differences of 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 |
You can backup this branch and create a new PR from it once this is merged. This won't lead to significant merge conflicts. |
There was a problem hiding this 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:
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 " |
64dee10
to
3a98c5c
Compare
@sakertooth Could you resolve merge conflicts? Once you do that, I will review and test this PR. |
@PhysSong Just got done doing that 👍 |
a157bd6
to
64decc9
Compare
Co-authored-by: Hyunjin Song <[email protected]>
Co-authored-by: Hyunjin Song <[email protected]>
Co-authored-by: Hyunjin Song <[email protected]>
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 <[email protected]>
Co-authored-by: Dominic Clark <[email protected]>
2336fa9
to
4472399
Compare
There was a problem hiding this 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.
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.