Skip to content

Rename FxMixer to Mixer - #6239

Merged
JohannesLorenz merged 42 commits into
LMMS:masterfrom
gityssh:master
Jan 9, 2022
Merged

Rename FxMixer to Mixer#6239
JohannesLorenz merged 42 commits into
LMMS:masterfrom
gityssh:master

Conversation

@gityssh

@gityssh gityssh commented Dec 23, 2021

Copy link
Copy Markdown
Contributor

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

@JohannesLorenz
JohannesLorenz self-requested a review December 26, 2021 11:53
@JohannesLorenz

Copy link
Copy Markdown
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

LmmsBot commented Dec 26, 2021

Copy link
Copy Markdown
🤖 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"}

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

@JohannesLorenz JohannesLorenz left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread README.md Outdated
Comment thread doc/lmms.1 Outdated
Comment thread include/Mixer.h Outdated
Comment thread include/Mixer.h Outdated
@JohannesLorenz

Copy link
Copy Markdown
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>
@gityssh
gityssh requested review from PhysSong and Spekular January 4, 2022 16:08

@JohannesLorenz JohannesLorenz left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread include/MixerView.h Outdated
Comment thread src/gui/SampleTrackView.cpp
Comment thread include/lmms_basics.h Outdated
Comment thread include/Mixer.h Outdated
@gityssh
gityssh requested a review from JohannesLorenz January 5, 2022 20:19

@JohannesLorenz JohannesLorenz left a comment

Copy link
Copy Markdown
Contributor

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.

@Spekular Spekular left a comment

Copy link
Copy Markdown
Member

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

Comment thread src/gui/MixerView.cpp Outdated
Comment thread src/gui/MixerView.cpp Outdated
Comment thread src/gui/PeakControllerDialog.cpp Outdated
@Spekular

Spekular commented Jan 6, 2022

Copy link
Copy Markdown
Member

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

@Spekular Spekular left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@JohannesLorenz

Copy link
Copy Markdown
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

gityssh commented Jan 9, 2022

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Contributor

@yashraj466 Merged. Thanks a lot for your help!

@gityssh

gityssh commented Jan 9, 2022

Copy link
Copy Markdown
Contributor Author

@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