-
Notifications
You must be signed in to change notification settings - Fork 16
Calling process detection fix #68
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0493ad9
Added more accurate detection of the process calling BrowserRouter (n…
Elihpmap cc944e3
Include the name of the process too in case the calling program has n…
Elihpmap 3ce3d82
Reconstructed process detection to work with a new IProcessService in…
Elihpmap b170bc7
Merge branch 'master' into CallingProcess-Detection
Elihpmap e10944f
updated Readme to reflect the changes to the source detection
Elihpmap 5c4d219
fix wrong markdown formating in ReadMe
Elihpmap File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| using System.ComponentModel; | ||
| using System.Diagnostics; | ||
| using System.Runtime.InteropServices; | ||
|
|
||
| namespace BrowseRouter.Interop.Win32 | ||
| { | ||
| /// <summary> | ||
| /// A utility class to determine a process parent. Originally copied from https://stackoverflow.com/a/3346055 | ||
| /// </summary> | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| public struct BasicProcessInfo | ||
| { | ||
| // These members must match PROCESS_BASIC_INFORMATION | ||
| internal IntPtr Reserved1; | ||
| internal IntPtr PebBaseAddress; | ||
| internal IntPtr Reserved2_0; | ||
| internal IntPtr Reserved2_1; | ||
| internal IntPtr UniqueProcessId; | ||
| internal IntPtr InheritedFromUniqueProcessId; | ||
|
|
||
| [DllImport("ntdll.dll")] | ||
| private static extern int NtQueryInformationProcess(IntPtr processHandle, int processInformationClass, ref BasicProcessInfo processInformation, int processInformationLength, out int returnLength); | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// Gets the parent process of a specified process. | ||
| /// </summary> | ||
| /// <param name="handle">The process handle.</param> | ||
| /// <returns>An instance of the Process class.</returns> | ||
| public static Process? GetParentProcess(IntPtr handle) | ||
| { | ||
| BasicProcessInfo pbi = new(); | ||
| int status = NtQueryInformationProcess(handle, 0, ref pbi, Marshal.SizeOf(pbi), out _); | ||
| if (status != 0) | ||
| throw new Win32Exception(status); | ||
|
|
||
| try | ||
| { | ||
| return Process.GetProcessById(pbi.InheritedFromUniqueProcessId.ToInt32()); | ||
| } | ||
| catch (ArgumentException) | ||
| { | ||
| // not found | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| using System.ComponentModel; | ||
| using System.Diagnostics; | ||
| using System.Runtime.InteropServices; | ||
| using BrowseRouter.Interop.Win32; | ||
|
|
||
| namespace BrowseRouter | ||
| { | ||
|
|
||
| public interface IProcessService | ||
| { | ||
| /// <summary> | ||
| /// Try to get the name of the parent process of the current process. | ||
| /// </summary> | ||
| /// <param name="parentProcessTitle">The name of the parent process main window title (may be empty) and the specific process name.</param> | ||
| /// <returns>True if the name was succesfully found, False otherwise.</returns> | ||
| public bool TryGetParentProcessTitle(out string parentProcessTitle); | ||
| } | ||
|
|
||
| public class ProcessService : IProcessService | ||
| { | ||
| public bool TryGetParentProcessTitle(out string parentProcessTitle) | ||
| { | ||
| Process? parentProcess = GetParentProcess(); | ||
| if (parentProcess is null || (parentProcess.MainWindowTitle == string.Empty && parentProcess.ProcessName == string.Empty)) | ||
| { | ||
| parentProcessTitle = string.Empty; | ||
| return false; | ||
| } | ||
|
|
||
| parentProcessTitle = parentProcess.MainWindowTitle + " -> " + parentProcess.ProcessName; | ||
| return true; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the parent process of the current process. | ||
| /// </summary> | ||
| /// <returns>An instance of the Process class.</returns> | ||
| public static Process? GetParentProcess() | ||
| { | ||
| return BasicProcessInfo.GetParentProcess(Process.GetCurrentProcess().Handle); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the parent process of specified process. | ||
| /// </summary> | ||
| /// <param name="id">The process id.</param> | ||
| /// <returns>An instance of the Process class.</returns> | ||
| public static Process? GetParentProcess(int id) | ||
| { | ||
| Process process = Process.GetProcessById(id); | ||
| return BasicProcessInfo.GetParentProcess(process.Handle); | ||
| } | ||
|
|
||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just want to check my understanding: Is this backward-compatible? So that it doesn't break for people with older config.ini files.
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.
Unfortunately it is not 100% compatible with previous config.ini files. But updating them is pretty easy in most cases. As I said in the PR message :
For example
* - Notepadin the config.ini will not recogniseNew File - Notepad -> notepadafter the update, but writing* - Notepad*will.Actually adding " -> *" would be even better to update old entries in the config file.
Following the previous example,
* - Notepad -> *will recognise correctlyNew File - Notepad -> notepadand not recogniseWeird window name that is not - Notepad, right? -> processName(whereas* - Notepad*would wrongly recognise it)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.
Since there is no auto-update system with this tool nor any promises of future compatibility I believe, I thought the minimal change wasn't a big deal. But maybe I was wrong ?
If its an issue, I don't see how to make this compatible with older config.ini files without making a lot of checks when reading the file. And since those checks would be only for this compatibility with a (by comparison) incomplete version of the same data, that seems like a lot of weight compared to simply adding a few characters to existing entries in previous config files.
But maybe you have a solution for this if its an issue ?
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.
Sorry for the delay in making a decision. I'm unwilling to break backward compatibility. If you can't see a way to do that, I'll have to turn down this contribution.
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 thought about it and I now see a few solutions for this :
However this pull request will never be fully backward compatible though, if that's what you want. Simply because the previous behaviour is wrong (see issue for the details) and this fixes it. So if any config file relied on this bug being present, they would not work the same afterward.
I must admit I'm having trouble grasping why you're unwilling to break backward compatibility exactly, I mean this program hasn't crossed the 1.0.0 mark yet, right ? And the previous releases are not wiped out from the repo either...
I guess if that's still incompatible with you, I could simply do the changes I want on my fork. But I'd be a little bummed out to no longer be able to share back my upgrades to this official repository (which I was really happy to discover when I needed it initially) once my version will have diverged too much from this one. And I fear this might arrive quickly if compatibility with incomplete previous versions is always expected...
Have a good day! <3
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.
You've won me over with your kind and thoughtful response, and with suggestion of the config file version. We can go ahead and merge this PR, and will wait to create a release until that's in place. This project inherited the
inifile from BrowserSelector, and I have often wished I could migrate to something better, like JSON or YAML. Adding a version is a start.Question. Do you find this syntax intuitive?
* - Notepad -> notepad = ffUh oh!
There was an error while loading. Please reload this page.
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.
Thank you very much ! Sorry, I took a long time to come back to this!
I'm going to do a draft of the Changelog file to start then!
I've looked into this too and found that a good upgrade could be TOML since its very close to common .ini syntax. But the smallest library to implement this easily I found was still a 2000 line .cs file so maybe not the best solution. I like the readability of the current file, a YAML parser would be less huge perhaps ?
I was mainly searching for a delimitter that wouldn't appear often in normal window titles as this could make it harder for users to write string comarison that wouldn't trigger false positive otherwise.
->seemed to fit the role (I suppose most well distributed software would use the→character instead in their window title if they really needed an arrow) and I feel like it translate to the idea of "process" pretty well too. But I'm definitely okay to change this if you have a better idea, its definitely possible we could find something more intuitive!