Skip to content

Conversation

@aliign
Copy link

@aliign aliign commented Oct 4, 2025

No description provided.

@rubiefawn
Copy link
Contributor

rubiefawn commented Oct 20, 2025

There's a couple of instances of "SF2/3" that have been mistakenly changed to "SoundFont/3". Please revert those to either "SF2/3" or at least restore the 2, like "SoundFont 2/3".

Edit: Literally all of these are unused Carla translations, which will be removed in #8013, so this isn't really a meaningful problem after all.

Copy link
Contributor

@rubiefawn rubiefawn left a comment

Choose a reason for hiding this comment

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

I have some additional non-blocking feedback. Whether you make these changes or not, I will extend approval regardless. See below.

In this PR, you touch several lines of old code that do not follow our coding conventions. Since you're updating these lines, you might as well take this opportunity to update the code style as well:

  • Function parameters: Some of the function parameters begin with an underscore, e.g. _instrument_track should be instrument_track. These parameters should be renamed to remove the underscore.
    • There's a few function parameters named _this; just rename these to el, since this is a keyword.
  • Braces: Extra spaces shouldn't be inside parentheses, e.g. ( thing ) should be (thing).
  • Pointers & references: Pointer/reference specifiers (*, &) should cozy up to the type, e.g. QDomDocument & doc should be QDomDocument& doc.

Good work!

const QString SAMPLES_PATH = "samples/";
const QString GIG_PATH = "samples/gig/";
const QString SF2_PATH = "samples/soundfonts/";
const QString SoundFont_PATH = "samples/soundfonts/";
Copy link
Contributor

Choose a reason for hiding this comment

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

(Non-blocking) Per the Coding Conventions:

Global scope constants SHALL use UpperCamelCase.

Suggested change
const QString SoundFont_PATH = "samples/soundfonts/";
const QString SoundFontPath = "samples/soundfonts/";

It may look out of place next to all the other UPPER_SNAKE_CASE constants; you may update those too if you feel like doing so.

@Veratil
Copy link
Contributor

Veratil commented Oct 25, 2025

"SOUNDFONT" is trademarked, we probably shouldn't rename this.

@rubiefawn
Copy link
Contributor

"SOUNDFONT" is trademarked

I didn't realize this, this is a pretty big problem. Should it be "sfplayer" then? What are some other options for renaming this?

@rubiefawn rubiefawn mentioned this pull request Oct 25, 2025
5 tasks
@aliign
Copy link
Author

aliign commented Oct 28, 2025

I'm quite caught up this week with university stuff, I'll see about getting to changing to sfplayer and implementing the code practices above this weekend.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants