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

Fixed detecting Windows 11 and wrong class casting #10

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

Conversation

skmedix
Copy link

@skmedix skmedix commented Sep 4, 2024

The changes merged from PR #6 are incorrect and are breaking the library. It introduced two bugs.

First, Windows 11 is no longer detected. The switch statement is getting a value from the os.version property, but we must remember that Windows 11 was essentially a Windows 10 refresh. This property returns 10 instead of 11 because of that. If we look into commons-lang3 to class SystemUtils, we can see that field IS_OS_WINDOWS_11 is also a result of an os.name match.

Secondly, returning an anonymous method may seem useful, but it is not possible with the current library design. If we encounter a situation where none of the system is supported and we try to cast the return into a specific ThemeWindowManager, it will throw a ClassCastException. It can be casted to ThemeWindowManager, but not to the created anonymous ThemeWindowManagerFactory$1. Therefore, I suggest making the method Nullable or considering using Optionals.

@dukke
Copy link
Owner

dukke commented Sep 9, 2024

Hi @skmedix thank you very much for this pull request!

One thing though, before I check the rest... It is my intention not to throw exceptions if there is an error in this library that doesn't cause an app to crash.
So, I would remove that exception you're throwing and replace it with the printing of a warning.
This will be similar to what happens in JavaFX CSS, if there is an error in your stylesheet definitions warnings are printed out and no exceptions are thrown because this won't affect the functioning of the app but only its aesthetics.

Thanks

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