-
Notifications
You must be signed in to change notification settings - Fork 223
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
base: main
Are you sure you want to change the base?
#1420 Fader Settings improvements #3151
Conversation
Unfortunately this is NOT what I intendended. Changing text does not change the behaviour. Though adding the "clear stored" options can overcome our problem it will take two actions instead of one. |
OK, I must be not understanding what you're after. Could you write it out long-hand for me, please. |
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? |
6876f7c
to
316537e
Compare
d5157f3
to
8ab8893
Compare
8ab8893
to
c3c844a
Compare
c9d2ef2
to
94315fe
Compare
94315fe
to
26b63fa
Compare
26b63fa
to
873856e
Compare
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? |
@pgScorpio should comment on that. |
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.)
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. |
873856e
to
9e76544
Compare
9e76544
to
d73703c
Compare
MacOS Legacy build failing for
|
unrelated to this PR. I've tried tracking it down with no success so far. |
d73703c
to
b423de8
Compare
b423de8
to
393a8f2
Compare
86893e3
to
7acdbd0
Compare
7acdbd0
to
b91b8dd
Compare
3bfc1d5
to
95fcc5b
Compare
@@ -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 ); |
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
95fcc5b
to
b51bf82
Compare
b39d0d3
to
68b735c
Compare
12b377a
to
d2af254
Compare
d2af254
to
57949d8
Compare
f71fd57
to
876f746
Compare
7339164
to
6aa5fc5
Compare
b988591
to
44589b6
Compare
44589b6
to
6595a18
Compare
5e4ce37
to
a93eadb
Compare
a93eadb
to
814b186
Compare
f514dc6
to
ffe511b
Compare
ffe511b
to
5091438
Compare
5091438
to
b225783
Compare
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
This has been reworded to make its behaviour clear
"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:
CHANGELOG: Client: Fader Settings improvements
Context: Fixes an issue?
Fixes: #1420
Does this change need documentation? What needs to be documented and how?
Yes!
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?
Checklist