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

Improvements #1274

Merged
merged 5 commits into from
Jan 6, 2023
Merged

Improvements #1274

merged 5 commits into from
Jan 6, 2023

Conversation

ge0rdi
Copy link
Member

@ge0rdi ge0rdi commented Jan 4, 2023

@ge0rdi ge0rdi self-assigned this Jan 4, 2023
@ge0rdi ge0rdi requested a review from bonzibudd January 4, 2023 11:26
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@bonzibudd
Copy link
Member

Looks good from what I can see, I will do some testing later today. However I noticed that Open-Shell seems to override the default for the user_name option on the classic skins, and this works well on the Classic1 style but on Classic2 I would prefer it to be enabled by default. If there’s a way an exception can be added for the classic2 style that would be good.

The modern tasks and Settings icon look good so far.

@bonzibudd
Copy link
Member

#162 and #262 are also related to the new theme.

@ge0rdi
Copy link
Member Author

ge0rdi commented Jan 4, 2023

However I noticed that Open-Shell seems to override the default for the user_name option on the classic skins

Here is the change:
https://github.com/Open-Shell/Open-Shell-Menu/pull/1274/files#diff-82c43507e4ac2c77acac190d1f5ac373ebaf3c7365b3555967e7e708704aa631L5098-R5102

options1 are default for classic skin, options2 for two column and options3 for win7 style.
What do you think should be proper defaults?

The modern tasks and Settings icon look good so far.

Cool 👍

@bonzibudd
Copy link
Member

The only thing that should be changed is that USER_NAME for options2 should be 1. The other defaults look correct.

@ge0rdi
Copy link
Member Author

ge0rdi commented Jan 4, 2023

The only thing that should be changed is that USER_NAME for options2 should be 1.

Hmm, but originally the USER_NAME defaulted to 0.
So you want to change that default for two column type, right?
I mean I have no problem with it. Just wanted to be sure (as it changes this for other skins too, but again no problem on my side).

@ge0rdi
Copy link
Member Author

ge0rdi commented Jan 4, 2023

In fact, I think I will revert that change of options1, options2, options3 defaults.
As it doesn't seem to be necessary (for Immersive skin). And for options not listed here we will pick defaults from actual skin definition.

So the only change there will be USER_NAME=1 for options2 if you'll confirm that you want to do this change (for all the skins).

@bonzibudd bonzibudd closed this Jan 4, 2023
@bonzibudd bonzibudd reopened this Jan 4, 2023
@AppVeyorBot
Copy link

@bonzibudd
Copy link
Member

Actually, since my skin has USER_NAME set at 1, we should be able to get rid of that override too. That should be a better solution for the other skins.

However, we should still keep the options1 overrides for USER_IMAGE and USER_NAME at 0, as it makes more sense with the format of those skins. The skin files can't specify different defaults between the two classic styles, so that's why it is defined here.

@ge0rdi
Copy link
Member Author

ge0rdi commented Jan 5, 2023

Oki, so I have reverted the change in defaults and removed USER_NAME=0 from options2.
That way it will use skin defaults (which is enabled for Immersive skin).

@AppVeyorBot
Copy link

@bonzibudd
Copy link
Member

Looks good, I will test as soon as I can.

@bonzibudd
Copy link
Member

Hey @ge0rdi, I've just found a potential crash with the Shutdown command on Windows 10, would you like me to attach info here or on a separate issue?

@ge0rdi
Copy link
Member Author

ge0rdi commented Jan 5, 2023

I guess separate issue would be better. Or just team discussion?

@bonzibudd
Copy link
Member

Everything looks good from what I can see. The skin seems to be loading fine in every configuration I tested. I tried the Settings icon in Win8.1 and several versions of 10, and it works well.

@ge0rdi ge0rdi merged commit ecd17cb into master Jan 6, 2023
@ge0rdi ge0rdi deleted the improvements branch January 6, 2023 17:05
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.

3 participants