Improve Meterstyle Naming options by henkdegroot · Pull Request #2356 · jamulussoftware/jamulus

henkdegroot · 2022-02-07T20:13:18Z

Short description of changes

Improve Meterstyle Naming and Sort Order

Context: Fixes an issue?

Fixes: #2353

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

Complete

What is missing until this pull request can be merged?

Needs review, tested on Windows

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

hoffie

Thanks! Looks good to me.

Should we also update the enum entries to match the UI names in this PR? E.g. MT_BAR_NARROW instead of MT_SLIM_BAR (I don't care about the word order, but we should use the same adjective at least?).

hoffie · 2022-02-07T20:26:17Z

src/util.h

 enum EMeterStyle
 {
    // used for settings -> enum values should be fixed
-    MT_LED       = 0,
+    MT_SLIM_BAR  = 0,
    MT_BAR       = 1,
-    MT_SLIM_BAR  = 2,
-    MT_SLIM_LED  = 3,
-    MT_SMALL_LED = 4
+    MT_LED       = 2,
+    MT_SMALL_LED = 3,
+    MT_SLIM_LED  = 4
 };



As far as I understand, this has been introduced in 130920c, which was not part of any (final) release, so it's ok to re-order this now (but not any later, as it's used in the ini settings).

$ git tag --contains 130920c3d63104726ee00dd11e19c20e2c319d86 r3.8.1devNightly1 r3_8_2beta1

hoffie · 2022-02-07T20:27:03Z

src/clientsettingsdlg.cpp

+                                       "Bar (Narrow) and LEDs (Small) options only apply to the mixerboard. When "
+                                       "Bar (Narrow) is selected, the input meters are set to Bar (Wide). When "
+                                       "LEDs (Small) is selected, the input meters are set to LEDs (Big). "


Just to note: This causes translation changes as well.

hoffie · 2022-02-07T20:33:18Z

src/clientsettingsdlg.cpp

+    cbxMeterStyle->addItem ( tr ( "LEDs (Stripe)" ) ); // MT_LED
+    cbxMeterStyle->addItem ( tr ( "LEDs (Small)" ) );  // MT_SMALL_LED
+    cbxMeterStyle->addItem ( tr ( "LEDs (Big)" ) );    // MT_SLIM_LED


The distinction still sounds a bit strange to me. Example: "You you like your LEDs rather as Stripes, Small or Big?" The words in brackets do not describe the same type of thing. I've got no better idea though. The following would be more accurate, but probably to long:

LEDs (striped)

LEDs (round, small)

LEDs (round, big)
This may quickly get into the bike-shedding area, so we should do the best we can but should merge nevertheless even if we think it's not perfect... :)

Should the adjectives be lowercase? (No idea, not a native speaker)

You you like your LEDs rather as Stripes, Small or Big?

What is "rather" doing in that? And why make "Stripe" plural? You're changing things and then complaining it doesn't sound consistent... ;)

Happy to update this, and would like to update the enum entries at the same time with similar logic.
So should we take this proposal (with lowercase) as the option to go for?

(I am not a native speaker as well)

What is "rather" doing in that? And why make "Stripe" plural? You're changing things and then complaining it doesn't sound consistent... ;)

I was trying to place the different options in an English sentence to see how they would sound.
What I was trying to say:

