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.

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

Add Meter Style Selection Option #1688

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

henkdegroot
Copy link
Contributor

This PR adds the option in the My Profile tab to allow the Meter Style type to be selected by the user.

Fixes #1658

@jujudusud
Copy link
Member

Do we have to update screenshots for website ?

@softins
Copy link
Member

softins commented May 15, 2021

Do we have to update screenshots for website ?

I think that can wait until after the 3.8.0 release. The change in this PR will most likely not be included until after that, so will be in the following release.

@hoffie hoffie added this to the Release 3.9.0 milestone May 15, 2021
@henkdegroot henkdegroot marked this pull request as draft May 20, 2021 16:16
@henkdegroot
Copy link
Contributor Author

henkdegroot commented May 20, 2021

Converted to Draft to allow an extra meter style option to be added. Would it make sense to rebase the PR on master before pushing again?
EDIT: Extra option added and rebased against current master

@henkdegroot henkdegroot force-pushed the MeterStyleSelection branch from a61b827 to f3f7393 Compare May 21, 2021 13:04
@henkdegroot henkdegroot marked this pull request as ready for review May 21, 2021 13:08
@henkdegroot henkdegroot force-pushed the MeterStyleSelection branch from f3f7393 to 130920c Compare June 3, 2021 05:12
@henkdegroot
Copy link
Contributor Author

Rebased on current Master and resolved conflicts.

@pljones
Copy link
Collaborator

pljones commented Jun 3, 2021

Keep an eye on the Clang-Format compliance checks.

@henkdegroot
Copy link
Contributor Author

Clang-Format compliance checks.

I actually did check them and at that moment I looked like the check failed in code which I have not changed....but checking again now and I see it also in code that I did change/add (I obvious did not run clang-format on my code before pushing again).

Will be updating this shortly and push again....

@ann0see
Copy link
Member

ann0see commented Sep 5, 2021

Probably not in 3.8.1, I'm afraid.

@pljones
Copy link
Collaborator

pljones commented Sep 5, 2021

Ooosch... wanted this to make it but I'd forgotten about it!

@ann0see
Copy link
Member

ann0see commented Sep 5, 2021

We still have time until Tuesday

@pljones pljones requested a review from gilgongo September 5, 2021 18:42
@pljones
Copy link
Collaborator

pljones commented Sep 5, 2021

I'd like @gilgongo's views as there are UI/UX changes here.

Oh - I saw a strange bug earlier (not entirely related but I better write it down now before I totally forget).

image
In the Fancy skin, the meter is wide enough for MAG's little widget to show without wrapping. Unfortunately........ in the Compact skin, it wraps.

This is with the Github build for:

