Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux: Prepare for Flatpak #416

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TheEvilSkeleton
Copy link

@TheEvilSkeleton TheEvilSkeleton commented May 13, 2023

  • Use app ID
  • Add appstream file

Related to flathub/flathub#4151 and #141

jacksongoode

This comment was marked as outdated.

</screenshots>
<content_rating type="oars-1.1"/>
<releases>
<release version="0.1.0" date="2023-05-13"/>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to update the date once it's released

@jacksongoode
Copy link
Collaborator

@TheEvilSkeleton What else is there to do to finalize the process, just add the mpris privileged?

@TheEvilSkeleton
Copy link
Author

I think so. Although, per flathub/flathub#4151 (comment), they say it'd be better not to add any additional permission and have it done properly. To be frank, I know nothing about MPRIS. @A6GibKm can you help us out?

@A6GibKm
Copy link

A6GibKm commented May 18, 2023

If I am not mistaken

let mut media_controls = MediaControls::new(PlatformConfig {
            dbus_name: APP_ID,
            display_name: "Psst",
            hwnd,
        })?;

is the required change. Note that while this would be ideal, it is not strictly needed and one could add a sandbox hole for org.mpris.MediaPlayer2.Psst (Psst being the current dbus_name).

@jacksongoode
Copy link
Collaborator

@TheEvilSkeleton Do you think this change is reasonable?

@TheEvilSkeleton
Copy link
Author

TheEvilSkeleton commented May 25, 2023

In my opinion, yes. I'm in favor of reducing permissions as much as possible. That being said, I'm not well versed in Rust, so I can't give my opinion on the code/implementation.


<description>
<p>
Fast Spotify client with native GUI, without Electron, built in Rust. Very early in development, lacking in features, stability, and general user experience. It is being tested only on Mac so far, but aims for full Windows and Linux support. Contributions welcome!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "It is being tested only on Mac so far, but aims for full Windows and Linux support." sentence should be removed since Flatpak is only on Linux anyway.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is unlerated to flatpak, it is the app's metainfo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's ok. We've since updated that sentence in the readme so it should be updated here anyways.

@jacksongoode
Copy link
Collaborator

@TheEvilSkeleton Has the been any movement on this since the Flatpak folks have talked?

@TheEvilSkeleton
Copy link
Author

Unfortunately, no. I don't have much free time anymore.

@orowith2os
Copy link

I will see if I can find some time to poke at this later.

@jacksongoode
Copy link
Collaborator

Yeah it would be good to finally release this thing officially.

@orowith2os
Copy link

Most of the changes for this seem to not be flatpak-specific; maybe merging those separately would be good?

@jacksongoode
Copy link
Collaborator

@orowith2os @TheEvilSkeleton Any further interest in seeing if we can get this packaged?

@TheEvilSkeleton
Copy link
Author

I can only help with packaging-related stuff, but nothing I can help with Rust code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants