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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

WIP: Optional media player auto-resume, central management of device call-state responses #1318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03914ea
Make auto-resume of paused media optional
ferdnyc Mar 29, 2022
86e4f50
Rename unpause pref to 'ended-resume'
ferdnyc Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion data/org.gnome.Shell.Extensions.GSConnect.gschema.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ SPDX-License-Identifier: GPL-2.0-or-later
<key name="talking-pause" type="b">
<default>true</default>
</key>
<key name="ended-resume" type="b">
<default>true</default>
</key>
</schema>
</schemalist>

96 changes: 96 additions & 0 deletions data/ui/preferences-device-panel.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,7 @@ SPDX-License-Identifier: GPL-2.0-or-later
<property name="visible">True</property>
<property name="can_focus">False</property>
<property name="label_xalign">0</property>
<property name="margin_bottom">32</property>
<child>
<object class="GtkListBox" id="talking-list">
<property name="visible">True</property>
Expand Down Expand Up @@ -1764,12 +1765,107 @@ SPDX-License-Identifier: GPL-2.0-or-later
</object>
</child>
</object>
<packing>
<property name="expand">False</property>
<property name="fill">True</property>
<property name="position">4</property>
</packing>
</child>
<child>
<object class="GtkLabel" id="ended-list-label">
<property name="visible">True</property>
<property name="can-focus">False</property>
<property name="halign">start</property>
<property name="margin-bottom">12</property>
<property name="label" translatable="yes">Call Ended</property>
<attributes>
<attribute name="weight" value="bold"/>
</attributes>
</object>
<packing>
<property name="expand">False</property>
<property name="fill">True</property>
<property name="position">5</property>
</packing>
</child>
<child>
<object class="GtkFrame">
<property name="visible">True</property>
<property name="can-focus">False</property>
<property name="label-xalign">0</property>
<child>
<object class="GtkListBox" id="ended-list">
<property name="visible">True</property>
<property name="can-focus">False</property>
<property name="hexpand">True</property>
<property name="selection-mode">none</property>
<signal name="keynav-failed" handler="_onKeynavFailed" swapped="no"/>
<signal name="row-activated" handler="_onToggleRowActivated" swapped="no"/>
<child>
<object class="GtkListBoxRow" id="ended-resume-row">
<property name="visible">True</property>
<property name="can-focus">True</property>
<property name="height-request">56</property>
<property name="selectable">False</property>
<child>
<object class="GtkGrid">
<property name="visible">True</property>
<property name="can-focus">False</property>
<property name="margin-start">12</property>
<property name="margin-end">12</property>
<property name="column-spacing">12</property>
<child>
<object class="GtkLabel" id="ended-resume-label">
<property name="visible">True</property>
<property name="can-focus">False</property>
<property name="halign">start</property>
<property name="valign">center</property>
<property name="hexpand">True</property>
<property name="vexpand">True</property>
<property name="label" translatable="yes">Resume Paused Media</property>
<accessibility>
<relation type="label-for" target="ended-resume-row"/>
<relation type="label-for" target="ended-resume"/>
</accessibility>
</object>
<packing>
<property name="left-attach">0</property>
<property name="top-attach">0</property>
</packing>
</child>
<child>
<object class="GtkSwitch" id="ended-resume">
<property name="visible">True</property>
<property name="can-focus">True</property>
<property name="valign">center</property>
<property name="action-name">settings.ended-resume</property>
<accessibility>
<relation type="controlled-by" target="ended-resume-row"/>
<relation type="labelled-by" target="ended-resume-label"/>
</accessibility>
</object>
<packing>
<property name="left-attach">1</property>
<property name="top-attach">0</property>
</packing>
</child>
</object>
</child>