38252048 2021-09-04 (HEAD -> master, upstream/master, origin/master, origin/HEAD) Unify JACK wording, fix bug, refactoring (#1889) [ann0see]

(so not quite latest but it was when I ran it).

@henkdegroot
Copy link
Contributor Author

Oh - I saw a strange bug earlier (not entirely related but I better write it down now before I totally forget).

See #1451 , though I thought that was fixed/merged already....but it seems not.

@pljones
Copy link
Collaborator

pljones commented Sep 5, 2021

Oh - I saw a strange bug earlier (not entirely related but I better write it down now before I totally forget).

See #1451 , though I thought that was fixed/merged already....but it seems not.

Raised #1994 (was 1993) with the fix. Needs more work (but not as much as 1993).

Copy link
Member

@gilgongo gilgongo left a comment

Choose a reason for hiding this comment

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

There are some minor glitches when changing through the different combinations, but mostly just a pixel getting pushed out here and there. Also notice that if small LEDs are chosen they don't seem to get used for the input meter sometimes? Not a big deal though.

@henkdegroot
Copy link
Contributor Author

Also notice that if small LEDs are chosen they don't seem to get used for the input meter sometimes? Not a big deal though.

This is by design. Compact settings apply to the mixer panel. It is something which should be detailed in the documentation (to prevent users registering issues for this as well).

The pixel shift, is that when changing from Fancy to a "Normal" view? Vaguely remember this might be caused by the difference in the slider.

@gilgongo
Copy link
Member

gilgongo commented Nov 7, 2021

Compact settings apply to the mixer panel. It is something which should be detailed in the documentation (to prevent users registering issues for this as well).

Ah I see.

The pixel shift, is that when changing from Fancy to a "Normal" view? Vaguely remember this might be caused by the difference in the slider.

Yes (I'm on Ubuntu 20.10 Gnome BTW). I think it may be unavoidable, and it's only in the transition - doesn't affect the view itself.

@gilgongo
Copy link
Member

gilgongo commented Nov 7, 2021

@jujudusud

Do we have to update screenshots for website ?

Yes, since the UI in settings has changed. Wish we had a better way of tracking PRs that need screenshot changes for each release.

I'm hoping the UI will be self-explanatory though and we don't have to add any text to Software-Manual.md

@henkdegroot
Copy link
Contributor Author

henkdegroot commented Nov 7, 2021

I'm hoping the UI will be self-explanatory

It should be the same as for Skins. The different skins are also not specifically shown on the website.

@gilgongo
Copy link
Member

gilgongo commented Nov 7, 2021

I would it is the same as for Skin. The different skins are also not specificallly shown on the website.

Yes, maintaining text in different languages is hard enough without also doing multi-language screenshots so best to have screenshots only when necessary really.

@gilgongo
Copy link
Member

gilgongo commented Nov 8, 2021

both as a pop-up and as an audible bleep

Which reminds me of this FR: #425

@gilgongo
Copy link
Member

gilgongo commented Nov 8, 2021

Anyway, on the subject of this PR... I think we can merge?

@pljones
Copy link
Collaborator

pljones commented Nov 8, 2021

Yeah, I don't see anything I don't like :).

I'm going to get conflicts on at least one of my work-in-progress branches... but that's my problem.

@pljones pljones merged commit 5c21445 into jamulussoftware:master Nov 8, 2021
@henkdegroot henkdegroot deleted the MeterStyleSelection branch November 9, 2021 19:59
@gilgongo gilgongo modified the milestones: Release 3.9.0, 3.8.2 Jan 17, 2022
@pgScorpio
Copy link
Contributor

I love the new meter styles, especially for use in the compact skin, but to be honest I would prefer a bar indicator with 3 colors. Is there a possibility for that too?

@pljones
Copy link
Collaborator

pljones commented Feb 4, 2022

You mean the one that's currently just green? Yeah...

The added advantage of the "LEDs" is that you can say, "Avoid the top one completely, only let the second one light occasionally, the next two mean 'good level' towards 'loud' and below that starts getting quiet very quickly". With only colour to go on, you're relying on someone being happy with "green", "yellow" and "red". For red/green colour blind, it might as well just be the single colour.

But having the option for those who can use it would be nice.

@pgScorpio
Copy link
Contributor

pgScorpio commented Feb 5, 2022

You mean the one that's currently just green? Yeah...

Yes I mean those we always had in the compact skin.

The added advantage of the "LEDs" is that you can say, "Avoid the top one completely, only let the second one light occasionally, the next two mean 'good level' towards 'loud' and below that starts getting quiet very quickly". With only colour to go on, you're relying on someone being happy with "green", "yellow" and "red". For red/green colour blind, it might as well just be the single colour.

If you, like me, regularly work with very large groups (50-100 participants), and so using compact skin and multiple fader rows, the leds become somewhat hard to follow (so many blinking dots)..... The bars are easier to follow, but then you have no clear indication of the correct level...

But having the option for those who can use it would be nice.

So Yes, it would be very nice!

P.S. Leds for Input L/R and 3 color bars for faders would be even nicer ;=)

@pgScorpio
Copy link
Contributor

pgScorpio commented Feb 5, 2022

Just found another bug on compact skin:

Changing from Round LEDS's to Small LEDS's doesn't change the fader width. (But changing to Narrow Bar does.)

Also the naming seems to be inconsistent and not logical when sorted on name in the list.
I suggest:
Bar (Narrow)
Bar (Wide)
LED's (Round)
LED's (Small)
LED's (Square)

:

@henkdegroot
Copy link
Contributor Author

(But changing to Narrow Bar does.)

Correct noticed and this is as per design. The narrow bar is using the minimal space required. All LED options use a little more space. To prevent impact on appearance and usage, the size of the narrow bar has been kept the same as in the current version when using the compact skin. It is not intended that someone will keep switching between the different styles, and should therefore not be an issue.

not logical when sorted on name in the list.

The current order of the list is the 3 existing options first and added new modes/options at the bottom.
This could be improved/changed but not sure if that is really needed.

naming seems to be inconsistent

The Bar (Wide) sound incorrect to me, true that it is wider compared to Narrow but it is not wider compared to the LED's.
The LED's (Square) also seems not correct to me as they are not really square. Not sure what the correct "shape" name would be, rectangle would come closer to the actual shape. Currently just went for "without" any shape reference to make it a more "default" choice which matches the current versions used in Fancy/Normal/Compact skin.

@pljones
Copy link
Collaborator

pljones commented Feb 5, 2022

LED's (Square)

Could be "Stripe"?

@henkdegroot
Copy link
Contributor Author

Leds for Input L/R and 3 color bars for faders would be even nicer ;=)

Guess an improvement could be implemented to allow independent selection of the Input Meter style vs Mixerboard.

@pgScorpio
Copy link
Contributor

