-
Notifications
You must be signed in to change notification settings - Fork 99
Linux: Wrapper and scripts modernizations #4982
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
base: develop-linux
Are you sure you want to change the base?
Conversation
…on and SLURL handling
|
Thanks - I'll check this out when I'm in front of my Linux box again (currently on a work trip). |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
precommit is complaining about spaces in install.sh line 105 |
Fixes precommit check
|
This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or it will be closed in 7 days |
|
This branch is not stale, it is awaiting review. |
Removed the label. But probably not going anywhere without Rye or at least Geenz. |
clairem-sl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments.
| build_data_file="build_data.json" | ||
| if [ -f "${build_data_file}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail if install.sh is not executed as ./install.sh.
Better to put this part after the detection of RUN_PATH, and embed $RUN_PATH in the build_data_file string.
| [Desktop Entry]\n\ | ||
| Name=Second Life\n\ | ||
| Version=1.4\n\ | ||
| Name=${launcher_name}\n\ | ||
| GenericName=Second Life Viewer\n\ | ||
| Comment=Client for the On-line Virtual World, Second Life\n\ | ||
| Comment=Client for the Online Virtual World, Second Life\n\ | ||
| Path=${installation_prefix}\n\ | ||
| Exec=${installation_prefix}/secondlife\n\ | ||
| Icon=${installation_prefix}/secondlife_icon.png\n\ | ||
| Icon=${desktopfilename}\n\ | ||
| Terminal=false\n\ | ||
| Type=Application\n\ | ||
| Categories=Game;Simulation;\n\ | ||
| StartupNotify=true\n\ | ||
| StartupWMClass="com.secondlife.indra.viewer"\n\ | ||
| X-Desktop-File-Install-Version=3.0" | ||
| StartupWMClass=\"com.secondlife.indra.viewer\"\n\ | ||
| X-Desktop-File-Install-Version=3.0 | ||
| PrefersNonDefaultGPU=true\n\ | ||
| Actions=DefaultGPU;AssociateMIME;\n\ | ||
| \n\ | ||
| [Desktop Action DefaultGPU]\n\ | ||
| Exec=env __GLX_VENDOR_LIBRARY_NAME=\"\" ${installation_prefix}/secondlife\n\ | ||
| Name=Launch on default GPU\n\ | ||
| \n\ | ||
| [Desktop Action AssociateMIME]\n\ | ||
| Exec=${installation_prefix}/etc/register_secondlifeprotocol.sh\n\ | ||
| Name=Associate SLURLs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does every line end with \n\ ? We can remove all of them. The quoting during printf should keep the line breaks.
| mv "$1" "$backup_dir" || die "Failed to create backup of existing installation!" | ||
| } | ||
|
|
||
| set_slurl_handler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not using function
Though it is not strictly necessary, the convention seems to be using the function keyword. In addition using function makes the function stand out more.
| echo | ||
| prompt "Would you like to set Second Life as your default SLurl handler? [Y/N]: " | ||
| if [ $? -eq 0 ]; then | ||
| exit 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be return 0 instead?
exit 0 ends the script completely, meaning there are steps in root_install() that will not be executed (involving "refresh_desktop_app_entry")
| @@ -1,57 +1,66 @@ | |||
| #!/bin/bash | |||
| #!/bin/env sh | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no benefits running this script with sh instead of bash.
It is understandable for the actual handler to be executed by sh (which is likely much lighter -- unless it's a symlink to bash), but for this script that is invoked only during installation, better to just use bash, enabling us to use bash's powerful features.
| # Check if xdg-mime is present, if so, use it to register new protocol. | ||
| if command -v xdg-mime >/dev/null 2>&1; then | ||
| urlhandler=$(xdg-mime query default x-scheme-handler/secondlife) | ||
| localappdir="${HOME}/.local/share/applications" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be using ${XDG_DATA_HOME}/applications instead?
Also, this causes breakage if installation was done using sudo.
Description
Related Issues
Checklist
Please ensure the following before requesting review:
Additional Notes
These changes have been live in Alchemy's release viewers for some time now. This PR is to merge them upstream.
Suggest review from @RyeMutt .