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

Fix reconnecting to PulseAudio, reattaching to devices #195

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bluesabre
Copy link
Member

@bluesabre bluesabre commented Apr 11, 2021

After some trial and error and logging, I identified some issues with reloading the indicator.

  • PulseAudio would become disconnected, and enter an infinite connect/disconnect loop.
  • Devices previously listed would no longer be displayed since the reconnecting PA service would see an updated instead of a new device.

This PR fixes this by:

  1. On disconnect from PulseAudio, a disconnected signal is sent and the indicator's device list is cleared.
  2. On successful reconnect to PulseAudio, stop attempting to reconnect by removing any outstanding reconnect source.
  3. When PulseAudio reconnects, it processes the sources and sinks again. If a known device is processed, an updated signal is now sent. This updated device is compared to the current device list, and if not found, added.
  4. Finally, since the device lists are being repopulated, the default device has to be explicitly set when added to update the radio group.

I think this is a fairly clean way to address the issue, but please let me know if you'd like me to make any changes!

Update 4/29

  • There were several objects that were not successfully being destroyed. This led to some of the issues I observed. Everything now cleans up pretty reliably.
  • PulseAudioManager now correctly handles the terminated state, stopping attempts to reconnect and cleaning up known devices on disconnect. This allow allows it to be fully destroyed.

Fixes #194

@bluesabre bluesabre requested a review from a team April 11, 2021 00:21
src/Widgets/DeviceManagerWidget.vala Outdated Show resolved Hide resolved
src/Widgets/DeviceManagerWidget.vala Outdated Show resolved Hide resolved
@bluesabre bluesabre requested a review from jeremypw April 30, 2021 00:07
@jeremypw
Copy link
Collaborator

jeremypw commented May 1, 2021

So this PR does reconnect the devices if you reinstall from source from the commandline without restarting wingpanel. However, this only works if the indicator is already installed. If you sudo apt purge the indicator and then install from this source, then the devices are not reconnected until wingpanel is restarted.

If you are developing a wingpanel indicator you should, in my opinion, always restart wingpanel after installing revised code otherwise there is no guarantee that the new code is actually running (it probably isn't).

I havent been able to test what happens if a new version of the indicator is installed by an update as there is only one version in the repository - it is possible that the devices are disconnected and so this PR would still be useful.

I'll see if I can simulate such an upgrade somehow.

@jeremypw
Copy link
Collaborator

jeremypw commented May 1, 2021

OK, I uninstalled the indicator with apt. Then I installed the master version from source with a fake version number of 2.1.7. I then installed version 2.1.8 with apt. The devices were not disconnected (and wingpanel did not flash suggesting it had not noticed the upgrade).

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

See previous comments. Are you able to demonstrate any other issue that this PR would solve? Or get it to connect all the devices if installed after purging?

@bluesabre
Copy link
Member Author

See previous comments. Are you able to demonstrate any other issue that this PR would solve? Or get it to connect all the devices if installed after purging?

@jeremypw Thanks for the review, and sorry for the delayed followup. I think the only issues this PR solves are ensuring the device list stays available and the infinite dis-/re-connect to PulseAudio. The first is just a minor usability inconvenience, and the second, while a bit spammy on the logs, should likewise have little impact.

I've so far not had any success with reconnecting devices after purging, so not really sure how to proceed with that particular fix. Since the issues being addresses are really low impact, I'm content with closing this out, but can continue investigating fixes for the purge-related issue.

@jeremypw jeremypw marked this pull request as draft June 21, 2021 08:29
@jeremypw
Copy link
Collaborator

@bluesabre Thanks for continuing to look into this. I'll convert this to draft while you are looking into a fix for install after purge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device Manager: Connected devices are cleared when the sound indicator is updated
4 participants