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.

Sign up for GitHub

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

Jump to bottom

Rename AudioPort -> AudioBusHandle #7712

Merged
merged 5 commits into from
Feb 28, 2025
Merged

Rename AudioPort -> AudioBusHandle #7712

merged 5 commits into from
Feb 28, 2025

Conversation

JohannesLorenz
Copy link
Contributor

No functional changes.

@JohannesLorenz
Rename AudioPort -> AudioBusHandle
7fd1012 
No functional changes.
@JohannesLorenz JohannesLorenz marked this pull request as ready for review February 15, 2025 20:00
sakertooth
sakertooth reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

In regards to the name change, I am okay with it. @messmerd might have something to say about this, but I am okay with AudioBusHandle, as I don't think its particularly efficient to ponder about this for too long.

@messmerd
Copy link
Member

messmerd commented Feb 18, 2025 •
edited
Loading

I'm rethinking whether "Handle" is a good term here.

Typically a "handle" is a pointer or integer identifier that is used to refer to some resource. This AudioPort on the other hand, is a ThreadableJob which processes an audio bus (and perhaps in the future, audio busses plural) rather than just a handle.

Some more applicable terms in my opinion would be "job" or "processor".

However, PlayHandle would also have the same issue of not technically being a handle. If this AudioBusHandle rename is for consistency with that, it may be okay for now.

@JohannesLorenz
Copy link
Contributor Author

That was what I was thinking. If we use Job here, we have to rename quite a lot of classes...

messmerd
messmerd reviewed Feb 20, 2025
View reviewed changes
messmerd
messmerd approved these changes Feb 28, 2025
View reviewed changes
Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

LGTM

@JohannesLorenz JohannesLorenz merged commit 3aa1a5d into LMMS:master Feb 28, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sakertooth sakertooth sakertooth left review comments

@messmerd messmerd messmerd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JohannesLorenz @messmerd @sakertooth