</object>
</child>
</object>
</child>
<child type="label_item">
<placeholder/>
</child>
</object>
<packing>
<property name="expand">False</property>
<property name="fill">True</property>
<property name="position">7</property>
</packing>
</child>
</object>
</child>
</object>
Expand Down
6 changes: 6 additions & 0 deletions src/preferences/device.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ var Panel = GObject.registerClass({
// Telephony
'telephony', 'telephony-page',
'ringing-list', 'ringing-volume', 'talking-list', 'talking-volume',
'ended-list',

// Shortcuts
'shortcuts-page',
Expand Down Expand Up @@ -484,6 +485,7 @@ var Panel = GObject.registerClass({
this.actions.add_action(settings.create_action('talking-volume'));
this.actions.add_action(settings.create_action('talking-pause'));
this.actions.add_action(settings.create_action('talking-microphone'));
this.actions.add_action(settings.create_action('ended-resume'));

// Pair Actions
const encryption_info = new Gio.SimpleAction({name: 'encryption-info'});
Expand Down Expand Up @@ -903,8 +905,12 @@ var Panel = GObject.registerClass({
this.ringing_list.next = this.talking_list;
this.talking_list.prev = this.ringing_list;

this.talking_list.next = this.ended_list;
this.ended_list.prev = this.talking_list;

this.ringing_list.set_header_func(rowSeparators);
this.talking_list.set_header_func(rowSeparators);
this.ended_list.set_header_func(rowSeparators);
}

/**
Expand Down
13 changes: 11 additions & 2 deletions src/service/components/mpris.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,7 @@ var Manager = GObject.registerClass({
if (player.PlaybackStatus === 'Playing' && player.CanPause) {
player.Pause();
this._paused.set(name, player);
debug(`Paused ${name}`);
}
}
}
Expand All @@ -1004,14 +1005,22 @@ var Manager = GObject.registerClass({
* A convenience function for restarting all players paused with pauseAll().
*/
unpauseAll() {
for (const player of this._paused.values()) {
if (player.PlaybackStatus === 'Paused' && player.CanPlay)
for (const [name, player] of this._paused) {
if (player.PlaybackStatus === 'Paused' && player.CanPlay) {
player.Play();
debug(`Unpaused ${name}`);
} else {
debug(`Cannot unpause ${name}, skipping`);
}
Comment on lines +1011 to +1014
Copy link
Member Author

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.

}

this._paused.clear();
}

clearPaused() {
this._paused.clear();
}

destroy() {
if (this._cancellable.is_cancelled())
return;
Expand Down
24 changes: 20 additions & 4 deletions src/service/plugins/telephony.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,32 @@ var Plugin = GObject.registerClass({
}

/**
* Restore volume, microphone and media player state (if changed), making
* sure to unpause before raising volume.
* Restore volume and microphone, plus media player state
* (if we changed it and restore is enabled).
*
* If we're going to auto-resume, make sure to do that first
* before raising volume.
* Otherwise, still raise the volume (so the user can unpause).
*
* TODO: there's a possibility we might revert a media/mixer state set for
* another device.
*/
_restoreMediaState() {
// Media Playback
if (this._mpris)
this._mpris.unpauseAll();
if (this._mpris) {
if (this.settings.get_boolean('ended-resume')) {
this._mpris.unpauseAll();
} else {
/* Even if resume is disabled, we still need to clear
* the list of which players we're holding paused
*
* TODO: This potentially worsens the multi-device
* problem, since we may be erasing the player
* list when another device still needs it.
Comment on lines +118 to +120
Copy link
Member Author

@ferdnyc ferdnyc Mar 30, 2022 •
edited
Loading

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:

  1. A device has an active incoming/outgoing call, for which it is configured to pause media
  2. It informs the MPRIS backend of this fact
  3. The backend pauses the media players and stores the device away as the reason for pausing
  4. A second device starts a call, and informs the backend
  5. The backend adds the second device's request to the list of reasons why playback is paused
  6. When either device ends its call, it informs the backend which removes that device from the list
  7. 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.
  8. 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?

Copy link
Collaborator

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.

Copy link
Member Author

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.)

Copy link
Member Author

@ferdnyc ferdnyc Apr 2, 2022 •
edited
Loading

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.)

Copy link
Collaborator

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 :)

Copy link
Member Author

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!

Copy link
Member Author

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.

Copy link
Collaborator

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.

Copy link
Member Author

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:

  1. 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 the Set the way a new, valid request would.
  2. 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.

Copy link
Collaborator

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 :)

*/
this._mpris.clearPaused();
}
}

// Mixer Volume
if (this._mixer)
Expand Down