Stripe is not a size. There could be small or big stripes (in theory, I'm not suggesting we add them)

Small and Big are sizes, but don't imply any form (in contrast to Stripe)

I don't think we have to worry about the total length of the text, as the "language" box just below the setting in the GUI will be wider. Good point that Small and Big no longer indicate the shape, so adding "round" would make sense to me.

hoffie · 2022-02-07T20:33:59Z

Tested on Linux: All labels do what they say. The proper value is also restored upon restart.

pljones

Looks okay.

henkdegroot · 2022-02-07T21:13:22Z

E.g. MT_BAR_NARROW instead of MT_SLIM_BAR

@hoffie, the "slim/small" LED reference is also used in the bitmap references, i.e. BitmCubeSlimLedGreen, BitmCubeSmallLedGreen. Should these be updated as well when the enum entries are updated?

hoffie · 2022-02-07T21:29:56Z

the "slim/small" LED reference is also used in the bitmap references, i.e. BitmCubeSlimLedGreen, BitmCubeSmallLedGreen. Should these be updated as well when the enum entries are updated?

I think it the naming should be as consistent as possible, yes. In general, aim for the Principle of least surprise, I'd say. :)
As far as I can see, those variables are also new (as in: they've not been part of a final release), so there should be little risk of breaking things, right?

So, yes, I'd be in favor of naming those references with the same words which are used in other places (enum, labels).

pljones · 2022-02-08T17:21:44Z

One thing, when this is released, we'll still need to highlight that anyone who's tested a post-3.8.1 build that had the new config item will need to expect that to mean something different following this change.

hoffie

Thanks a lot for working on this!
All of the changes sound meaningful. I've re-tested the behavior and everything looks fine.

This is a huge improvement at code readability. :)

Should be squash-merged.

hoffie · 2022-02-08T19:58:11Z

src/levelmeter.cpp

+    BitmCubeRoundBigLedBlack ( QString::fromUtf8 ( ":/png/LEDs/res/CLEDBlackSmall.png" ) ),
+    BitmCubeRoundBigLedGreen ( QString::fromUtf8 ( ":/png/LEDs/res/CLEDGreenSmall.png" ) ),
+    BitmCubeRoundBigLedYellow ( QString::fromUtf8 ( ":/png/LEDs/res/CLEDYellowSmall.png" ) ),
+    BitmCubeRoundBigLedRed ( QString::fromUtf8 ( ":/png/LEDs/res/CLEDRedSmall.png" ) )


It's a bit surprising for me to see "Big" being mapped to "Small", but I guess the reason is that this was the naming of the original files?
Should not be a blocker, IMO.

to see "Big" being mapped to "Small",

Good catch. The Small icons already existed....and is one of the reason why I used the "odd" names in the beginning. Though when started, it was still associated to a skin choice as well and later changed to changing the meter style as a separate option.

Tracking this in #2365 as a future improvement now.

Related: jamulussoftware#2349 jamulussoftware#2356

Update zh_CN app translation for #2356

hoffie added this to the Release 3.8.2 milestone Feb 7, 2022

hoffie reviewed Feb 7, 2022

View reviewed changes

pljones approved these changes Feb 7, 2022

View reviewed changes

hoffie approved these changes Feb 8, 2022

View reviewed changes

hoffie mentioned this pull request Feb 8, 2022

Prepare Release 3.8.2 #2247

Closed

56 tasks

Improve Meterstyle Naming

3117acd

henkdegroot force-pushed the improve-meterstyle-options branch from 449c0d0 to 3117acd Compare February 9, 2022 06:23

pljones approved these changes Feb 9, 2022

View reviewed changes

pljones merged commit 70a4f41 into jamulussoftware:master Feb 9, 2022

hoffie mentioned this pull request Feb 9, 2022

Levelmeter: Ensure that variables and png names (roughly) match #2365

Closed

henkdegroot deleted the improve-meterstyle-options branch February 9, 2022 11:33

ann0see added a commit that referenced this pull request Feb 9, 2022

Update .ts files for r3.8.2 after #2356

366dd0b

hoffie added a commit to hoffie/jamulus that referenced this pull request Feb 13, 2022

Update de_DE translation for 3.8.2 (Meter style)

b7b294f

Related: jamulussoftware#2349 jamulussoftware#2356

hoffie mentioned this pull request Feb 13, 2022

Update de_DE app translation for 3.8.2 (Meter style) #2387

Merged

hoffie added a commit to hoffie/jamulus that referenced this pull request Feb 13, 2022

Update de_DE translation for 3.8.2 (Meter style)

cde741f

Related: jamulussoftware#2349 jamulussoftware#2356

BLumia added a commit to BLumia/jamulus that referenced this pull request Feb 14, 2022

Update zh_CN app translation for jamulussoftware#2356

1f87444

BLumia mentioned this pull request Feb 14, 2022

Update zh_CN app translation for #2356 #2391

Merged

ann0see added a commit that referenced this pull request Feb 14, 2022

Merge pull request #2391 from BLumia/new-string-2356

7d57231

Update zh_CN app translation for #2356

Comments

Conversation

henkdegroot commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

hoffie left a comment

Choose a reason for hiding this comment

Uh oh!

hoffie Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henkdegroot Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoffie commented Feb 7, 2022

Uh oh!

pljones left a comment

Choose a reason for hiding this comment

Uh oh!

henkdegroot commented Feb 7, 2022

Uh oh!

hoffie commented Feb 7, 2022

Uh oh!

pljones commented Feb 8, 2022

Uh oh!

hoffie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

henkdegroot commented Feb 7, 2022 •

edited

Loading

hoffie Feb 7, 2022 •

edited

Loading

henkdegroot Feb 7, 2022 •

edited

Loading