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

Sanitize shortcuts #766

Closed
wants to merge 10 commits into from
Closed

Sanitize shortcuts #766

wants to merge 10 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 4, 2021

Fixes #760 and adds a steam prompt like this:
image

  • Adapt functions and methods
  • Remove settings
  • Sanitize desktop shortcut name
  • Prompt UI

@ghost ghost requested a review from flavioislima December 4, 2021 14:28
@ghost ghost marked this pull request as draft December 4, 2021 14:28
@ghost ghost changed the title Draft: Sanitize shortcuts Sanitize shortcuts Dec 4, 2021
@ghost ghost linked an issue Dec 4, 2021 that may be closed by this pull request
@ghost ghost marked this pull request as ready for review December 4, 2021 14:53
@ghost ghost added the pr:ready-for-review Feature-complete, ready for the grind! :P label Dec 4, 2021
@ghost ghost self-assigned this Dec 4, 2021
@flavioislima
Copy link
Member

I thibk you have removed the options from backend but is keeping on frontend. Need to remove from settings screen as well.

@flavioislima
Copy link
Member

I think that if this is to fix the issue, the pr could focus only on that part. Using regex to remove the special characters.
After that you could use another pr to make the other changes and the ui prompt. Sounds good?

@ghost
Copy link
Author

ghost commented Dec 4, 2021

I thibk you have removed the options from backend but is keeping on frontend. Need to remove from settings screen as well.

I've removed them. And tested locally and the options weren't visible.

@ghost
Copy link
Author

ghost commented Dec 4, 2021

I think that if this is to fix the issue, the pr could focus only on that part. Using regex to remove the special characters. After that you could use another pr to make the other changes and the ui prompt. Sounds good?

OK I will work on the prompt on another PR but lets leave this PR as-is

@flavioislima
Copy link
Member

I thibk you have removed the options from backend but is keeping on frontend. Need to remove from settings screen as well.

I've removed them. And tested locally and the options weren't visible.

Ah OK. I'm blind and missed the changes on the settings file.
OK then.

@ghost
Copy link
Author

ghost commented Dec 4, 2021

I think it needs to be replaced by a empty string sonit will be removed instead of adding a empty space on it.

OK fixed

@ghost
Copy link
Author

ghost commented Dec 5, 2021

I think that this is ready to merge, what do you think @flavioislima

Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

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

Looks good. Just check my suggestion for regexp

dragonDScript and others added 2 commits December 5, 2021 14:30
Co-authored-by: Flávio F Lima <flavioislima@gmail.com>
@ghost
Copy link
Author

ghost commented Dec 5, 2021

@flavioislima OK I think we can merge now

@ghost ghost requested a review from flavioislima December 5, 2021 13:52
if (process.platform === 'darwin') {
return
}

const gameInfo = await this.getGameInfo()
const launchWithProtocol = `heroic://launch/${gameInfo.app_name}`
const [ desktopFile, menuFile ] = this.shortcutFiles(gameInfo.title)
const { addDesktopShortcuts, addStartMenuShortcuts } = await GlobalConfig.get().getSettings()
const sanitizedTitle = gameInfo.title
Copy link
Member

Choose a reason for hiding this comment

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

Since the issue at #760 is on Windows, this actually doesn't fix it, since this title is used only on linux.
We need another solution for windows.

Copy link
Author

Choose a reason for hiding this comment

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

What title is used in windows to create the shortcut filename?

@ghost ghost removed the pr:ready-for-review Feature-complete, ready for the grind! :P label Dec 22, 2021
@ghost ghost marked this pull request as draft December 22, 2021 10:36
@ghost ghost closed this Dec 28, 2021
@ghost ghost deleted the sanitize-shortcuts branch December 28, 2021 17:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant