Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

Changed the mpris slot name to "brave" #21

Merged
merged 1 commit into from
May 3, 2023

Conversation

wknapik
Copy link

@wknapik wknapik commented Feb 21, 2023

This is a follow-up to #19, prompted by a request from the Snap store folks

@wknapik wknapik self-assigned this Feb 21, 2023
@wknapik wknapik requested a review from a team as a code owner February 21, 2023 14:38
@wknapik
Copy link
Author

wknapik commented Feb 21, 2023

@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?

@jayschwa
Copy link

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.

@wknapik
Copy link
Author

wknapik commented Feb 21, 2023

@jayschwa thanks for the quick response.

Our previous snap release got blocked with the message

Hello, is it possible to change the mpris name from "chromium" to "brave"? Thanks!

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.

@wknapik wknapik requested a review from fmarier February 21, 2023 15:38
@fmarier
Copy link
Member

fmarier commented Feb 21, 2023

Do we need this or we just need to revert #19 like @jayschwa said?

@jayschwa
Copy link

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.)

@wknapik
Copy link
Author

wknapik commented Mar 1, 2023

cc @oajara

@wknapik
Copy link
Author

wknapik commented Mar 1, 2023

cc @pfsmorigo

@wknapik
Copy link
Author

wknapik commented Mar 1, 2023

@oajara
Copy link

oajara commented Mar 2, 2023

The current mpris name was approved by Canonical.

“Granted use of mpris name chromium - this is common for browsers based on the upstream chromium project and has been granted previously for other similar snaps.”

I think we don't need this PR anymore.

@wknapik
Copy link
Author

wknapik commented Mar 2, 2023

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)

@wknapik
Copy link
Author

wknapik commented Mar 10, 2023

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.

@wknapik
Copy link
Author

wknapik commented May 1, 2023

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.

@wknapik wknapik merged commit b89a28c into stable May 3, 2023
@wknapik wknapik deleted the wknapik-mpris-slot-name-brave-instead-of-chromium branch May 3, 2023 20:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants