Add Event Sounds stream to Applications list
ClosedPublic

Authored by drosca on Apr 9 2016, 11:53 AM.

Details

Summary

It is now possible to change volume of notification sounds.

BUG: 355470
FIXED-IN: 5.7.0

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
drosca updated this revision to Diff 3234.Apr 9 2016, 11:53 AM
drosca retitled this revision from to Add Event Sounds stream to Applications list.
drosca updated this object.
drosca edited the test plan for this revision. (Show Details)
drosca added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptApr 9 2016, 11:53 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
drosca added a comment.Apr 9 2016, 3:03 PM

Looks like this (always first in the list). It will need different icon, ideas?

In D1366#25480, @drosca wrote:

Looks like this (always first in the list). It will need different icon, ideas?

Event sounds are usually notifications, right? If so, I'd just use the notifications icon (preferences-desktop-notification)

Not sure about the term *event* sounds. What about *notification* sounds?

drosca updated this revision to Diff 3300.Apr 12 2016, 3:56 PM

Rename to "Notification Sounds"

drosca updated this revision to Diff 3308.Apr 13 2016, 6:37 AM

preferences-desktop-notification icon

drosca updated this revision to Diff 3431.Apr 20 2016, 7:00 AM

Store device name for event role stream

broulik added inline comments.May 10 2016, 6:02 PM
src/context.cpp
154

Braces pls

449

Avoid creating a temporary list keys() just to find something in it, use constFind and std::distance

src/eventstream.cpp
29–32 ↗(On Diff #3431)

Initializer list:
: SinkInput(parent)
, m_clientIndex(PA_INVALID_INDEX)
, ...

41 ↗(On Diff #3431)

const

src/kcm/package/contents/ui/StreamListItem.qml
74

I can't move notification sounds to another device? :(

Makes sense, and I agree that "notification sounds" is better.

drosca added inline comments.May 13 2016, 5:33 PM
src/kcm/package/contents/ui/StreamListItem.qml
74

omg i had lengthy answer and phab ate it...

So in short, you can't even do that in pavucontrol. There is an issue with module-stream-restore streams, because its device may be even device that is not currently available (the structure holds the device name, not its index), but in our GUI it only lists available devices.

I'll rewrite this patch to make it less hacky on the C++ side (it now derives from SinkInput, but the underlying pa object differs a lot from the sinkinput object) and try to think of a way to solve that issue.

markg added a subscriber: markg.May 15 2016, 8:17 PM
markg added inline comments.
src/context.cpp
449

You can merge this one and the earlier isNew i think.

Something like:

auto result = m_sinkInputs.data().constFind(eventRoleIndex); 

if (result == m_sinkInputs.data().constEnd()) {
    emit m_sinkInputs.added(... something ...);
} else {
    emit m_sinkInputs.updated(result.value() .. or something like it);
}

You have to change it obviously, but you can make it nicer by using constFind

src/kcm/package/contents/ui/StreamListItem.qml
57–66

Just a preference, feel free to ignore.
Can you try to make get this text value from the C++ side instead of if/elseif/else in QML.. Would be cleaner.

In fact, you might be able to tweak PulseObject.name and and just use that as text.

src/maps.h
74

This isn't a neat way.

You're returning the data in a writeable way so that you can add items later on. I think it would be cleaner and better if you simply add an "insertEntry" function (just like you already have an updateEntry and a removeEntry). Then use the new "insertEntry" where you use this data() method instead.

77–78

Big pros if you rewrite this to a:

auto result = m_data.constFind(t);

if (result != m_data.constEnd()) {
    return result.value();
} else {
    return -1;
}

Or something alike. My example probably doesn't work _exactly_ like that ;)

159

Massive +1 (i know you didn't touch this here.. just commenting on it since i'm reading the code now).

If you transform this into smart pointers you save quite some code within the removeEntry and the reset method. And you prevent accidental "oops, forgot to delete the object" mistakes (aka, memory leaks).

Note: that could also make m_pendingRemovals redundant. But you would have to test that.

drosca updated this revision to Diff 4171.Jun 2 2016, 6:17 PM

Rewrite

This revision was automatically updated to reflect the committed changes.