-
Notifications
You must be signed in to change notification settings - Fork 11
Changed the mpris slot name to "brave" #21
Changed the mpris slot name to "brave" #21
Conversation
@jayschwa any comments regarding this change? Have you tested with the "brave" name as set in this PR? Also, should this PR also close brave/brave-browser#22617? |
What was the communication from the Snap store? Human or automated QA rule? I did not test with value "brave", but I believe this change will be functionally equivalent to reverting PR #19, so you might just do that instead. Since the AppArmor rule will continue to not match the media player name in DBus, the issues regarding GNOME media controls should remain open. I guess the fix will have to be in the browser source code instead to rename the DBus name if it can't be applied here. |
@jayschwa thanks for the quick response. Our previous snap release got blocked with the message
The next one was not blocked. At the moment we have something that works, so we could just keep it, but I'd expect the snap store reviewers to eventually try and force it again. To make the brave-core change, we'd need help from someone familiar with code on that side. |
Before changing anything, I'd suggest tagging the person from the Snap Store into this discussion. They may not have the context to understand why the mpris slot name was changed. Next, maybe they can explain why it would be bad to have the mpris slot name configured as "chromium". I personally have no problems running the Brave and Chromium snaps side-by-side, but I'm also not an expert in Snap. If it's bad for the Brave snap to use "chromium" for the mpris slot name, then #19 should be reverted, and the fix will have to be made in the browser itself. (I lack the knowledge to do that.) |
cc @oajara |
cc @pfsmorigo |
The current mpris name was approved by Canonical.
I think we don't need this PR anymore. |
If @fmarier's one-line PR does the trick, we could ahead with it (and this one) |
I see brave/brave-core#17440 went in, but is only on master for now. Once it reaches the release channel, we'll merge this and release a new snap. |
Per Kamil, the next stable release will most likely be off 1.51.x (unless there's another c112 update), which already has Francois's patch. Let's merge this once we're sure there will be no more releases off 1.50.x, which should be this week. |
This is a follow-up to #19, prompted by a request from the Snap store folks