Skip to content

Conversation

@Elihpmap
Copy link
Contributor

@Elihpmap Elihpmap commented Dec 16, 2024

fix proposition for this issue
Here's a comparison of logs with my AutoHotkey script

// ----- launching on master branch with "OpenURL" profile from Visual Studio :
16/12/2024 14:27:52 BrowseRouter: Attempting to launch "example.org" for "BrowseRouter (Exécution) - Microsoft Visual Studio"
16/12/2024 14:27:52 BrowseRouter: Found URL preference "*" => "ff" ("C:\Program Files\Mozilla Firefox\firefox.exe")
16/12/2024 14:27:52 BrowseRouter: Launching C:\Program Files\Mozilla Firefox\firefox.exe with args " https://example.org"

// ----- launching on master branch with a custom autohotkey script
16/12/2024 14:28:18 BrowseRouter: Attempting to launch "example.org" for "BrowseRouter (Exécution) - Microsoft Visual Studio"
16/12/2024 14:28:18 BrowseRouter: Found URL preference "*" => "ff" ("C:\Program Files\Mozilla Firefox\firefox.exe")
16/12/2024 14:28:18 BrowseRouter: Launching C:\Program Files\Mozilla Firefox\firefox.exe with args " https://example.org"

// ----- launching on CallingProcess-detection branch with "OpenURL" profile from Visual Studio :
16/12/2024 14:28:51 BrowseRouter: Attempting to launch "example.org" for "BrowseRouter (Exécution) - Microsoft Visual Studio -> devenv"
16/12/2024 14:28:51 BrowseRouter: Found URL preference "*" => "ff" ("C:\Program Files\Mozilla Firefox\firefox.exe")
16/12/2024 14:28:51 BrowseRouter: Launching C:\Program Files\Mozilla Firefox\firefox.exe with args " https://example.org"

// ----- launching on CallingProcess-detection branch with a custom autohotkey script
16/12/2024 14:29:17 BrowseRouter: Attempting to launch "example.org" for " -> AutoHotkey64"
16/12/2024 14:29:17 BrowseRouter: Found URL preference "*" => "ff" ("C:\Program Files\Mozilla Firefox\firefox.exe")
16/12/2024 14:29:17 BrowseRouter: Launching C:\Program Files\Mozilla Firefox\firefox.exe with args " https://example.org"

I hope adding "-> " to the main window title isn't too big of a problem for compatibility. In most cases, I believe adding "*" to the program name in the config file should be sufficient to update them to this new detection system.

…ow it should detects non foreground applications too)
…o main windows (so no main windows title too in this case)
@nref
Copy link
Owner

nref commented Dec 17, 2024

Neat idea! Yeah, another user contributed the window title solution, and I observed then that if I quickly changed the focused application after clicking a link, the newer app title was used.

Cool that it works. Here are the changes I'd like

  • Rename ParentProcessUtilities to BasicProcessInfo
  • Extract the other methods to a new nonstatic class, ProcessService, which implements IProcessService. This will make it mockable and testtable, if we ever add tests.

Thanks!

@Elihpmap
Copy link
Contributor Author

I also updated a few line in the readme for the recent changes to implement silent notifications and environment variable handling while I was at it!
I was wondering: perhaps the default config.ini file and the example one written in the readme file shouldn't be that different from one another ? I feel like at least adding the comments from the example to the default would be friendlier to the ones not reading the whole readme before testing the program.

# - Will take precedence over any URL preferences.
# - Matches on window title and specific process of the application used to open the link, like so "WindowTitle -> ProcessName".
[sources]
* - Notepad -> notepad = ff
Copy link
Owner

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.

Copy link
Contributor Author

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 :

I hope adding "-> " to the main window title isn't too big of a problem for compatibility. In most cases, I believe adding "*" to the program name in the config file should be sufficient to update them to this new detection system.

For example * - Notepad in the config.ini will not recognise New File - Notepad -> notepad after 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 correctly New File - Notepad -> notepad and not recognise Weird window name that is not - Notepad, right? -> processName (whereas * - Notepad* would wrongly recognise it)

Copy link
Contributor Author

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 ?

Copy link
Owner

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.

Copy link
Contributor Author

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 :

  1. As proposed in my previous message : checks when reading the config file lines for the [sources] part and try to detect if it need to compare to the legacy version or the new one. Might be unreliable, adds processing for each read line but allow to have the legacy version and the new one side by side...
  2. Add another category called [Process Source] for example just to compare the process with no link with the source window. This is cleaner but there is no way to differentiate two processes with the same name but from different programs for example. I'm also not sure whether this should take precedence over [source] or not... Not the best solution.
  3. Add a config file version number to detect which version to use (and suppose no version # means 0.14.0 or lower) (my prefered solution for now):
    • a new Changelog file could be added to repository and contain simple directions for anyone wanting to update their config file.
    • we could also add an integrated config file updater in the program for the simpler changes (to easily add the mentionned characters in this case)
    • I have other thing I plan to implement that risk to break compatibility at one point or another so having this system might mitigate them too

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

Copy link
Owner

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 ini file 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 = ff

Copy link
Contributor Author

@Elihpmap Elihpmap Jan 31, 2025

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 have often wished I could migrate to something better, like JSON or YAML

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 ?

Question. Do you find this syntax intuitive? * - Notepad -> notepad = ff

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!

@nref nref merged commit 6d233a1 into nref:main Jan 17, 2025
1 of 2 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.

2 participants