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

Fix the StartMenuDLL for classic theme support on the taskbar #1642

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

OrthodoxWindows
Copy link

Fixed DLL code for classic theme support.

These changes add classic theme support to the taskbar, as discussed here and here. The original author of these modifications is valinet

Fixed DLL code for classic theme support.
@AppVeyorBot
Copy link

@OrthodoxWindows OrthodoxWindows changed the title Update StartMenuDLL.cpp Fix the StartMenuDLL for classic theme support on the taskbar Aug 1, 2023
@ge0rdi
Copy link
Member

ge0rdi commented Aug 2, 2023

I'm sorry but this PR breaks functionality of Open-Shell, so it can't be merged in this state.

I have just installed in in Windows Sandbox (clean Win10) and enabled Customize taskbar option and here is the result:
image
You can try it for yourself.

Do you understand what those changes are about?
Or just created PR using patch that Valinet provided for single custom use case?

Because most of them just break existing functionality (Open-Shell has to support much more cases than just Classic theme with ExplorerPatcher).

@OrthodoxWindows
Copy link
Author

OrthodoxWindows commented Aug 2, 2023

Yes, I understand that in this state, it is not mergeable. On the other hand, I imagine that it is possible if it becomes configurable (a possible option which will leave the choice to activate it). However, it seems to me that it didn't break the installations in the original version, but maybe that's because it was a previous version. I will try to compare the current version to previous versions. When modifying the DLL, I noticed a change in line 2957 (among the lines modified by valinet). For the other lines, it was identical.
For use cases, it seems to be more than a single custom use case, quite a few people seem to be using it. Also, the usage is no longer specific to ExplorerPatcher, there is now a reproduction with a Windawk mods (which does the same thing as ExplorerPatcher for the taskbar in the classic theme).

@OrthodoxWindows
Copy link
Author

I'll look at the display issues in more detail (that said, I understood most of what the changes seem to do, as valinet explained the changes pretty well), and come back here if I know more.

@ge0rdi
Copy link
Member

ge0rdi commented Aug 2, 2023

It would be also great to describe what exactly is the issue and steps to replicate it.
I don't quite get what those changes are trying to solve.

@ge0rdi ge0rdi self-requested a review September 11, 2023 06:15
Copy link
Member

@ge0rdi ge0rdi left a comment

Choose a reason for hiding this comment

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

Cannot be merged in current state. See discussion above.

@Anixx
Copy link

Anixx commented Sep 28, 2023

Hello! Here is how the taskbar looks like without the patch compared to when using the patch:
nofix
fix

OpenShell version: 4.4.169 (the last build for which the patched dll is available)
Explorer Patcher version: 22621.2361.58.1
Windows build: 10.0.22000.2176

@ge0rdi
Copy link
Member

ge0rdi commented Nov 15, 2023

Please, don't add binary installer to PR changes. It won't get merged that way.
If you want to share installer for testing you can attach it here in comments.

@OrthodoxWindows
Copy link
Author

OrthodoxWindows commented Nov 15, 2023

Please, don't add binary installer to PR changes. It won't get merged that way.
If you want to share installer for testing you can attach it here in comments.

It wasn't on purpose, I wanted to add it to my fork, I didn't think it would add the file here. I'm deleting it right now.

@ge0rdi
Copy link
Member

ge0rdi commented Nov 15, 2023

Since you have opened this PR, then all changes in the source branch of your fork are synced here.

I'd probably suggest to just close this PR and probably submit new one once you finish it?

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.

4 participants