Skip to content

[GEN][ZH] Refactor implemention for Shell recreation on Display Resolution change #1031

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

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

Conversation

xezon
Copy link

@xezon xezon commented Jun 8, 2025

With this change the Options Menu no longer closes when the Display Resolution is changed.

This change refactors the way the Shell recreation is implemented. It now recreates the window stack with the previous state, instead of recreating it manually by pushing the "Menus/MainMenu.wnd".

The initial motivation here was to try make the Display Resolution change also work in-game, but that will require a lot more work on top of this. So this change only makes improvements towards changing the resolution in the Main Menu as per the original restrictions.

User facing nothing changes.

TODO

  • Replicate in Generals

@xezon xezon added Bug Something is not working right, typically is user facing GUI For graphical user interface Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour labels Jun 8, 2025
@Polypheides
Copy link

Polypheides commented Jun 8, 2025

I'll try to ask this question more clearly.

By default, pressing the ACCEPT button closes the options menu. Example video:
Accept closes options

With this change, the ACCEPT button now keeps the options menu open. Example video:
Accept leave options open

Given this new behavior, would it make sense to rename the ACCEPT and CANCEL buttons to APPLY and CLOSE?

@xezon
Copy link
Author

xezon commented Jun 8, 2025

ACCEPT does not close the Options Menu.

// recreate the shell
delete TheShell;
TheShell = MSGNEW("GameClientSubsystem") Shell;
TheShell->init();
Copy link

@Caball009 Caball009 Jun 9, 2025

Choose a reason for hiding this comment

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

I noticed this was also in the original code, but is there a way to reset TheShell instead of delete and new?

Shell has a reset function, does that do the equivalent? It seems like it does:

Reset the shell system to a clean state just as though init had just been called and ready to re-use

Copy link
Author

Choose a reason for hiding this comment

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

Tested that. It works.

I think this then also means we can move most of the code of shell::recreateShell into a new function in Shell.

Copy link
Author

Choose a reason for hiding this comment

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

I was wrong. When I tested I did not update generalszh.exe. The Shell::reset function does not do what we need and using it breaks the menus on resolution change. I think it was meant to be used for non resolution change reset.

I did a refactor anyway and moved some of the code into the Shell class. The new function will now reconstruct the Shell from within. I also improved the conditioning for the main menu button transition, to only trigger it if the Main Menu is actually in the Screen stack and not hidden (which is always the case in the Menu, but would be relevant if we wanted to recreate the UI in-game, where the Main Menu is hidden).

@xezon xezon force-pushed the xezon/fix-optionsmenu-close-on-resolution-change branch from 2b8b0bb to e9f2ede Compare June 10, 2025 18:07
@xezon xezon changed the title [GEN][ZH] No longer close the Options Menu when changing Display Resolution [GEN][ZH] Refactor implemention for Shell recreation on Display Resolution change Jun 11, 2025
@xezon xezon added Refactor Edits the code with insignificant behavior changes, is never user facing and removed Bug Something is not working right, typically is user facing labels Jun 11, 2025
@xezon
Copy link
Author

xezon commented Jun 11, 2025

By default, pressing the ACCEPT button closes the options menu.

Right. I was totally confused here with what I was doing. This change was bogus.

I have now thrown out all the junk that changed the Options Menu ACCEPT button behaviour. It now functions the same way as it did originally. It now is a Refactor towards making the Resolution change work in-game in a future change.

This change should be correct now.

Copy link

@roossienb roossienb left a comment

Choose a reason for hiding this comment

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

I don't understand what this PR wants to be.

  • It fixes spelling errors in comments, removes tabs and add/removes blank lines in some locations, but not in others.
  • It adds the const keyword to some functions but not others.
  • It refactors the construction and deconstruction
  • It add new functionality through recreateWindowLayouts.

Choose a reason for hiding this comment

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

There are many other missing or multiple line breaks and random tabs. Why not fix those?

Copy link
Author

Choose a reason for hiding this comment

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

Right. I reverted all now unnecessary changes to this file.

@xezon xezon requested a review from a team June 13, 2025 10:19
@xezon
Copy link
Author

xezon commented Jun 15, 2025

  • It fixes spelling errors in comments, removes tabs and add/removes blank lines in some locations, but not in others.

These were remnants of earlier changes. I reverted this now.

  • It adds the const keyword to some functions but not others.

This is sort of required as part of this refactor.

void Shell::recreateWindowLayouts( void )
{
...
  const WindowLayout* layout = getScreenLayout(screenIndex);
  ScreenInfo& screenInfo = screenStackInfos[screenIndex];
  screenInfo.filename = layout->getFilename(); // this would fail to compile if the function was not const
  screenInfo.isHidden = layout->isHidden(); // this would fail to compile if the function was not const
  • It refactors the construction and deconstruction

This is required as part of this refactor. These are called in constructor + destructor, and also in recreateWindowLayouts.

  • It add new functionality through recreateWindowLayouts.

This is required as part of this refactor.

@xezon xezon requested a review from roossienb June 15, 2025 10:59
@xezon xezon force-pushed the xezon/fix-optionsmenu-close-on-resolution-change branch from c527755 to 373a9f3 Compare June 19, 2025 09:13
@xezon
Copy link
Author

xezon commented Jun 19, 2025

Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gen Relates to Generals GUI For graphical user interface Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing ZH Relates to Zero Hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants