-
Notifications
You must be signed in to change notification settings - Fork 260
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
WIP: Optional media player auto-resume, central management of device call-state responses #1318
base: main
Are you sure you want to change the base?
Conversation
@@ -182,6 +182,8 @@ | |||
<key name="talking-pause" type="b"> | |||
<default>true</default> | |||
</key> | |||
<key name="after-unpause" type="b"> |
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.
I don't love the name here, but I tried to follow the "state-action" format of the other names, like "ringing-pause" and "talking-volume". (Maybe "end-unpause" would be a better choice?) Using "unpause" instead of "resume" was definitely a deliberate choice, to more directly indicate what exactly this controls.
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.
I think "call ended" is probably the most recognizable term. "Call Ended" doesn't really flow from the other titles, but hey.
For the GSettings key I'd probably for ended-pause
. Even though that's sort of inverted, the key itself isn't user-visible and flows from talking-pause
, etc. I expect someone will ask for something to the effect of ended-volume
eventually.
That's my two cents :)
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.
We've already had a request (#1316 (comment)) for ended-delay
, which I quickly erected a Somebody Else's Problem Field around.
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.
I think "call ended" is probably the most recognizable term. "Call Ended" doesn't really flow from the other titles, but hey.
So, "Call Ended" for the UI string (the title of the new one-item list that is sure to grow longer eventually), ened-pause
for the key? Works for me!
Hmmm. Tho... so, ended-pause
should be false
by default, then? And the preference would flip it to true
, if it should not auto-unpause? (Ow, negatives! 🤯 ) Man, that really is kind of confusing. But, that's what comments are for, I guess!
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.
And the preference would flip it to true, if it should not auto-unpause?
That would also make the key value be the inverse of the UI control. Ouch.
Maybe ended-resume
really is a better idea.
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.
ended-resume
is fine, I was just thinking foobar-pause
would refer to the paused state rather than "Pause" the action. I'm not into bikeshedding that particular one though :)
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.
@andyholmes *nod* ended-pause
would still have the same problem, though, whether it's verb or noun. Normally we don't want to leave the players in a paused state after the call, so ended-pause
would default to false
. But then the UI switch still ends up being inverted, unless you word it something convoluted like, "Pause state continues after call end", or "Keep players paused at end of call".
But whenever I try to come up with a normal-person-speak explanation for what the switch in preferences does, it always contains some variation on "resume" and would be enabled by default. Which means its true
/false
states are the inverse of the most-obvious meaning of ended-pause
.
It's fine. ended-resume
seems perfectly workable, no reason not to just go with that.
debug(`Unpaused ${name}`); | ||
} else { | ||
debug(`Cannot unpause ${name}, skipping`); | ||
} |
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.
Oops. Hadn't intended to leave these in, but maybe that's not the worst thing.
* TODO: This potentially worsens the multi-device | ||
* problem, since we may be erasing the player | ||
* list when another device still needs it. |
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.
As the comment notes, this potentially exacerbates an existing problem that can come up when multiple devices are paired to the local machine. Solving that is beyond the scope of this change.
(The problem is that local player state is global to the extension (or should be), but the actions to manage that state are implemented in the per-device plugins. We may need to take a page from GNOME's Freedesktop's (I'm pretty sure) power-management, which allows applications to request and clear "inhibit" flags that will prevent various transitions, without providing them any direct control over the powersave state or functionality.
So, instead of the current logic here, something like:
- A device has an active incoming/outgoing call, for which it is configured to pause media
- It informs the MPRIS backend of this fact
- The backend pauses the media players and stores the device away as the reason for pausing
- A second device starts a call, and informs the backend
- The backend adds the second device's request to the list of reasons why playback is paused
- When either device ends its call, it informs the backend which removes that device from the list
- When all devices have cleared their requests, regardless of the order in which that happens, the list of reasons why playback can't continue will have become empty.
- The backend therefore resumes playback. (Assuming the user hasn't disabled that, via the new preference from this PR — which should probably be a global setting rather than a per-device one, if we move to an inhibit-based model).
Managing audio volumes is probably an even trickier juggling act, since the states there aren't as simple as "paused" and "not paused" and multiple devices' requests will have more complex interactions.
Also, open question: If a device disconnects from GSConnect, is its request deleted immediately? Deleted after a grace period (to see if it reconnects)? Deleted only if there are no other requests? Deleted only when there are other requests?
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 sounds pretty reasonable.
As for the last question I don't really know if there's a good answer, but the best answer would probably be to err on the side of leaving the media in its current state (ie. paused). Likewise, if the playback is changed externally that should probably clear any pending "inhibitors" and assume the user has taken the steering wheel.
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.
@andyholmes That makes sense. So, if a device drops off unexpectedly, it still gets removed from the inhibit list immediately. (Because we have no idea what its call state is anymore, even if it comes back.) Buuuut, playback doesn't resume even if it's the last device on the list. At which point, since the list is empty, the MPRIS backend still throws away its stored player state and washes its hands of the whole thing, leaving it to the user to do as they please. (Basically, returning to its default state of not managing any player controls, where it will stay until a new call event comes in and prompts it to start the pause-and-monitor cycle over again.)
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.
We could post a notification to the Shell, too, when a device drops off unexpectedly. (Only in the specific case that it had an active call in progress and its inhibits were dropped. Otherwise, we're just pestering the user with irrelevancies.)
That way, they'll know why the auto resume "didn't work". (And hopefully not generate any new bug reports.)
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.
Hmm, I'd lean towards no notification myself, but I see the argument for it. I'll leave that to you :)
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.
@andyholmes So, to monitor devices with active pause tokens for unexpected disconnects from the MPRIS backend code, is a Gio.bus_watch_name()
on each device's interface my best option? Or are there convenience functions somewhere I can use to ask, "This ID still connected?"
...Hrm, actually, that wouldn't work, I'd need to know if it'd disconnected and reconnected at any point as well. bus_watch_name()
it is!
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.
Oh, wait, now I see it. The telephony plugin's .disconnected()
will be called (well all of the plugins' .disconnected()
s will be called) if the device drops off, I can have it actively inform the backend that it's unexpectedly going bye-bye.
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.
You could also use the pattern from the component code, with a acquire()
/release()
pair that returns some no-op object with a "refCount" property.
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.
@andyholmes I ended up concluding that explicit ref/token counting didn't really buy me anything. I wanted to keep a central list (a Set
, actually) of all devices with an active request, so its .size
at any moment represents the count. That way:
- Any duplicate requests that may come through (for whatever reason) can just be processed normally and require zero special handling, since they won't affect the
.size
of theSet
the way a new, valid request would. - The device plugin instances don't have to hold onto any state at all, for their pause requests — they can statelessly fire off updates to the backend whenever a call event occurs that has some pause-affecting action enabled. The backend tracks the state of device requests right alongside the pause state it's already tracking for the media players.
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.
Sounds good, I await your commit so I can have a good look :)
d6dc2a6
to
fa8f2ad
Compare
I think that's okay, in fact it's reassuring that this is the case when something goes wrong. I think the worst case scenario is someone's music suddenly blasting if something goes sideways.
Agreed, you have to really try to shoot yourself in the foot with this.
I think that's okay, too. It's good to have some logic here for these cases, but the likelihood someone will actually have to phones receiving calls at the same time is pretty unlikely. With a bit of a framework in place this can always be tweaked later, as has been the case with MPRIS handling in general. |
Add a new Telephony preference, 'after-unpause', default true. (New UI in the preferences allows it to be switched on/off.) When it's set to false, exiting the ringing and talking states will still restore volume levels (if configured), but will leave the media player(s) paused so the user can resume manually.
4f1e0d7
to
86e4f50
Compare
Was this one good to go? |
Add a new Telephony preference, 'after-unpause', default true.
(New UI in the preferences allows it to be switched on/off.)
When it's set to false, exiting the ringing and talking states
will still restore volume levels (if configured), but will leave
the media player(s) paused so the user can resume manually.
Fixes: #1316
The added preference UI: