Skip to content

Extract MIXER command into its own file#2839

Merged
johnnovak merged 13 commits intomainfrom
jn/extract-mixer-command
Sep 8, 2023
Merged

Extract MIXER command into its own file#2839
johnnovak merged 13 commits intomainfrom
jn/extract-mixer-command

Conversation

@johnnovak
Copy link
Copy Markdown
Member

@johnnovak johnnovak commented Sep 6, 2023 •
edited
Loading

Prequel to #2468

mixer.cpp has become quite large, so the more things we can move out of it, the better.

This incidentally also forced better modularisation and API barriers; the MIXER command can no longer poke in the mixer's internals. 🎉

No functional changes; the MIXER command will be extensively improved as part of #2468, and then it will be properly unit tested (now it's a bit of a mess; half of the bugs are caused by the very loose validation of the CLI input).

Suggest to review commit by commit.

@johnnovak johnnovak force-pushed the jn/extract-mixer-command branch from 7a6bf92 to 72ca675 Compare September 6, 2023 03:14
@johnnovak johnnovak self-assigned this Sep 6, 2023
@johnnovak johnnovak added audio Audio related issues or enhancements refactoring Code refactoring without any functional changes labels Sep 6, 2023
@johnnovak johnnovak force-pushed the jn/extract-mixer-command branch from 72ca675 to fc8a19a Compare September 6, 2023 03:41
@johnnovak johnnovak marked this pull request as ready for review September 6, 2023 03:42
@johnnovak johnnovak force-pushed the jn/extract-mixer-command branch from fc8a19a to ab5b53b Compare September 6, 2023 03:43
@kcgen
Copy link
Copy Markdown
Member

kcgen commented Sep 6, 2023

Very nice change! Put that mixer.cpp on a diet 🏃‍♂️ :-)

@johnnovak johnnovak force-pushed the jn/extract-mixer-command branch from ab5b53b to f36a9c9 Compare September 6, 2023 09:53
@johnnovak johnnovak requested a review from shermp September 7, 2023 23:55
kcgen
kcgen approved these changes Sep 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

All looks good from my side. Looking forward to pure float pass-through!

@johnnovak
Copy link
Copy Markdown
Member Author

johnnovak commented Sep 8, 2023

Thanks for the review @kcgen 😄

@johnnovak johnnovak merged commit 23fe564 into main Sep 8, 2023
@Kappa971
Copy link
Copy Markdown
Contributor

Kappa971 commented Sep 9, 2023

Isn't the mixer command using MORE?
scr1

With mixer /?|more it works correctly:
scr2

@johnnovak
Copy link
Copy Markdown
Member Author

johnnovak commented Sep 9, 2023

Isn't the mixer command using MORE?

Looks like it isn't. I'll see what I can do, probably it's not hard (the English text fits on a single page, btw).

@Kappa971
Copy link
Copy Markdown
Contributor

Kappa971 commented Sep 9, 2023 •
edited
Loading

Looks like it isn't. I'll see what I can do, probably it's not hard (the English text fits on a single page, btw).

I think all commands use MORE (right?), so MIXER would be the only one not using it.
The English text would also benefit from it (the Where and Note sections have no space and crossfeed, reverb and chorus on the same line).

@johnnovak
Copy link
Copy Markdown
Member Author

johnnovak commented Sep 9, 2023 •
edited
Loading

Looks like it isn't. I'll see what I can do, probably it's not hard (the English text fits on a single page, btw).

I think all commands use MORE (right?), so MIXER would be the only one not using it. The English text would also benefit from it (the Where and Note sections have no space and crossfeed, reverb and chorus on the same line).

Don't know if all commands use MORE; I think you need to address each command individually. Yeah, when I rewrote the MIXER help, I was shuffling things around until it fit on a single page... the English version 😄 With MORE support, I can add back those blank lines, etc. Thanks for flagging this @Kappa971.

@FeralChild64
Copy link
Copy Markdown
Collaborator

FeralChild64 commented Sep 9, 2023

All the commands should have used the MORE engine for displaying help now - unfortunately, it seems I’ve missed MIXER when doing the rework in commit c824679, sorry for this.

It’s actually trivial to switch the command to use MORE for help display - see changes to program_keyb.cpp in the commit I’ve linked.

@johnnovak
Copy link
Copy Markdown
Member Author

johnnovak commented Sep 9, 2023

All the commands should have used the MORE engine for displaying help now - unfortunately, it seems I’ve missed MIXER when doing the rework in commit c824679, sorry for this.

It’s actually trivial to switch the command to use MORE for help display - see changes to program_keyb.cpp in the commit I’ve linked.

Thanks for the tip @FeralChild64, and no worries—everybody misses a few things from time to time 😄 I'm rewriting the MIXER now, so I'll address this as well.

@kcgen kcgen deleted the jn/extract-mixer-command branch October 17, 2023 19:24
@Grounded0 Grounded0 mentioned this pull request Nov 17, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kcgen kcgen kcgen approved these changes

@weirddan455 weirddan455 Awaiting requested review from weirddan455

@FeralChild64 FeralChild64 Awaiting requested review from FeralChild64

@shermp shermp Awaiting requested review from shermp

Assignees

@johnnovak johnnovak

Labels

audio Audio related issues or enhancements refactoring Code refactoring without any functional changes

Projects

No open projects
0.81 Project
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@johnnovak @kcgen @Kappa971 @FeralChild64