-
-
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
Split Track.cpp and Track.h #5806
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://11291-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.44%2Bg996763e-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11291?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://11290-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.44%2Bg996763e56-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11290?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://11292-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.44%2Bg996763e56-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11292?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/j05v3978ggu7s8lp/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36650353"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/ag6j9ke0b1wmc2en/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36650353"}]}, "commit_sha": "58ab3f06c591027071938a3262acf8b26c39b86b"} |
@M374LX This is just moving code between files, nothing else? |
@JohannesLorenz Other than moving code between files, there are a few changes in includes and forward declarations. |
Are you able to rebase this branch, so the mentioned conflicts will vanish? |
Let me know when I should review it. Also, as you develop more PRs, just as information: We are currently trying to stop writing new PRs, in order to get our already started PRs closed for the reorganizing (#5592). |
@JohannesLorenz It is ready for review.
|
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.
Very useful PR, thanks for your efforts.
I left a lot of comments, but it's almost all about unused includes. Removing them will partly be a bit of work, but it will increase compile time.
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.
Only one review comment added about some code comments. Functional it's all OK.
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!
Thanks for the PR! I'll wait for CI to finish and then I will "Squash and Merge". If you think it's too early for a merge because you want to add more commits for any reason, scream now 😃 |
@JohannesLorenz I believe it is ready to be merged. |
Fixes conflict caused by LMMS#5806, which splitted Track.cpp and Track.h
This PR splits Track.cpp and Track.h into one pair of files per class, as suggested in #5592.