feat(media): Event-based updating #16
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces the rough base for event-based updates to the media widget, instead of timer-based. Multiple users have experienced high cpu uasge, NPSMSvc crashes, and other odd behaviour. This was likely caused by the bar spamming the service with update requests. Especially when multiple bars were present (multiple monitors), the amount of requests was large since each widget would do its own request. The current implementation subscribes update functions to the `winsdk` events, and updates from there. Known bugs are that the widget does not hide upon no media, and that the widget does not immediately detect media on launch (a song switch is needed to trigger the first update).
Introduced an event listener for the session change to set the hide/show properly, instead of relying on the media_info change like before. Known bug: I also identified that the 'C++' error fixed in commit `b08c77a` has resurfaced, due to us making our own event loop, for which the tasks are not killed. It would be better if the bar would call a 'on_close' method for all widgets, and we could put this kind of graceful stopping in there.
Fix this issue (again), now by making sure we clean up the state of the WindowsMedia handling nicely, specifically clearing the callbacks.
Fixed forced updates, which allows detecting already playing media on startup, and fixes the toggle-label behaviour.
Fixed some issues which could freeze the bar because multiple threads were trying to run the async media update. Moved the lock to lock out the full async call to prevent this. Also added thumbnail error handling. This prevents thumbnail cropping errors from crashing the bar, which would occur in some media players which put '-' as title/artist on switch.
I do not yet want to remove the verbose log calls, but I did want to rename them so it is clearer that they originate from the media widget.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR provides a total rewrite for much of the backend of the media widget, now using event-based instead of timer-base updates. Multiple users have experienced high cpu uasge, NPSMSvc crashes, and other odd behaviour. This was likely caused by the bar spamming the service with update requests. Especially when multiple bars were present (multiple monitors), the amount of requests was large since each widget would do its own requests for updating.
The current implementation subscribes update functions to the
winsdkevents, and updates from there. The wrapper forwinsdkmedia handling is a singleton to which all media widgets subscribe, meaning that it only makes one windows api call(back) no matter how many widgets.Remaining known issues: Updates slow on Firefox, which seems to be Firefox not firing update events at immediately (like e.g. Spotify or Arc do).