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.

Sign up for GitHub

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

Jump to bottom

Linux desktop entry filename fix #91

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Linux desktop entry filename fix #91

merged 3 commits into from
Dec 4, 2023

Conversation

artemv
Copy link
Contributor

@artemv artemv commented Nov 12, 2018 •
edited
Loading

This changes things for Linux so that the desktop entry file is now named ${name}.desktop instead of previous ${pathFilename}.desktop, so if previous version of app-launch worked for the same app this version will add another desktop entry. This will cause OS to run 2 app instances on user login unless app is set up as singleton, which may cause issues - so this should be a major version update.

@artemv
fix: Linux desktop entry file should be named based on name, not `p…
5aa6d89 
…ath`

no need to overwrite `name` options with filename on Linux

..plus test

fixes Teamwork#90
@artemv
fix(test): corrected test
fcad050
artemv added a commit to artemv/node-auto-launch that referenced this pull request Nov 12, 2018
@artemv
chore: publish fork
85d45f3 
.. to be able to use it until Teamwork#91 is merged
@artemv artemv mentioned this pull request Nov 12, 2018
Closed
@yafp
Copy link

yafp commented Jan 9, 2020

would love to see that merged

@jopemachine jopemachine mentioned this pull request May 31, 2021
Open
@Oxalin
Copy link
Contributor

Oxalin commented Nov 30, 2023

Hi @artemv This PR has been rotting for a long time. I'm working on improving the auto-launch experience on Linux. I'll review the PR later tonight. A quick look seems ok.

@Oxalin
Copy link
Contributor

Oxalin commented Dec 3, 2023

I see that you removed Linux and BSDs from any name changes from fixOpts, am I right? I also see that you moved the path separator treatment either under Windows or macOS. This makes sense.

However, what I'm wondering right now is why would we change the appName in the first place. The "name" parameter is mandatory when calling the constructor, so why are we not respecting this. This appName can be anything under Windows and my understanding is it can also be anything on macOS.

The only concern we must have is with "reserved characters" like spaces in the appName. Under POSIX derived OSes, the filename must be properly escaped in a path, while the appName can contain spaces in the data we write to the .desktop (Linux) or .plist (macOS) file.

Your test shall then be applied to all OSes.

Any feedback is welcomed @artemv @4ver @yafp

Oxalin
Oxalin approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@Oxalin Oxalin left a comment

Choose a reason for hiding this comment

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

We should add a comment about keeping the appName provided to the constructor under Linux.

Side note: we should consider doing the same for other OSes. The name provided should be honored and there is no technical reason to not do it.

@Oxalin Oxalin merged commit 066f341 into Teamwork:master Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oxalin Oxalin Oxalin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@artemv @yafp @Oxalin