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

#1420 Fader Settings improvements #3151

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Aug 23, 2023

Short description of changes

Two changes relating to #1420:

  • Edit -> Set All Faders to New Client Level
    This retained the last known level for any client, so when that client rejoined the server, their old level was restored
    image
    This has been reworded to make its behaviour clear
    image
    "Set Current Faders to New Client Level" will be the behaviour that is intended - it will not take any stored fader levels into account. (Of course, these fader levels will then apply should a "current" client leave and rejoin.)

  • Edit -> Stored Fader Settings
    This new sub-menu replaces "Clear All Stored Solo and Mute Settings" and adds finer-grained control, including allowing all settings to be purged:
    image
    image

CHANGELOG: Client: Fader Settings improvements

Context: Fixes an issue?

Fixes: #1420

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

Yes!

  • App translations are affected
  • Site documentation is affected

Status of this Pull Request

It runs and looks okay. I don't generally worry about stored fader settings, so I've not got much to test on. It could do with several people who use the feature heavily looking at it.

@pgScorpio, hopefully this is what you were after.

What is missing until this pull request can be merged?

  • Code review.
  • More testing by people who use the feature.
  • Usability feedback.

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

@pljones pljones self-assigned this Aug 23, 2023
@pljones pljones added this to the Release 3.11.0 milestone Aug 23, 2023
@ann0see ann0see requested a review from gilgongo August 23, 2023 10:17
@pgScorpio
Copy link
Contributor

Unfortunately this is NOT what I intendended.

Changing text does not change the behaviour.
See wanted behaviour at #1420.

Though adding the "clear stored" options can overcome our problem it will take two actions instead of one.
Also splitting the (very often used) reset mute and solo seams a bit odd to me, since this now, most of the time, will take two actions instead of one also. So maybe we could (re-)add a "clear (current) mute and solo settings" too ?

@pljones
Copy link
Collaborator Author

pljones commented Aug 23, 2023

OK, I must be not understanding what you're after.

Could you write it out long-hand for me, please.

@ann0see
Copy link
Member

ann0see commented Aug 24, 2023

I believe we might have two options then? Reset all client levels to default (= clear ini file = reset all faders) and reset currently connected clients to default?

@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch 3 times, most recently from 6876f7c to 316537e Compare August 28, 2023 12:30
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch 2 times, most recently from d5157f3 to 8ab8893 Compare September 12, 2023 16:21
@pljones pljones marked this pull request as draft September 18, 2023 17:27
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 8ab8893 to c3c844a Compare September 18, 2023 18:05
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch 2 times, most recently from c9d2ef2 to 94315fe Compare October 2, 2023 17:54
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 94315fe to 26b63fa Compare October 5, 2023 17:45
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 26b63fa to 873856e Compare October 15, 2023 14:17
@gilgongo
Copy link
Member

gilgongo commented Oct 27, 2023

I believe we might have two options then? Reset all client levels to default (= clear ini file = reset all faders) and reset currently connected clients to default?

What's the scenario in which you'd set the level, but then opt to apply that to current but not to new clients (or vice versa, but I suppose that's when you don't want to change the current mix)? Maybe I'm confusing myself...

Also feels like these functions would be better on the same screen as the level setting, assuming the majority case is to set the level then apply it to clients in some way?

@gilgongo gilgongo added the needs documentation PRs requiring documentation changes or additions label Oct 27, 2023
@ann0see
Copy link
Member

ann0see commented Nov 1, 2023

@pgScorpio should comment on that.

@pgScorpio
Copy link
Contributor

I believe we might have two options then? Reset all client levels to default (= clear ini file = reset all faders) and reset currently connected clients to default?

What's the scenario in which you'd set the level, but then opt to apply that to current but not to new clients (or vice versa, but I suppose that's when you don't want to change the current mix)? Maybe I'm confusing myself...

My idea too... That's why I just opted that "Set all levels to new client level" should clear stored levels too, nothing more than that. (And I really don't know why somebody decided otherwise and added all kinds of other functionality too.)

Also feels like these functions would be better on the same screen as the level setting, assuming the majority case is to set the level then apply it to clients in some way?

I can understand your feeling/assumption, though this is certainly not always the case... This function is often used to reset the mix between songs, without changing new client level.
I think, to be consistent, the reset function is quite ok at the current location... (Always accessible within two mouse clicks, and no need to open settings.)

@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 873856e to 9e76544 Compare November 2, 2023 18:28
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 9e76544 to d73703c Compare December 9, 2023 11:12
@pljones
Copy link
Collaborator Author

pljones commented Dec 10, 2023

MacOS Legacy build failing for dmg build.

==> Downloading https://ghcr.io/v2/homebrew/core/create-dmg/manifests/1.2.1
==> Fetching create-dmg
==> Downloading https://ghcr.io/v2/homebrew/core/create-dmg/blobs/sha256:a1d0ce1a3da65fb140ef2740dd778066c9f26122f5fb58fd64f4200bb168fc85
==> Pouring create-dmg--1.2.1.all.bottle.tar.gz
🍺  /usr/local/Cellar/create-dmg/1.2.1: 12 files, 67.9KB
Creating disk image...
hdiutil: create failed - No child processes
Error: Process completed with exit code 1.

@hoffie
Copy link
Member

hoffie commented Dec 10, 2023

MacOS Legacy build failing for dmg build.

unrelated to this PR. I've tried tracking it down with no success so far.

@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from d73703c to b423de8 Compare December 14, 2023 17:58
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from b423de8 to 393a8f2 Compare January 14, 2024 10:24
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch 2 times, most recently from 86893e3 to 7acdbd0 Compare February 18, 2024 11:43
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 7acdbd0 to b91b8dd Compare March 5, 2024 17:04
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from b91b8dd to 3bfc1d5 Compare March 15, 2024 17:09
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 3bfc1d5 to 95fcc5b Compare March 28, 2024 16:46
@@ -298,9 +298,17 @@ CClientDlg::CClientDlg ( CClient* pNCliP,
// Edit menu --------------------------------------------------------------
QMenu* pEditMenu = new QMenu ( tr ( "&Edit" ), this );

pEditMenu->addAction ( tr ( "Clear &All Stored Solo and Mute Settings" ), this, SLOT ( OnClearAllStoredSoloMuteSettings() ) );
QMenu* pFaderMenu = new QMenu ( tr ( "Stored &Fader Settings" ), this );
Copy link
Member

Choose a reason for hiding this comment

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

&F is already used in the same menu for Auto-Adjust all &Faders


pEditMenu->addAction ( tr ( "Set All Faders to New Client &Level" ),
pEditMenu->addMenu ( pFaderMenu );
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be the first time Jamulus has used a cascading menu. Works fine on desktop devices, but do we know if it is friendly to Android and iOS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's a screenshot from my phone
Screenshot_20240328-175036

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The iconography isn't clear -- the higher level menu doesn't indicate that the first item is a sub-menu. But that's par for the course with Qt on Android.

@pljones pljones modified the milestones: Release 3.11.0, Release 3.12.0 May 3, 2024
@pljones pljones removed their assignment May 6, 2024
@pljones pljones modified the milestones: Release 3.12.0, Release 4.0.0 May 6, 2024
@pljones pljones force-pushed the feature/1420-rework-storedfader-settings branch from 95fcc5b to b51bf82 Compare May 6, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs documentation PRs requiring documentation changes or additions
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

"Set all faders to new client level" only affects current users
6 participants