Add a .clang-format file

[LeonarddeR]

I invested an hour or two writing down a .clang-format that more or less follows the code style that is now common in OSARA.
@jcsteh What would you like? Should I add a reformatted version of one file as well? Then you and probably others can comment on changed lines you dislike.
Before merge we can revert these changes, then after merge I can provide a pr that should be with the whole code base formatted, including a .git-blame-ignore-revs. We should merge that using rebase and merge or a regular merge commit, not squashing.

Fixes #923

[AppVeyorBot]

Build succeeded! Build osara pr940-1178,7c33d940 completed (commit 7c33d940dd by @LeonarddeR)
Downloads:

Windows

• installer/osara_pr940-1178,7c33d940.exe
• locale/osara_pr940-1178,7c33d940.pot

Mac

• installer/osara_pr940-1178,7c33d940.dmg

[AppVeyorBot]

Build succeeded! Build osara pr940-1180,ac9dc06b completed (commit ac9dc06b9e by @LeonarddeR)
Downloads:

Windows

• installer/osara_pr940-1180,ac9dc06b.exe
• locale/osara_pr940-1180,ac9dc06b.pot

Mac

• installer/osara_pr940-1180,ac9dc06b.dmg

[AppVeyorBot]

Build succeeded! Build osara pr940-1181,dba8a65f completed (commit dba8a65fdd by @LeonarddeR)
Downloads:

Windows

• installer/osara_pr940-1181,dba8a65f.exe
• locale/osara_pr940-1181,dba8a65f.pot

Mac

• installer/osara_pr940-1181,dba8a65f.dmg

[jcsteh]

Thanks very much for working on this.

I think it might be worth seeing the diff for reaper_osara.cpp. It's the biggest and oldest file, so this will give us a good idea of how much churn there will be and how we might minimise it.

[jcsteh]

I think there are some edge cases where include order matters; e.g. see the inclusion of osara.h in fxChain.cpp and others. Will this break that?

[LeonarddeR]

I'll have a look and will act accordingly.

[LeonarddeR]

Right, we definitely want to leave include ordering alone.

[LeonarddeR]

I addressed your comments and added one commit with the whole code base formatted, so you can have a look on what it will do.

[AppVeyorBot]

Build succeeded! Build osara pr940-1182,c89b942e completed (commit c89b942ec6 by @LeonarddeR)
Downloads:

Windows

• installer/osara_pr940-1182,c89b942e.exe
• locale/osara_pr940-1182,c89b942e.pot

Mac

• installer/osara_pr940-1182,c89b942e.dmg

[AppVeyorBot]

Build failed! Build osara pr940-1183,a69381b2 failed (commit a69381b2dd by @LeonarddeR)

[jcsteh]

Not a strange question at all. The reason is that xgettext doesn't support some of the things we need such as .rc files and string lists (translateFirstString, etc.).

[LeonarddeR]

Actually with my last change, I think we are now pretty safe. WhitespaceSensitiveMacros will just leave alone the translate and translate_plural calls.

[LeonarddeR]

I just took a different tack and took the Microsoft style as a basis. This seems to be going a lot better.

[LeonarddeR]

O wait, Microsoft doesn't have a maximum column count it seems. Things go wild again if we limit the amount of columns on 80.
I'm a bit surprised that clang format makes things so difficult for us.

[AppVeyorBot]

Build succeeded! Build osara pr940-1184,95c113b0 completed (commit 95c113b0e1 by @LeonarddeR)
Downloads:

Windows

• installer/osara_pr940-1184,95c113b0.exe
• locale/osara_pr940-1184,95c113b0.pot

Mac

• installer/osara_pr940-1184,95c113b0.dmg

[AppVeyorBot]

Build succeeded! Build osara pr940-1185,32beb394 completed (commit 32beb39469 by @LeonarddeR)
Downloads:

Windows

• installer/osara_pr940-1185,32beb394.exe
• locale/osara_pr940-1185,32beb394.pot

Mac

• installer/osara_pr940-1185,32beb394.dmg

[jcsteh]

Wo. What on earth is going on here? It seems to keep adding continuation indents even though each translate call is closed. I wonder if WhitespaceSensitiveMacros is interfering here somehow?

[jcsteh]

It does seem that AlignAfterOpenBracket: DontAlign makes this worse. But it's definitely very confused. If I add random indentation to these lines, it preserves that even with AlignAfterOpenBracket: BlockIndent. So I guess the whitespace sensitivity applies to the line before the macro, not just inside the macro, which is very unfortunate.

I think I'm going to need to fix makePot... somehow.

[LeonarddeR]

I'm not sure. There's also some black magic going on in uia.cpp I referenced earlier, which I think is related. It really looks like we are missing an alignment setting somewhere, but I'm pretty sure we have covered them all.

I'm starting to regret this all.

[jcsteh]

I really do appreciate you working on this. Even if we end up shelving it for a while, we can always come back to it.

I'll look at fixing makePot at some point to cope with multi-line calls. I have some ideas on how it might be done and that's probably a good idea for other reasons anyway.

