GNOME Bugzilla – Bug 690539
volume: Clean up the volume indicator code
Last modified: 2012-12-25 17:34:48 UTC
The current code here is "JavaScript"-y, where we have a fake namespace done by a prefix. I've never really liked this model; so I'm cleaning it up a bit. I'm not particularly happy with the large cleanup at the end, but I think it can be workshopped towards something good. Looking at all the crazy mute manipulations, I wonder if it makes sense to get some smarter semantics baked into gvc so that muting is handled for us automatically. We could probably also add a "normalized-volume" property so we wouldn't have to multiply/divide all over the place.
Created attachment 231961 [details] [review] volume: Make icon calculation stable Calculate an icon based on our current state, not a mess of signal emissions and callbacks. This is a preliminary basic cleanup patch in preparation for the next one.
Created attachment 231962 [details] [review] volume: Merge the two update handlers With our mess of callbacks gone, we can update stream volume in a single place only.
Created attachment 231963 [details] [review] volume: Don't set the visibility of the main icon As the main icon is inside the actor, this is needless calculation.
Created attachment 231964 [details] [review] volume: Don't have a separate syncVisibility method With it doing less now, we can simply do it inline.
Created attachment 231965 [details] [review] volume: Pick up hasHeadphones from the signal handler A preliminary patch for big cleanups happening soon.
Created attachment 231966 [details] [review] volume: Clean up code Rather than using naming schemes and dynamic property lookups as a kind of namespace, use what was designed to be used as a namespace: a class.
Review of attachment 231963 [details] [review]: Wrong, mainIcon.visible is bound to the volume icon in the lock screen menu.
Review of attachment 231962 [details] [review]: Uhm, ok.
Review of attachment 231961 [details] [review]: Not bad.
Review of attachment 231964 [details] [review]: Wrong, because of the previous patch
Review of attachment 231965 [details] [review]: Ok
With the other bug sorted out, the two rejected patches become acn. The remaining one seems fine, but let me test it first.
Review of attachment 231966 [details] [review]: Regression found: _notifyVolumeChange should notify only for the output stream.
Attachment 231961 [details] pushed as f60fb95 - volume: Make icon calculation stable Attachment 231962 [details] pushed as 571aaec - volume: Merge the two update handlers Attachment 231963 [details] pushed as 9ebeb64 - volume: Don't set the visibility of the main icon Attachment 231964 [details] pushed as 66da3f5 - volume: Don't have a separate syncVisibility method Attachment 231965 [details] pushed as 1a4948f - volume: Pick up hasHeadphones from the signal handler That actually isn't a regression -- the logic to play a canberra sound event on the input slider drag-end is in the original code, too. I'll file a second bug for a follow-up fix. Pushed after IRC review.
Anything left here? Or can we close this?
(Merry Christmas!) Let's close this.