Make playhead red when recording - #7847
Conversation
|
I briefly tested this, and I am able to get into a state where the step recorder playhead is detached from the normal playhead. Then again, this might be how it is on master also.
Also, the step recorder color seems to be a bit more pink/orange than the recording playhead? (And I tested, the themeable recording line color does seem to work! I'm able to change it's color via css in my purple theme) |
Yeah It's the same as master. Maybe I should hide the playhead while step recording?
Yeah I noticed that. I got the color from the step recorder button (which is just (255, 0, 0). Which colour do you prefer? I'll make sure they match up. |
To be honest, the current red color of the recording line sort of feels really dark, but maybe that's just because I'm not used to it. Maybe someone else can give their opinion too?
Well, you are able to use the playhead to kind of "preview" the notes you've added so far, so I think it makes sense to keep it there. Actually now that I think about it, the recording done in the step recorder is with it's own playhead, not with the piano roll's playhead, so it might even be okay to keep the normal playhead white? |
@bratpeki what do you think about the colour of the playhead? maybe we use one of the reds mentioned in #5166
One thing's for sure is we should agree on a red to use for recording and stick to it across the board.
Yeah, I think you're right. |
|
mentioning @bratpeki again for opinions on the color |
|
If it's still like in the video above, maybe a bit too dark. What are some reds we use in the program? The only red thing that comes to mind is the Stereo Matrix. |
|
Hmm, now that I've thought about it, maybe that shade of red might be fine? It seems like tenacity uses a similar dark shade for their recording playhead. |
I listed them in a previous comment. |
|
I decided to try out
|
|
@szeli1 didn't mean to request a review, feel free to dismiss if you'd like (i dont have permissions to dismiss requests for some reason) |
…the position line in PianoRoll. Co-authored-by: Dalton Messmer <[email protected]>
2c966ce to
8f16f78
Compare
Co-authored-by: Dalton Messmer <[email protected]>
Co-authored-by: Dalton Messmer <[email protected]>
|
I'll try testing this again, can we merge afterwards @messmerd? |
|
@bratpeki Sounds good to me. I think this PR is fine code-wise. |
|
Getting back to you when I test it, then! |
|
Okay, tested with step and regular clip recording, everything works great! I would greatly appreciate a follow-up PR where the playhead color can be set to a custom value. @regulus79 did that for the oscilloscope and some other PRs so you could ask there for help, but I think we should definitely allow the user to set the line color so it matches the theme. |
I believe that has already been added to this PR. I remember testing it a while back. |
|
should be ready for merge assuming these builds pass |
|
I tested this again, and I notice when you turn on step recording, the triangle for the playhead on the timeline disappears, but the position line is still visible. Is that intended? |
|
Playhead and position line are both now fully visible during step recording |
| m_recording = false; | ||
| m_scrollBack = m_timeLine->autoScroll() != TimeLineWidget::AutoScrollState::Disabled; | ||
|
|
||
| auto* songEditor = GuiApplication::instance()->songEditor()->m_editor; |
There was a problem hiding this comment.
Is there any difference between using GuiApplication::instance() vs getGUI()? It seems like most of the codebase uses the latter, but maybe it doesn't matter.
There was a problem hiding this comment.
GetGUI() just calls GuiApplication::instance() and returns the result
| m_editor->m_timeLine->setRecording(false); | ||
| m_editor->m_positionLine->setRecording(false); |
There was a problem hiding this comment.
I notice the same two calls to update the recording state of the timeline and position line appear multiple times in the PR. That's fine, I'm just wondering if there would be a cleaner way to somehow automatically update them whenever their editor starts recording. But that's probably for a future PR.
|
Are we good to merge? |
|
This PR has two approvals with write access, plus one approval from bratpeki. It appears it has also been tested by bratpeki and I, so I think it is reasonable to merge it now. |
5 participants
2025-06-07.19-40-11.mp4