Skip to content

Conversation

@steven-jaro
Copy link

@steven-jaro steven-jaro commented May 20, 2025

Changes:

  1. 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.
  1. 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.
  1. SongEditor.h:
  • Removed the "record()" method as it will not be used.
  1. 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.
  1. 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.
  1. PianoRoll.cpp:
  • Added "(true, true, true)" to the parammeters entry for the Editor constructor to reflect which buttons need to be shown.
  1. 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.

@steven-jaro
Copy link
Author

I added the changes you asked me.

@steven-jaro
Copy link
Author

steven-jaro commented May 22, 2025

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

@steven-jaro steven-jaro requested a review from regulus79 May 22, 2025 12:38
@regulus79
Copy link
Member

Nice! That does look much cleaner

@steven-jaro steven-jaro requested a review from regulus79 May 22, 2025 20:09
@steven-jaro
Copy link
Author

The error is now fixed.

Copy link
Contributor

@szeli1 szeli1 left a comment

Choose a reason for hiding this comment

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

Please could you address my suggestions? Also I did not test this PR.

@steven-jaro
Copy link
Author

@szeli1 I will adress all your naming and commenting suggestions as soon as I can.

@steven-jaro
Copy link
Author

steven-jaro commented May 25, 2025

@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).

@steven-jaro steven-jaro requested a review from szeli1 May 25, 2025 11:24
Copy link
Contributor

@szeli1 szeli1 left a comment

Choose a reason for hiding this comment

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

I will approve this, but I did not test. I still don't like how many comments there are.

@szeli1
Copy link
Contributor

szeli1 commented May 25, 2025

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.

@steven-jaro
Copy link
Author

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.

@steven-jaro steven-jaro requested a review from szeli1 May 26, 2025 23:01
Copy link
Contributor

@szeli1 szeli1 left a comment

Choose a reason for hiding this comment

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

Making a lambda was a good Idea, I couldn't find any issues, so I will approve this

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.

I only have 3 style suggestions left, but overall this PR is good and can go into testing phase.

@bratpeki
Copy link
Member

bratpeki commented Jun 3, 2025

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.

@bratpeki
Copy link
Member

bratpeki commented Jun 3, 2025

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.

@bratpeki
Copy link
Member

bratpeki commented Jun 3, 2025

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.

Copy link
Member

@bratpeki bratpeki left a comment

Choose a reason for hiding this comment

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

Remove unneeded whitespace.

@regulus79
Copy link
Member

The requested whitespace changes by @bratpeki don't seem to appear in the current PR diff.

Given that this PR has 3 approvals and has (I believe) been tested by bratpeki, would it be reasonable to merge it now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs testing This pull request needs more testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants