-
-
Notifications
You must be signed in to change notification settings - Fork 449
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
use AssocQueryString instead of directly querying the registry #4362
Conversation
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.
Overall the functionality of this change looks good, just some refactoring to leave the windows-specific implementation in its own file would be nice.
As a consequence of this change, Windows builds that do build without the Windows SDK enabled will no longer support incognito links. This is something I'm fine with.
I also plan to implement linux support (well, freedesktop support) for this in a separate PR. Should I create a new XDGHelper or LinuxHelper file for that? |
Yes, for that sort of functionality additional OS-specific helper files in the same directory is fine (i.e. LinuxHelpers.cpp/hpp) |
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.
Nitpicking as I'm mostly unfamiliar with Windows APIs, thank you!
- use enum class instead of typedef and correct naming style for AssociationQueryType - use correct include path and order for local include - don't use unnecessary ASSOCF_VERIFY flag - check return code of both AssocQueryStringW calls - use wchar_t instead of TCHAR alias
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.
One last nitpick, otherwise looks good to me. @kornes if you have time to take a look that would be appreciated ❤️
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.
good change, no issues just 1 nitpick 👍
tested on win7 vm and incognito opens correctly, it fallbacks to .html assoc as expected.
This avoids returning an empty but not null QString if the stored association has an empty command.
Also split up the FAILED check & resultSize checks into separate if-statements
@sheybey I pushed two commits, are you ok with a merge after the changes I made? |
Looks good to me |
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.
works on windows 10 but I doubt anyone thought it wouldn't
Pull request checklist:
CHANGELOG.md
was updated, if applicableThe existing getCommand() in IncognitoBrowser.cpp directly queries undocumented registry keys which could break if Windows decides to change the registry layout (as they have in the past.)
This PR changes getCommand() to instead call AssocQueryString, which smooths out differences between versions of Windows.