Notify headphone being plugged in on some hardware
AbandonedPublic

Authored by thsurrel on Oct 9 2018, 4:33 PM.

Details

Reviewers
drosca
ngraham
Group Reviewers
Plasma
VDG
Summary

On my computer, plugging the headphones in and out does not create
a sink change but only a port change within the audio 'build-in' sink.

I don't know if many users are in the same case, but this patch brings
back the OSD notifier.

I don't know if what I did is the right approach, comments are welcomed.

Test Plan

Buy a Lenovo T470s :)
Plug headphones in and enjoy the nice icon on the OSD.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Branch
arc_ports (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3710
Build 3728: arc lint + arc unit
thsurrel created this revision.Oct 9 2018, 4:33 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 9 2018, 4:33 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
thsurrel requested review of this revision.Oct 9 2018, 4:33 PM
broulik added a subscriber: broulik.Oct 9 2018, 8:27 PM
broulik added inline comments.
applet/contents/ui/main.qml
173

Please cache defaultSink.ports[defaultSink.activePortIndex] in a variable.

Also, can this be out of bounds or otherwise throw an error?

175

I would prefer if hardcoded values like these are put into a helper function or formFactorIcon augmented to take those into account also

181

if (!icon) { is sufficient

188

The text = assignments seem to serve no purpose, ie. the end result would be no different than having it down where it was

src/pulseaudio.cpp
264

You can connect a signal to a signal:

connect(sink, &Sink::activePortIndexChanged, this, &Sink::defaultSinkChanged);
thsurrel updated this revision to Diff 43271.Oct 10 2018, 9:35 AM

Fixes as per broulik comments.
Thanks for the review!

thsurrel marked 5 inline comments as done.Nov 6 2018, 10:03 PM

Ping !
Anybody else managed to test this ?

Seems to work well here. But I'll leave it to @drosca or @nicolasfella to decide. Sorry for the long wait time :/

Does this have the potential to inappropriately show a headphones OSD when you plug in speakers, or vice versa?

I think it can tell the difference between regular speaker jacks on the back of a desktop computer and the headphone port on a laptop. Not sure, though, might be a good idea to test that.

But you can plug speakers into the headphone jack on a laptop. They can use the same 1/16 minijack as actual headphones.

I should be able to test that tonight.

But what happens today on a computer that does a _sink change_ (and not a port change like on my computer) when plugging in speakers in a laptop jack connector ? Can it tell it is not headphones ? Anybody could try that ?

So, it seems that pulseaudio can not tell the difference what we are plugging in the jack connector on a laptop, but I guess it must be the same behavior with computers that get a sink change when switching from internal speaker to external ?

drosca added a comment.Nov 8 2018, 9:07 AM

So, it seems that pulseaudio can not tell the difference what we are plugging in the jack connector on a laptop, but I guess it must be the same behavior with computers that get a sink change when switching from internal speaker to external ?

What do you mean with "sink change"? Do you mean that plugging in something in jack will trigger adding new sink (device) to Pulseaudio? That will only be the case for USB headphones, but for jacks it will always just switch port.

What do you mean with "sink change"? Do you mean that plugging in something in jack will trigger adding new sink (device) to Pulseaudio? That will only be the case for USB headphones, but for jacks it will always just switch port.

Ok, thanks for the explanation, I did not know that. I assumed that depending on the hardware you could either receive a sink change or a port change.

So now the question is whether it is acceptable to show a headphone on the OSD when we could have been plugging something else in the jack.

drosca added a comment.Nov 8 2018, 9:27 AM

So now the question is whether it is acceptable to show a headphone on the OSD when we could have been plugging something else in the jack.

It will already show in device list as "headphones" regardless of what is actually plugged in, so it is fine by me.
VDG should give the final ack though.

broulik added inline comments.Nov 9 2018, 8:39 PM
applet/contents/ui/main.qml
179

What I don't like about this is that it no longer reflects the actual device volume, e.g. when I unpair my headphones, I get a "Speakers" notification with full volume icon despite the speakers actually being muted.

The speaker icon could maybe get a newer version that would be different than the one used to show the volume level ?
It does also look a bit unpolished compared to the headphone one.

So now the question is whether it is acceptable to show a headphone on the OSD when we could have been plugging something else in the jack.

I don't think so, sorry. I think this would be very confusing for people who plug speakers into their laptops. The bug is not very noticeable as long as the wrong icon is only displayed in plasma-pa itself, but with this patch, the wrong icon will be shoved in the user's face every time they plug in something that's not headphones.

Maybe we could use a generic icon, or make a new one that depicts speakers plus headphones or something?

Maybe we could use a generic icon, or make a new one that depicts speakers plus headphones or something?

It can be any device really, not just speakers/headphones, as long as it has jack. If this icon has to reflect the reality, then it should be (un)plugged jack.

Now that I think about it, maybe we keep it the way it is and see if anyone actually complains rather than trying to make the perfect the enemy of the good. I don't think I've actually heard any complaints from the jack always displaying a headphone icon in the applet itself.

That said, now I'm wondering whether or not showing an OSD for this is really all that useful. I personally don't like pop-ups and notifications that essentially say, "Hey, you just did a thing!" It's like, duh, I know I did a thing, you don't need to tell me because I just did it! Those kinds of notifications are just noise.

Thoughts?

Having a visual feedback would be useful when pairing bluetooth speakers IMO (does it ? I don't have any to test with).

For a jack connection, that is indeed less useful, but that would be consistent. In addition, the plasma-pa has an option to turn the OSD off, right now having it checked or not does not change anything when plugging headphones in a laptop.

Well, there are types of visual feedback that are useful and there are types that aren't. A "Bluetooth device successfully paired" notification is useful because there was always some chance that it would fail, and you need to know whether or not that happened. I could see a "headphones/speakers plugged in" notification being theoretically useful because it communicates that all running streams will be redirected to the new audio output... but isn't that what the user was expecting to happen anyway? Is there any value to notifying the user that what they expected to happen happened when they performed an action that was 100% guaranteed to succeed? This may be just me, but I find such feedback to be really annoying. People likewise complain about the "you trashed this item!" notification in Dolphin. They think, "duh I trashed it, you don't need to tell me that it worked!"

Absolutely, as said in my previous comment, I agree the usefulness of the OSD in that case is very much questionable.
I was raising questions on a different level:

  • would it be import to be consistent and show an OSD in the jack case if we are showing one in the bluetooth case ?
  • the plasma-pa option is hard to understand if a user will not see any difference in behavior between checked and not checked.

That being said, I lived happily with no OSD notification and would have turned it off had this patch made its way in.
I posted this patch in the first place only because _I assumed_ this was working for other laptops. I thought I was fixing a corner case, not changing a de facto behavior.

Absolutely, as said in my previous comment, I agree the usefulness of the OSD in that case is very much questionable.
I was raising questions on a different level:

  • would it be import to be consistent and show an OSD in the jack case if we are showing one in the bluetooth case ?

IMHO no. The bluetooth OSD is useful, while the jack OSD is annoying noise.

  • the plasma-pa option is hard to understand if a user will not see any difference in behavior between checked and not checked.

That's true. Perhaps we could re-word it to better describe under what circumstances it will show an OSD.

That being said, I lived happily with no OSD notification and would have turned it off had this patch made its way in.
I posted this patch in the first place only because _I assumed_ this was working for other laptops. I thought I was fixing a corner case, not changing a de facto behavior.

Yeah, it seems that this was not a bug but rather a feature. :)

ngraham requested changes to this revision.Nov 14 2018, 6:27 PM
This revision now requires changes to proceed.Nov 14 2018, 6:27 PM

Should I do anything special about this Diff ? Do we just keep it as it is for reference ?

My recommendation would be to abandon the diff or change it to address the problem of the OSD checkbox not being clear about what it affects.

thsurrel abandoned this revision.Nov 14 2018, 9:46 PM