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

Added a "Undo Idle action upon activity" setting. #3068

Merged
merged 1 commit into from May 14, 2017

Conversation

@AndrewJDR
Copy link
Contributor

commented May 6, 2017

Added a new "Undo Idle action upon activity" checkbox underneath the idle action area in the Audio Input settings.

When this checkbox is enabled, the idle action will be reversed as soon as the user returns from idle. (i.e. If the idle action mutes you, when you're active again, it unmutes. If the idle action deafened you, when you're active again, it undeafens you). I tried to keep this as simple as possible to increase the chances of it being merged.

I know there are other, more ambitious efforts around this such as #1724, but it appears to be trying to do a lot of things and has been stalled for nearly a couple of years now. I'd very much like to get this merged, so feedback is welcome.

@AndrewJDR AndrewJDR force-pushed the AndrewJDR:master branch 3 times, most recently from 4b1213a to 3c736a2 May 6, 2017

@mkrautz mkrautz self-requested a review May 7, 2017

@mkrautz mkrautz self-assigned this May 7, 2017

@mkrautz
Copy link
Member

left a comment

I've requested changes because I feel that the activityState variable should always be up-to-date regardless of what our idle settings are.

Please let me know if my suggested changes are not sensible. Perhaps they don't make sense.

@@ -849,6 +849,9 @@ bool GlobalShortcutEngine::handleButton(const QVariant &button, bool down) {

if (down) {
AudioInputPtr ai = g.ai;
if (ai->activityState == AudioInput::ActivityStateIdle) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz May 7, 2017

Member

Move inside the if (ai.get()) { block below.

emit doMute();
}
}

if (g.s.iaeIdleAction != Settings::Nothing && g.s.bUndoIdleActionUponActivity) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz May 7, 2017

Member

Can we change this up to ensure the value of activityState is always consistent? I'd like activityState to always be truthful, even if the new setting is not turned on. We might use it for other things in the future.

Something like this:

if (activityState == ActivityStateExitedIdle) {
	activityState = ActivityStateActive;
	if (g.s.iaeIdleAction != Settings::Nothing && g.s.bUndoIdleActionUponActivity) {
			if (g.s.iaeIdleAction == Settings::Deafen && g.s.bDeaf) {
				emit doDeaf();
			} else if (g.s.iaeIdleAction == Settings::Mute && g.s.bMute) {
				emit doMute();
			}
		}
	}
}

(also, please fix the minor coding style issue: if( -- missing space between if and ()

@@ -815,13 +816,30 @@ void AudioInput::encodeAudioFrame() {

This comment has been minimized.

Copy link
@mkrautz

mkrautz May 7, 2017

Member

Can we change this code to always keep activityState up-to-date?

I think it would be something like:

This comment has been minimized.

Copy link
@mkrautz

mkrautz May 7, 2017

Member
if (((tIdle.elapsed() / 1000000ULL) > g.s.iIdleTime)) {
	activityState = ActivityStateIdle;
	tIdle.restart();
	if (g.s.iaeIdleAction == Settings::Deafen && !g.s.bDeaf) {
		emit doDeaf();
	} else if (g.s.iaeIdleAction == Settings::Mute && !g.s.bMute) {
		emit doMute();
	}
}

This comment has been minimized.

Copy link
@AndrewJDR

AndrewJDR May 8, 2017

Author Contributor

Yeah I think this is sensible. After your suggested change, one thing to note is that tIdle.restart() is being called more aggressively now -- not just when g.s.iaeIdleAction != Nothing, as it was before. I had avoided messing with that, since I wasn't sure exactly how tIdle was being used everywhere in the code.
Upon looking at how tIdle is used, I think it should be fine, so I made the change.

@@ -849,6 +849,9 @@ bool GlobalShortcutEngine::handleButton(const QVariant &button, bool down) {

if (down) {
AudioInputPtr ai = g.ai;
if (ai->activityState == AudioInput::ActivityStateIdle) {
ai->activityState = AudioInput::ActivityStateExitedIdle;

This comment has been minimized.

Copy link
@mkrautz

mkrautz May 7, 2017

Member

This is a data race. However, so is accessing ai->tIdle from the main thread like we do below.
Maybe add a comment:

// XXX: This is a data race: we write to ai->activityState (accessed by the AudioInput thread) from the main thread.

?

Alternatively, we should fix it by putting both of these accesses behind a lock. They're related, so it shouldn't be bad to do. But we can do it in a different PR.

This comment has been minimized.

Copy link
@AndrewJDR

AndrewJDR May 8, 2017

Author Contributor

Yeah, good point. I think a race problem in practice is unlikely here do to the Active -> Idle -> ReturnedFromIdle cyclic nature of this state variable, but that may change in the future and bite us if it's unprotected.

Once this PR is through, I can submit a separate one that locks tIdle / activityState behind a QMutex.

Added a "Undo Idle action upon activity" setting.
When enabled, the idle action will be reversed as soon as the user returns from idle.

@AndrewJDR AndrewJDR force-pushed the AndrewJDR:master branch from 3c736a2 to 48c2cc3 May 8, 2017

@AndrewJDR

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

@mkrautz Just a heads up -- I made the changes you requested and re-pushed (see my notes in the review). Let me know if you have any other feedback. Thanks!

@mkrautz

This comment has been minimized.

Copy link
Member

commented May 13, 2017

I hope to do a next pass today or tomorrow.

@mkrautz mkrautz merged commit 5c9a46e into mumble-voip:master May 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mkrautz

This comment has been minimized.

Copy link
Member

commented May 14, 2017

Looks good now, thanks! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.