Adds the name of the string note to the open string tooltips (fixed)

[xanathar]

This change adds the name of the note in the tooltip, as in
.

This makes it easier to understand what is going on with open strings.

Notes about the change:

• Gets around the localization problem by putting the name of the note between round parenthesis
• Works with both left handed and right handed layouts (sorry for the bug and noise in Adds the name of the string note to the open string tooltips. #48 - I feared it could accidentally be merged so I yanked it.
• Much cleaner than before, now the logic is driven entirely by chord_diagram that updates the note_name in the top toggle.

Final closure: this is of course an unsolicited change... if you think the product is better without, feel free to reject.

[bragefuglseth]

Hi, and thanks a lot for the contribution! This looks like a good idea to me. Two suggestions:

1. It would make sense to show the note names for muted/unopen strings as well (e.g. "Muted (G)").
2. The way the tooltip text is formatted here is a subtle case of sentence splitting, which is considered bad i18n practice. The entire format string should be made translatable as a single string, so usage and placement of the parentheses can be localized as well. Because of lacking Rust support in xgettext, Fretboard uses the i18n-format workaround crate for cases like this. See the no variants page code for a usage example. To make things easier for translators, a comment should be added explaining what each {block} is replaced with.

Otherwise this LGTM. Thanks again for taking the time to work with Fretboard's currently messy codebase :-)

[xanathar]

Thanks for the pointers, I'll check the localization stuff and send a new revision (and probably learn something new in the process :)).

What is the expectation for notes and chord names though? At the moment (as far as I can see, at least, I did a quick check with it_IT and de_DE) they are not localized. Shall I put a note for the translators not to translate the note names or, otoh, is the future direction to also translate names/chords (not trivial, I guess)?

[bragefuglseth]

Fretboard does not have localization of note names, so the individual note names are not marked as translatable; they're inserted into the formatted strings as-is. Just add a comment asking for the {} blocks to be kept intact, and explain what they're used for. For i18n, it's also a good idea to name the parameters, so translators can reorder the blocks freely within the string.

[bragefuglseth]

For i18n, it's also a good idea to name the parameters, so translators can reorder the blocks freely within the string.

Seems like this particular thing isn't possible with i18n-format, so ignore that. Just note down the order of the parameters in a comment above the code ("the first {} block is replaced with x, the second with y", and so on) 🙂

[bragefuglseth]

FWIW I'll do a new release with this and a few other improvements once this has been merged 🙂

Just take your time, though, I have other things to work on in the meantime :)

[xanathar]

Hi, sorry for taking long on this, but the weekend has been busy.

Anyway, I didn't have much time to work on the translation issue, but I had enough to change my mind about sixteen times :). Essentially:

• I can i18n_fmt!(i18n_fmt("{} ({})", gettext("Open"), self.imp().note_name.get())) ... but feels a little wrong. It's probably still better than a format!(...), but creates a little conflict between the translation of "Open" and "Muted" and the string composition. Also, the format string is generic enough that it could lead to conflicts (e.g. being the same format for Open and Muted).
• ... or I can i18n_fmt!(i18n_fmt("Open ({})", self.imp().note_name.get())) which is a lot cleaner, allows for any kind of different structured sentences and doesn't risk overlapping with unrelated format strings. But, it's a brand new translation that needs to be added to all languages; it's trivial to add, but there are a lot of languages to be fixed, and the translations would be broken in the meantime.
• ... or a third option that at the moment I can't see.

[bragefuglseth]

The second one sounds fine to me. Translations will come eventually, and it’s not a very high-exposure area in the app.

[xanathar]

The rust change is ready, I will push a new version later.

But: how are you updating the .pot file? I tried with xgettext but didn't get a perfect result yet but, looking at po/meson.build it seems meson should do the extraction automatically? However, I could not get the correct meson incantation to get the files updated (meson dist fails on my system though, so it might be that one).

[bragefuglseth]

Weblate does that for us, we don't need to worry about it at all :)

[xanathar]

I just pushed the updated version, then!

[bragefuglseth]

Looks good! The "Not Open" tooltip could also have the note name, but I'll just add that myself. Thanks again for your contribution!