Guess an improvement could be implemented to allow independent selection of the Input Meter style vs Mixerboard.

That would be very appreciated!

@pgScorpio
Copy link
Contributor

(But changing to Narrow Bar does.)

Correct noticed and this is as per design. The narrow bar is using the minimal space required. All LED options use a little > more space. To prevent impact on appearance and usage, the size of the narrow bar has been kept the same as in the > > current version when using the compact skin. It is not intended that someone will keep switching between the different > styles, and should therefore not be an issue.

But when changing from "small LED's" to "Round LED's" the fader width is adjusted, but when changing back to "small LED's" it stays wide. Only switching to "Narrow Bar" gives the narrow fader again.
And "Small LED's" seem to fit into the same fader width as "Narrow Bar", as changing from "Narrow Bar" to "Small LED's" does not change the total fader width.

But when changing the meter type the fader width should always depend on the selected type, and it should not depend on a previous selected meter type. That's the bug I meant.

not logical when sorted on name in the list.

The current order of the list is the 3 existing options first and added new modes/options at the bottom. This could be improved/changed but not sure if that is really needed.

OK. But I think it would be more logical to group Bars and LED's in a logical order with a consistent naming format.

naming seems to be inconsistent

The Bar (Wide) sound incorrect to me, true that it is wider compared to Narrow but it is not wider compared to the LED's. The LED's (Square) also seems not correct to me as they are not really square. Not sure what the correct "shape" name would be, rectangle would come closer to the actual shape. Currently just went for "without" any shape reference to make it a more "default" choice which matches the current versions used in Fancy/Normal/Compact skin.

I think Wide/Narrow and Big/Small is not really incorrect since they are relative, and you can't compare Wide to Big or Square, since width, size and shape are different types of attributes.
Also "LEDS's Square" could also be named "LED Bar" as this type of led VU meters are commonly referred to as such.

I was more referring to LED's/Bar without prefix/suffix and those with prefix/suffix, but I understand that's related to your previous remark about "the 3 existing options".

So I still suggest changing the names for the official release, but this could also be:

Bar (Narrow)
Bar (Wide)
LED Bar (or LED's (Bar) or Bar (LED's) to be consistent in using a suffix)
LED's (Small)
LED's (Big)

or (Though I personally like the previous one better)

Narrow Bar
Wide Bar
LED Bar
Small LED's
Big LED's

@hoffie
Copy link
Member

hoffie commented Feb 6, 2022

But when changing from "small LED's" to "Round LED's" the fader width is adjusted, but when changing back to "small LED's" it stays wide. Only switching to "Narrow Bar" gives the narrow fader again.
And "Small LED's" seem to fit into the same fader width as "Narrow Bar", as changing from "Narrow Bar" to "Small LED's" does not change the total fader width.
But when changing the meter type the fader width should always depend on the selected type, and it should not depend on a previous selected meter type. That's the bug I meant.

I can confirm and have opened #2351 to track this.

@henkdegroot
Copy link
Contributor Author

But when changing from "small LED's" to "Round LED's" the fader width is adjusted, but when changing back to "small LED's" it stays wide. Only switching to "Narrow Bar" gives the narrow fader again. And "Small LED's" seem to fit into the same fader width as "Narrow Bar", as changing from "Narrow Bar" to "Small LED's" does not change the total fader width.

But when changing the meter type the fader width should always depend on the selected type, and it should not depend on a previous selected meter type. That's the bug I meant.

Thank you for spotting this, and thanks to @hoffie for already getting a fix for this.

@henkdegroot
Copy link
Contributor Author

So I still suggest changing the names for the official release, but this could also be:

Bar (Narrow) Bar (Wide) LED Bar (or LED's (Bar) or Bar (LED's) to be consistent in using a suffix) LED's (Small) LED's (Big)

or (Though I personally like the previous one better)

Narrow Bar Wide Bar LED Bar Small LED's Big LED's

Created #2353 for this.

@ann0see
Copy link
Member

ann0see commented Feb 26, 2022

Oh - I saw a strange bug earlier (not entirely related but I better write it down now before I totally forget).

Interesting: I also saw this happening with Qt6 #2363

@hoffie
Copy link
Member

hoffie commented Feb 26, 2022

Oh - I saw a strange bug earlier (not entirely related but I better write it down now before I totally forget).

Interesting: I also saw this happening with Qt6 #2363

Not sure if the strange emoji splitting in @pljones's screenshot is what he meant, but this should have been solved by #1451 (and its multiple related PRs) with 3.8.2.

@ann0see
Copy link
Member

ann0see commented Feb 26, 2022

No it's about the white bars in the screenshot.

Edit: they seem to be screenshots merged together. So this picture is unrelated.

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.

Allow meter style selection to be used in UI
9 participants