Removed the first recording button from SongEditor as it has no use
Changes:
- Editor.h:
- Changed the name of the default value of Editor.cpp constructor from "record_step" to "recordStep".
- Added bool "recordAccompany" and set to false to reflect the changes in the parammeters on Editor.cpp.
- Song.h:
- Removed the "record()" method as it will not be used.
- Remove parts of the code that had spaces instead of tabs size 4.
- SongEditor.h:
- Removed the "record()" method as it will not be used.
- Song.cpp:
- Removed the "record()" method as it will not be used.
- Remove parts of the code that had spaces instead of tabs size 4.
- Editor.cpp:
- Added bool "recordAccompany" to the constructor.
- Renamed "stepRecord" to "recordStep" so that every parammeter name starts with "record*"
- Added a lambda functionally for conditional button creation.
- Changed the order in which the buttons are created. They were ordered in phases, now it is ordered by button.
- PianoRoll.cpp:
- Added "(true, true, true)" to the parammeters entry for the Editor constructor to reflect which buttons need to be shown.
- SongEditor.cpp (the key change):
- Added "(false, true, false)" to the parammeters entry for the Editor constructor to reflect which buttons need to be shown. This is for showing only the recordAccompany button in the song editor.
- Removed connection fro "m_recordAction" as it wil not be used here.
- Removed checking for "m_recordAction" as it doesn't exist anymore.
- Removed the "record()" method as it will not be used.
How to test?: This PR just removes the button in the Song Editor. So one only has to make sure that the correct buttons are shown on each editor and that they work properly as intended.
All of these was decided because there is no really a use for a button that records without playing the song. So the only audio recording button left is the "RecordAccompany". Also this PR solves the first issue out of 4 in #7786.
I added the changes you asked me.
I modified the logic of the set up for all the buttons so when adding the conditions, the code looks as compact and simplified as much as possible. The code order was: [Action set up for each button (play, record, recordAccompany, stepRecord, Stop)] -> [Connection set up for each button (play, record, recordAccompany, stepRecord, Stop)] -> [Toolbar set up for each button (play, record, recordAccompany, stepRecord, Stop)]
And now I changed it to: [Complete setup for play (action, connection, toolbar)] -> [Complete setup for record (action, connection, toolbar)] -> [Complete setup for recordAccompany (action, connection, toolbar)] -> [Complete setup for stepRecord (action, connection, toolbar)] -> [Complete setup for stop (action, connection, toolbar)]
I made this change because otherwise I would have to add 9 "ifs" if I wanted to keep the previous order. So for adding just 3 "ifs" I reordered the logic of the code grouping by button instead of by set up stage. I also changed the order of the bools in the constructor and updated the change for the constructor of the PianoRoll and SongEditor
Nice! That does look much cleaner
The error is now fixed.
@szeli1 I will adress all your naming and commenting suggestions as soon as I can.
@szeli1 I made the changes you requested but I kept the amount of documentation as I think it is useful. In the future, I will adress your suggestion for refactoring some bad variable names (in another PR).
I will adress your suggestion for refactoring some bad variable names (in another PR).
Don't bother with it, thanks for working on this issue.
I will approve this, but I did not test. I still don't like how many comments there are.
I would like to know the opinion from the other devs. If another devs says it is too much commenting, then I will remove them.
You changed all TABs to spaces, can you please fix? Also, what about the unique_ptr question?
So, this PR just nuked one recording button from the Song-Editor, not touching anything else.
I can confirm it's not there for SDL, Jack, PulseAudio and ALSA, which I think is sufficient.
What I'm wondering is this: Are we completely removing it, or is it still possible to get it to happen via the add recordAccompany parameter?
IMO, it shouldn't be able to happen at all. We should have one type of recording, that being the "accompanying recording". If the user chooses to keep certain items unmuted while recording, we shouldn't stop him. If he sets the recording sample track to solo, we record to that track only.
This raises the question of what happens when the recording track is (un)muted, but that might be a problem for the future. Still, would be good to discuss it.
I trust @regulus79, @szeli1 and @JohannesLorenz that the PR is well made, and the button is not there, so I approve it. Still, please change the following before merging.