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

Rename FxMixer to Mixer #6239

Merged
merged 42 commits into from
Jan 9, 2022
Merged

Rename FxMixer to Mixer #6239

merged 42 commits into from
Jan 9, 2022

Conversation

gityssh
Copy link
Contributor

@gityssh gityssh commented Dec 23, 2021

This PR aims to rename FxMixer to Mixer, as decided in #6089 and #5592.

@JohannesLorenz
Copy link
Contributor

The code can not compile due to this:

In Engine.h, you use static Mixer * Mixer(). This means you define a function with the same name as your class. Also, note that our coding conventions imply that class members (like the Mixer function) must use camelCase.

Same issue in GuiApplication.h with function MixerView.

@LmmsBot
Copy link

LmmsBot commented Dec 26, 2021

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

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://15475-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.200%2Bge63e2f0bb-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15475?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://15479-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.200%2Bge63e2f0bb-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15479?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/myrfnb4ivjumwf2q/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42123895"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/30336pb8yemdsbmu/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42123895"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://15478-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.200%2Bge63e2f0bb-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15478?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://15477-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.200%2Bge63e2f0bb-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15477?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "abf64c976adf50509318eed35368d534a70574db"}

include/GuiApplication.h Outdated Show resolved Hide resolved
include/Mixer.h Show resolved Hide resolved
src/core/Engine.cpp Outdated Show resolved Hide resolved
src/gui/MainWindow.cpp Show resolved Hide resolved
src/core/DataFile.cpp Outdated Show resolved Hide resolved
Yashraj Shinde and others added 2 commits December 28, 2021 07:38
Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Only 4 comments added.

Also, don't forget the stuff from Discord (e.g. the "Send to FX channel" should be "Send to Mixer channel", and the "FX" in InstrumentWindow should be "CH", ...).

README.md Outdated Show resolved Hide resolved
doc/lmms.1 Outdated Show resolved Hide resolved
include/Mixer.h Outdated Show resolved Hide resolved
include/Mixer.h Outdated Show resolved Hide resolved
@JohannesLorenz
Copy link
Contributor

Additional note: The submodule "doc/wiki" contains multiple occurrences of "fx", "Fx" or "FX". Those should be fixed, too.

Yashraj Shinde and others added 5 commits December 28, 2021 23:52
Co-authored-by: Johannes Lorenz <1042576+JohannesLorenz@users.noreply.github.com>
Co-authored-by: Johannes Lorenz <1042576+JohannesLorenz@users.noreply.github.com>
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Only 4 new comments, and the remaining wiki changes.

include/MixerView.h Outdated Show resolved Hide resolved
src/gui/SampleTrackView.cpp Show resolved Hide resolved
include/lmms_basics.h Outdated Show resolved Hide resolved
include/Mixer.h Outdated Show resolved Hide resolved
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Code is OK, except remaining wiki (Becoming-a-Theme-Developer.md, LMMS-Architecture.md)/GitBook changes.

Basic tests done, all OK.

Copy link
Member

@Spekular Spekular left a comment

Choose a reason for hiding this comment

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

Review limitations:

  • Only checked line diffs, no real bigger picture considerations
  • Filtered out all changes to mmp, mpt, ts, and xpf files

src/gui/MixerView.cpp Outdated Show resolved Hide resolved
src/gui/MixerView.cpp Outdated Show resolved Hide resolved
src/gui/PeakControllerDialog.cpp Outdated Show resolved Hide resolved
@Spekular
Copy link
Member

Spekular commented Jan 6, 2022

An additional request: In the comments for the upgrade routines, please either change "savings" to "project files", or omit it entirely.

I would also suggest moving all the upgrades into one function (upgrade_mixerRename) with a top level comment // Remove FX prefix from mixer and related nodes. The current comments about nodenames could then be moved to inside this function. However, before you do this I'd want to confirm with someone else (e.g. @JohannesLorenz) that they're not opposed.

@PhysSong PhysSong removed their request for review January 7, 2022 01:41
@gityssh gityssh requested a review from Spekular January 7, 2022 17:26
Copy link
Member

@Spekular Spekular left a comment

Choose a reason for hiding this comment

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

LGTM.

@JohannesLorenz
Copy link
Contributor

@yashraj466 Spekular and me think that the PR is good. Do you want us to merge this, or do you still want to do more on this PR?

@gityssh
Copy link
Contributor Author

gityssh commented Jan 9, 2022

@yashraj466 Spekular and me think that the PR is good. Do you want us to merge this, or do you still want to do more on this PR?

Okey 👍

@JohannesLorenz JohannesLorenz merged commit bf323d2 into LMMS:master Jan 9, 2022
@JohannesLorenz
Copy link
Contributor

@yashraj466 Merged. Thanks a lot for your help!

@gityssh
Copy link
Contributor Author

gityssh commented Jan 9, 2022

@yashraj466 Merged. Thanks a lot for your help!

Welcome 😃

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.

5 participants