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

classic-maximized-windows-fix 2.0 #447

Merged

Conversation

ephemeralViolette
Copy link
Contributor

  • Moved global window procedure hooking to leverage the availability of the theme engine (uxtheme), rather than using SetWindowsHookEx.
  • Improved compatibility with many apps, such as pre-Windows 8 versions of Task Manager.

- Moved global window procedure hooking to leverage the availability of the theme engine (uxtheme), rather than using SetWindowsHookEx.
- Improved compatibility with many apps, such as pre-Windows 8 versions of Task Manager.
symbolHooks[i].optional
};

if (!WindhawkUtils::HookSymbols(module, &proxyHook, 1))
Copy link
Member

Choose a reason for hiding this comment

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

I don't see much value in this wrapper. Is it to have an in-memory cache instead of a registry cache? Did you measure the difference?

Also, this mod only hooks one symbol, but generally this is going to be very slow, since this wrapper handles each symbol separately, and so the symbols will be enumerated again for each symbol, and enumerating symbols is usually the slow part in symbol loading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is the goal. I've noticed that application startup can still be quite slow, even with the registry cache, so I wanted to try something else out to make it nicer to use for hooking all running applications. It seems that the slowness of reading from the registry is dependent on the application in particular, but there are some for which it can be particularly noticeable (in my experience, Git and some old versions of Notepad).

I will improve the wrapper for future usage later, but I think it should be fine here since it's only used for the one symbol.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind merging it, but I doubt that the single registry read causes the slowness, Windhawk reads other values from the registry, the cache value really shouldn't matter. That's unlike the first run when the cache isn't available, for which symbols have to be downloaded and enumerated, but your wrapper doesn't improve this case.

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 elaborated on the possible inefficiency of the wrapper in the latest commit.

Added a comment stating a limitation of the design of the CmwfHookSymbols wrapper for anyone looking through the code for goodies.
@m417z m417z merged commit a7051cb into ramensoftware:main Dec 25, 2023
1 check 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.

None yet

2 participants