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 the --dolphin-process-name command-line option. #105

Merged
merged 7 commits into from
Mar 7, 2024

Conversation

cristian64
Copy link
Collaborator

Used to override the name of the Dolphin Emulator process that the DME will search for in the system while trying to hook.

The short version of the option is -d.

The [undocumented] DME_DOLPHIN_PROCESS_NAME environment variable can
be used alternatively.

Fixes #42.

This work also enables the --version and --help command-line arguments.

Bonus: Trailing spaces, as well as \r characters, have been removed.

@cristian64
Copy link
Collaborator Author

cristian64 commented Mar 6, 2024

The first two three commits are only whitespace changes. (I can move them to a separate PR if it makes things easier.)

The changes for Windows and macOS have been made blindly; unable to test. Let's see if the build succeeds. (It builds.)

EDIT: @dreamsyntax Would you able to test at least on Windows? It'd be interesting to test also with non-ASCII characters, which I'm hoping it will work.

@cristian64 cristian64 force-pushed the application_arguments branch 5 times, most recently from d1b07bc to 8a84dd2 Compare March 6, 2024 22:44
@dreamsyntax
Copy link
Collaborator

Would you able to test at least on Windows? It'd be interesting to test also with non-ASCII characters, which I'm hoping it will work.

Tested with process name '攫'
image

Launching with no arg:
image

Launching with --dolphin-process-name 攫 or -d 攫:
No luck, does not detect the process.

Launching with --dolphin-process-name 攫.exe or -d 攫.exe:
Attached properly.

Also tested with ShadowSX as name of Dolphin process. The results are the same.

--
Testing with no modification to Dolphin.exe also works as expected.

--
Can we implicitly add .exe to the search type of the user does not specify? Or do you think its standard to require it?

@cristian64
Copy link
Collaborator Author

Would you able to test at least on Windows? It'd be interesting to test also with non-ASCII characters, which I'm hoping it will work.

[...]

-- Can we implicitly add .exe to the search type of the user does not specify? Or do you think its standard to require it?

I think my preference is to be specific, but I'm having a hard time finding a case where the implicit extension could be a problem. So, happy to add it if you haven't changed your mind since your comment.

@dreamsyntax
Copy link
Collaborator

dreamsyntax commented Mar 7, 2024

Would you able to test at least on Windows? It'd be interesting to test also with non-ASCII characters, which I'm hoping it will work.

[...]
-- Can we implicitly add .exe to the search type of the user does not specify? Or do you think its standard to require it?

I think my preference is to be specific, but I'm having a hard time finding a case where the implicit extension could be a problem. So, happy to add it if you haven't changed your mind since your comment.

I would think
-> check explicit, if not found and on windows assume .exe or something like that

Or just make it clear in the help/readme that windows must include the extension.
I'm fine with that too.

@cristian64
Copy link
Collaborator Author

Would you able to test at least on Windows? It'd be interesting to test also with non-ASCII characters, which I'm hoping it will work.

[...]
-- Can we implicitly add .exe to the search type of the user does not specify? Or do you think its standard to require it?

I think my preference is to be specific, but I'm having a hard time finding a case where the implicit extension could be a problem. So, happy to add it if you haven't changed your mind since your comment.

I would think -> check explicit, if not found and on windows assume .exe or something like that

Or just make it clear in the help/readme that windows must include the extension. I'm fine with that too.

The option's help description does say the file extension must be given, but the Windows ecosystem is known for not minding.

@dreamsyntax
Copy link
Collaborator

Would you able to test at least on Windows? It'd be interesting to test also with non-ASCII characters, which I'm hoping it will work.

[...]
-- Can we implicitly add .exe to the search type of the user does not specify? Or do you think its standard to require it?

I think my preference is to be specific, but I'm having a hard time finding a case where the implicit extension could be a problem. So, happy to add it if you haven't changed your mind since your comment.

I would think -> check explicit, if not found and on windows assume .exe or something like that
Or just make it clear in the help/readme that windows must include the extension. I'm fine with that too.

The option's help description does say the file extension must be given, but the Windows ecosystem is known for not minding.

Your call. I'm fine either way, merge when you think its good to go.
If you dont do this:
#105 (comment)
I can just add it before release.

A few lines in a few files were still using `CRLF`.
The `.vscode` had been inadvertently checked in in aed47a1.
It enables the use of `--version` and `--help` in the command line.
Used to override the name of the Dolphin Emulator process that the DME
will search for in the system while trying to hook.

The short version of the option is `-d`.

The [undocumented] `DME_DOLPHIN_PROCESS_NAME` environment variable can
be used alternatively.

Fixes aldelaro5#42.
@cristian64 cristian64 force-pushed the application_arguments branch from 8a84dd2 to 6de1975 Compare March 7, 2024 20:07
@cristian64
Copy link
Collaborator Author

I've made the tweak so that, on Windows, the .exe file extension can be omitted.

Copy link
Collaborator

@dreamsyntax dreamsyntax left a comment

Choose a reason for hiding this comment

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

Tested with
-d test
and
-d test.exe

Both work as expected.

@dreamsyntax dreamsyntax merged commit 0dea3f5 into aldelaro5:master Mar 7, 2024
3 checks passed
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.

Feature: Support any executable name
2 participants