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

Add macOS support and uninstaller #28

Merged
merged 3 commits into from
Aug 27, 2023
Merged

Conversation

SpaceyKasey
Copy link
Contributor

@SpaceyKasey SpaceyKasey commented Aug 25, 2023

  • Confirmed that python script works on macOS
  • Added support for macOS to the install script
  • Added uninstall script

@SpaceyKasey SpaceyKasey changed the title Add macOS support and installer Add macOS support and uninstaller Aug 25, 2023
@JustTemmie
Copy link
Owner

this looks great, thanks!

@Radiicall - are you able to test and confirm that this works on your macbook?

@JustTemmie JustTemmie added the enhancement New feature or request label Aug 25, 2023
@Radiicall
Copy link
Contributor

this looks great, thanks!

@Radiicall - are you able to test and confirm that this works on your macbook?

I dont have my mac on hand, test it in a VM maybe

systemctl --user daemon-reload
systemctl --user --now enable "steam-presence.service"
# Detect OS and perform OS-specific tasks
if [[ "$OSTYPE" == "linux-gnu"* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never seen this environment variable before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.gnu.org/software/bash/manual/html_node/Bash-Variables.html
It is in the Bash documentation. Works on macOS but I don't have a linux env to test it rn.

installer.sh Outdated
systemctl --user --now enable "steam-presence.service"
elif [[ "$OSTYPE" == "darwin"* ]]; then
echo "Setting up launchd plist file for macOS"
PLIST="$HOME/Library/LaunchAgents/com.steampresence.app.plist"
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming could be a bit better, for example "com" implies commercial, if you want to use that naming scheme then i would suggest "com.github.justtemmie.steam-presence.plist" because it is a more accurate description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switch to com.github.justtemmie.steam-presence.plist. 'com' is the convention, although there appears to not be any hard rule for that. What about changing to 'dev' or something else? They are expecting domain-style naming.

Copy link
Contributor

@Radiicall Radiicall Aug 27, 2023

Choose a reason for hiding this comment

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

Naming it com.github.<author>.<project> is pretty normal in flatpaks if you know anything about those so i think that's fine

installer.sh Outdated
Comment on lines 77 to 78
<key>Label</key>
<string>com.steampresence.app</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Label should be "Steam Presence" or "Steam-Presence"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Copy link
Contributor

Choose a reason for hiding this comment

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

The label string doesnt have to match the file name, you should label it as "Steam Presence" or "Steam-Presence" so its easier to tell at a glance what the login item is for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I updated to Steam Presence

installer.sh Outdated

echo "This script is LINUX ONLY, and it's still in testing phases, if you encounter any bugs please open an issue"
echo "This script is for LINUX and macOS, and it's still in testing phases. If you encounter any bugs, please open an issue."
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change "LINUX" to "linux", it was capitalized because it was very important that you understood that it only supported linux before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@JustTemmie
Copy link
Owner

this looks great, thanks!
@Radiicall - are you able to test and confirm that this works on your macbook?

I dont have my mac on hand, test it in a VM maybe

guh 😔

- Updated name of plist file
- Updated capitalization of Linux
@Radiicall
Copy link
Contributor

this looks great, thanks!

@Radiicall - are you able to test and confirm that this works on your macbook?

I dont have my mac on hand, test it in a VM maybe

guh 😔

Did you test this on linux?

Copy link
Contributor

@Radiicall Radiicall left a comment

Choose a reason for hiding this comment

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

Works on macOS!

@JustTemmie
Copy link
Owner

JustTemmie commented Aug 27, 2023

Works on macOS!

sick, massive thanks to both of you!

@JustTemmie JustTemmie merged commit 80d5289 into JustTemmie:main Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants