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

Change Detune To Pitch Bend in Piano Roll For Stable-1.2 #4194

Merged
merged 39 commits into from Mar 19, 2018

Conversation

Anonymouqs
Copy link
Contributor

Fixes #4174 in order to give a clearer name to the mode in the Piano Roll.

This Pull Request is for the 1.2 Stable Branch while there is another Pull Request for the Master Branch.

The name of detuneAction is changed into pitchBendAction as well.

Fixes Issue LMMS#4174 in order to give a clearer name to the mode in the Piano Roll.
@Anonymouqs Anonymouqs changed the title Change Detune To Pitch Bend in Piano Roll Change Detune To Pitch Bend in Piano Roll For Stable-1.2 Feb 26, 2018
@@ -4077,14 +4077,14 @@ PianoRollWindow::PianoRollWindow() :
QAction* drawAction = editModeGroup->addAction( embed::getIconPixmap( "edit_draw" ), tr( "Draw mode (Shift+D)" ) );
QAction* eraseAction = editModeGroup->addAction( embed::getIconPixmap( "edit_erase" ), tr("Erase mode (Shift+E)" ) );
QAction* selectAction = editModeGroup->addAction( embed::getIconPixmap( "edit_select" ), tr( "Select mode (Shift+S)" ) );
QAction* detuneAction = editModeGroup->addAction( embed::getIconPixmap( "automation" ), tr("Detune mode (Shift+T)" ) );
QAction* pitchBendAction = editModeGroup->addAction( embed::getIconPixmap( "automation" ), tr("Detune mode (Shift+T)" ) );
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to change the text for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so the source code is clearer.

Copy link
Member

@tresf tresf Feb 26, 2018

Choose a reason for hiding this comment

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

@Anonymouqs I do not understand your reply. Did you make a mistake and forget this line? Will you fix it? (Hint, the text still reads "Detune mode (Shift+T)")

@Anonymouqs
Copy link
Contributor Author

Added the fix. Apologies for the confusion.

@@ -4077,14 +4077,14 @@ PianoRollWindow::PianoRollWindow() :
QAction* drawAction = editModeGroup->addAction( embed::getIconPixmap( "edit_draw" ), tr( "Draw mode (Shift+D)" ) );
QAction* eraseAction = editModeGroup->addAction( embed::getIconPixmap( "edit_erase" ), tr("Erase mode (Shift+E)" ) );
QAction* selectAction = editModeGroup->addAction( embed::getIconPixmap( "edit_select" ), tr( "Select mode (Shift+S)" ) );
QAction* detuneAction = editModeGroup->addAction( embed::getIconPixmap( "automation" ), tr("Detune mode (Shift+T)" ) );
QAction* pitchBendAction = editModeGroup->addAction( embed::getIconPixmap( "automation" ), tr("Pitch Bend Mode (Shift+T)" ) );
Copy link
Member

Choose a reason for hiding this comment

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

"Mode" here ought to start with a lowercase "m" for consistency with the other tooltips.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new commit.

…DetuningHelper

Rename DetuningHelper.h as PitchBendHelper.h:
* Detune to PitchBend
* detune to pitchBend

PianoRoll.cpp:
* detuning to pitchBend
* Detuning to PitchBend
* I would reccomend testing to make sure all of the features still work. Any detune mention not pertaining to the pitch bend funtion remained detune.
@PhysSong
Copy link
Member

PhysSong commented Mar 8, 2018

@Anonymouqs You can find remaining detune or detuning by:

git grep -i detune -- include/ src/ plugins/ # for detune
git grep -i detuning -- include/ src/ plugins/ # for detuning

However, I think first three commits are sufficient because internal references are not important for stable releases. That may be done in master after synchronizing branches.

@Anonymouqs
Copy link
Contributor Author

@PhysSong Should we discuss on #devtalk if we should revert the internal reference commits?

fix references to detuning to pitchBend
I'm at the point where I am hoping that AI will be able to carry out this repetitively tedious task of refactoring. It's simple enough for reptitiveness, but just a tad bit complex to require a human(for now)
I feel like a while true do loop
Thank you for my dilligence of making this piece of softare better. Thank you for giving the strength on not leaving this branch behind to rot for 10 years and leave LMMS with the Pitch Bench feature instead of the Detune feature. Thank you for giving the strength to go on and get into the Bassline. Thank you for the fact that this is not a corporate setting where every commit has to be 100% serious. Thank you for
@PhysSong
Copy link
Member

PhysSong commented Mar 9, 2018

@Anonymouqs There have been some discussions on Discord #devtalk. Changing UI part and related variable names(which is done by first three commits) seems enough for stable-1.2. Further renaming can be done later(in master).

@Anonymouqs
Copy link
Contributor Author

Anonymouqs commented Mar 9, 2018

@PhysSong I just read your message when I just finished renaming all of the code; I had to trace the dependencies of the variable names across files or else the build would crash. This branch passes through the Travis Build successfully now.

@Umcaruje Umcaruje added this to In Progress in Release LMMS 1.2.0-RC6 Mar 9, 2018
@tresf
Copy link
Member

tresf commented Mar 19, 2018

Although I'm sympathetic to learning the PR process, 39 commits is growing excessive for a cosmetic change. Let's please wrap this up.

@Sawuare Sawuare merged commit fc5fc1c into LMMS:stable-1.2 Mar 19, 2018
@Sawuare Sawuare moved this from In Progress to Done in Release LMMS 1.2.0-RC6 Mar 19, 2018
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants