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 FX-Mixer to Mixer #4893

Closed
wants to merge 4 commits into from
Closed

Conversation

claell
Copy link
Contributor

@claell claell commented Mar 13, 2019

Closes #3300

Also referring previous PR #4890

This time I also tested compiling and running. Not much, if more testing is needed, please tell.

Screenshots:

Bildschirmfoto vom 2019-03-13 17-20-15

Bildschirmfoto von 2019-03-13 16-44-07

@qnebra
Copy link
Collaborator

qnebra commented Mar 13, 2019

That's more logical, good point.

@JohannesLorenz JohannesLorenz added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR labels Apr 17, 2019
@JohannesLorenz
Copy link
Contributor

I'll review.

I consider this PR accepted as the discussions in the issue were in favor and there were no further vetos in 2 months.

@JohannesLorenz JohannesLorenz self-requested a review May 5, 2019 20:05
@JohannesLorenz JohannesLorenz removed needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR labels May 5, 2019
@JohannesLorenz
Copy link
Contributor

I'm OK with all the changes, both stylistic and functional.

Before I'll merge: There are a few remains of fx mixer in the code. If you read that code, you'll have no idea what the FX means because it's vanished from the UI. @claell Do you want to change this, too?

$ grep -i 'fx[[:space:]_-]mixer' -r src/ include/ plugins/ doc/ tests/ data/
src/gui/MainWindow.cpp: ToolButton * fx_mixer_window = new ToolButton(
src/gui/MainWindow.cpp:                                 embed::getIconPixmap( "fx_mixer" ),
src/gui/MainWindow.cpp: fx_mixer_window->setShortcut( Qt::Key_F9 );
src/gui/MainWindow.cpp: m_toolBarLayout->addWidget( fx_mixer_window, 1, 5 );
src/gui/MainWindow.cpp: m_viewMenu->addAction(embed::getIconPixmap( "fx_mixer" ),
src/gui/FxMixerView.cpp:        setWindowIcon( embed::getIconPixmap( "fx_mixer" ) );
include/FxMixer.h:#ifndef FX_MIXER_H
include/FxMixer.h:#define FX_MIXER_H
include/FxMixerView.h:#ifndef FX_MIXER_VIEW_H
include/FxMixerView.h:#define FX_MIXER_VIEW_H

@claell
Copy link
Contributor Author

claell commented May 9, 2019

I did changes to the code in #4890 but afaik was told that this is not wanted by @tresf. So I went ahead with just changing those Strings.

@JohannesLorenz
Copy link
Contributor

@claell I just asked tres via PM whether he meant that and he said no. I'm also against keeping anything "fx mixer" relate in the sources. Can you please fix the occurences I highlighted? Then we can merge 😄

@claell
Copy link
Contributor Author

claell commented Jun 18, 2019

Ah ok. Sad he did not elaborate what he meant with inconsistencies in the code.

Unfortunately there are more fx mixer related occurrences than the ones you highlighted. So if you are against keeping anything of them it won't be enough to rename the things you highlighted. However it is unclear whether @tresf and you share the same opinion about renaming everything, because he seemed to be against it the last time. So for now I will just rename your highlights and the rest can be done or discussed at a different place.

@claell
Copy link
Contributor Author

claell commented Jun 18, 2019

@JohannesLorenz Done. I don't know, what (FX_)MIXER_(VIEW_)H does, because it does not seem to be used in the code. However I also renamed it. Unfortunately now there are conflicts introduced that can hopefully be resolved.

@tresf
Copy link
Member

tresf commented Jun 18, 2019

I don't know, what FX_MIXER_VIEW_H does

@claell It's just a standard C/C++ double-import guard to avoid double-declarations if a header has circular references.

@tresf
Copy link
Member

tresf commented Jun 18, 2019

@claell a complete rename should also rename files. e.g. FxMixerView --> MixerView.

@claell
Copy link
Contributor Author

claell commented Jun 19, 2019

@tresf So you are ok with renaming everything?

@JohannesLorenz Same question to you, since you only highlighted a few occurrences. Could also be postponed to another PR.

@JohannesLorenz
Copy link
Contributor

I'm just seeing that my regex misses a * after the brackets:

$ grep -i 'fx[[:space:]_-]mixer' -r src/ include/ plugins/ doc/ tests/ data/

I'm not sure if the changes (297 lines) are worth the effort. It's doable, but...

@JohannesLorenz
Copy link
Contributor

I just saw that we currently have Mixer and FxMixer. Renaming FxMixer to Mixer would clash with current Mixer. We have two options:

  1. Do not change the class names in code
  2. Change Mixer to AudioRenderer/AudioProcessor/... and then change FxMixer to Mixer

Opinions?

@claell
Copy link
Contributor Author

claell commented Jul 7, 2019

Sorry for my late reply. I think that renaming all occurrences makes sense at least long term wise.

Regarding the double classes I'd also go for option two, since it seems more holistic.

Unfortunately it will take some time until I get to do this.

@ryuukumar
Copy link
Member

Considering that this has been inactive for a while, and we're going to have to redo a lot of stuff anyways when the changes in #5592 do take place, I'll close this for now. If you want to take this on again, you can reopen it.

Thanks for your hard work!

@ryuukumar ryuukumar closed this Oct 24, 2020
@claell
Copy link
Contributor Author

claell commented Oct 27, 2020

Makes sense!

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.

Rename "FX-Mixer" to "Mixer"
5 participants