Losing translateFirstString is a bit of a shame, but I guess that was always a bit sketchy anyway, and not having to worry about code formatting eventually (which is a total waste of people's time) probably outweighs that loss.

[LeonarddeR]

Sure. I will mark this as draft for now.

[Timtam]

I'm curious as to what you're suggesting is a more modern tool. As I understand it, clang-format is the industry standard these days.

I'm not too deep into "the industry" anymore. Last time I was working for a tech company they were doing all the formatting with a pre-commit hook through the IDE, namely IntelliJ. Dunno if that one uses clang-format under the hood, but I kinda doubt it. I know other people who do all the formatting in IDE too, tools like clang-format, prettier and such usually just are fallbacks there, but obviously that is nowhere near the entire picture.

We could just reformat the whole code base without caring, but that isn't great for anyone already familiar with contributing to the project.

IMO the advantage of not having to care about the entire formatting at all is way, way more important than keeping the current formatting in tact. I watched Scott and Jen navigate the codebase multiple times by now, kinda all that matters is that you can count the braces and parens there, indentation is there but usually gets skipped anyway, and nooone is entirely sure how to indent properly anyway in specific cases like line breaks within a function call. IMO the actual way of formatting isn't as important as having the ability to just write down your code without having to care about indents and stuff, hit a button before committing and just ignore code formatting at all. That is basically what all my sighted colleagues do too from what I can tell. Noone nowadays cares about counting tabs and breaking lines correctly.

Also, there are sections of the code that have been formatted the way they have for very specific reasons, so we need to consider the trade-offs there.

Just put them onto a ignore list and don't format them at all? I know, sounds easier than it probably is, and I know you discussed that already. Could you give me a rundown why exactly that doesn't work properly with clang-format? I think I remember something along the lines of clang-format messes other things up if you do that? That might be one reason why it might be worth investigating other tools to see if they handle this case better.

[jcsteh]

I totally hear you on the advantages of reformatting the whole code base and just not caring about it from there. That's certainly been life changing in my work at Mozilla, for example. However, there are still trade-offs when figuring out what style you want, etc. Ideally, you don't want every single line of the code base to be rewritten during the initial reformat. Some of the formatting we were seeing with some of this work was really hard to read for some cases.

It doesn't help that our translation code is currently pretty brittle and relies on regular expressions, so it gets rather unhappy if things break across lines in ways it doesn't expect. We can't use xgettext because it doesn't support .rc files as far as I know, among other things.

Ignoring things works fine in clang-format. But then you don't get the advantages of having it consistent in that section of the code. This is totally a case of me wanting to have my cake and eat it too, but consider the lines we have in our command registration tables, just as an example:

{MAIN_SECTION, {{0, 0, 40285}, nullptr}, nullptr, cmdGoToNextTrack}, // Track: Go to next track

Because that line is long, any formatter is going to split it across multiple lines. You could end up with something like this:

{
  MAIN_SECTION,
  {{0, 0, 40285}, nullptr},
  nullptr,
  cmdGoToNextTrack
}, // Track: Go to next track

So now the registration for every command is 6 lines instead of 1. Given that this table is already 215 lines long, that means it would now be about 1290 lines long. This is just one example that comes to the top of my mind.

[jcsteh]

There's also the fact that this would break all existing open pull requests, some of which we can't merge yet for various reasons.

[Copilot]

Pull Request Overview

Adds a C++ formatting configuration to standardize code style and updates Git attributes.

• Introduces a .clang-format file with OSARA-specific style rules based on Microsoft’s conventions.
• Updates .gitattributes to mark .clang-format as a text file.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
.gitattributes	Added .clang-format to text attributes
.clang-format	New configuration file defining code style settings

Comments suppressed due to low confidence (1)

.clang-format:89

• [nitpick] This comment is in past tense; consider rephrasing to an imperative form like 'Treat macros as complete statements.' to match other directives.

# Interpreted macros as complete statements.

[github-actions]

Build 178 succeeded!

• Download for Windows
• Download for Mac

[LeonarddeR]

@jcsteh What do you think about this? At some point, we might just want to settle on something that is not ideal per se

[github-actions]

Build 187 succeeded!

• Download for Windows
• Download for Mac

[jcsteh]

I made a few tweaks here. I set the ColumnLimit to 80, which does introduce a bunch more lines, but I think it's nicer than lines which drag on forever. It's not that I specifically care about 80 character lines, more that very long lines can get really intense. I actually couldn't figure out what value was implicitly being used here. Do you know what it was?

Do you know if there's a way to prevent clang-format from trying to align comments when it splits a trailing comment across lines? Even though we have AlignTrailingComments: never, it still wants to align them if it has to split them; e.g. in the command maps. IMO, visual alignment (vs semantic alignment) is never something we want, but I can't find a way to disable this case. That said, if I get rid of the ColumnLimit, I guess this problem goes away, so... hmm.

[jcsteh]

Oh, looks like it's 120. Maybe that's fine and I should leave this alone. What do you think?

[jcsteh]

I did a before and after with scons pot and it generates the same .pot file, minus comment line wrapping differences. So that's good.