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

Draft : Add option to lock/unlock window resizing via keyboard shortcut #6423

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

TusharGautam29
Copy link

A new feature that allows users to toggle window resizing on or off that users can toggle it dynamically via a keyboard shortcut.(with F11)

Feature- #15124
#15124

Changing the flag of window created based on the private variable _lockedScreen. Will add a SetLockedScreen(bool val) function and implement it into event system.

Request for Feedback

@@ -43,7 +43,7 @@ int __stdcall WinMain(HINSTANCE /*hInst*/, HINSTANCE /*hPrevInst*/, LPSTR /*lpC
SDL_SetModuleHandle(GetModuleHandle(NULL));
#endif
// HACK: __argc, __argv are broken and return zero when using mingwrt 4.0+ on MinGW
// HACK: MinGW-w64 based toolchains neither feature _argc nor _argv. The 32 bit
//- HACK: MinGW-w64 based toolchains neither feature _argc nor _argv. The 32 bit
Copy link
Member

Choose a reason for hiding this comment

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

This seems to have been modified by mistake

Suggested change
//- HACK: MinGW-w64 based toolchains neither feature _argc nor _argv. The 32 bit
// HACK: MinGW-w64 based toolchains neither feature _argc nor _argv. The 32 bit

@bluegr
Copy link
Member

bluegr commented Feb 4, 2025

Looks good at a first glance, up to now
Please adapt the commit message according to our commit guidelines

@lephilousophe
Copy link
Member

The 3D graphics part (in graphics3d/openglsdl) is also missing.

@TusharGautam29
Copy link
Author

While working on this feature i noticed that if I initialise _lockedScreen = true then no GUI theme can load,
Error - Failed to load any GUI theme, aborting!

I removed all the _lockedScreen variables , now only one exists in WindowedGraphicsManager.

OpenGLSdlGraphics3dManager, OpenGLGraphicsManager ,SdlGraphicsManager now has a function to toggle the variable.

I'll commit the code so you can analyse and help me with it

Copy link
Member

@lephilousophe lephilousophe left a comment

Choose a reason for hiding this comment

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

Do you really need to touch to WindowedGraphicsManager and OpenGLGraphicsManager?

These classes are used by many other backends and touching them doesn't seem necessary to me.
The first commit seemed to start better.

Please note that SdlGraphicsManager is a common ancestor for all SDL graphical backends (2D and 3D).

@@ -115,6 +115,10 @@ class WindowedGraphicsManager : virtual public GraphicsManager {
int getWindowWidth() const { return _windowWidth; }
int getWindowHeight() const { return _windowHeight; }

// New methods to lock and unlock the screen
virtual void setLockedScreen(bool val) { _lockedScreen = val; }
Copy link
Member

Choose a reason for hiding this comment

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

Why do you define getter and setter?
You could put the variable protected and it would be accessible to children classes.

@@ -1272,57 +1275,67 @@ void OpenGLGraphicsManager::grabPalette(byte *colors, uint start, uint num) cons

void OpenGLGraphicsManager::handleResizeImpl(const int width, const int height) {
// Setup backbuffer size.
_targetBuffer->setSize(width, height, getRotationMode());
if (WindowedGraphicsManager::isScreenLocked()) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this?

@@ -207,6 +207,9 @@ bool OpenGLGraphicsManager::setGraphicsMode(int mode, uint flags) {
int OpenGLGraphicsManager::getGraphicsMode() const {
return _currentState.graphicsMode;
}
void OpenGLGraphicsManager::setLockedScreen(bool val) {
Copy link
Member

Choose a reason for hiding this comment

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

This is completely unnecessary.

@@ -299,6 +299,9 @@ void OpenGLSdlGraphicsManager::initSize(uint w, uint h, const Graphics::PixelFor

return OpenGLGraphicsManager::initSize(w, h, format);
}
void OpenGLSdlGraphicsManager::setLockedScreen(bool val) {
Copy link
Member

Choose a reason for hiding this comment

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

Either you put stuff in WindowedGraphicsManager, either you put it in SdlGraphicsManager.
In both case this is useless.

@@ -455,7 +458,9 @@ bool SdlGraphicsManager::notifyEvent(const Common::Event &event) {
return false;
}
}

void SdlGraphicsManager::setLockedScreen(bool val) {
Copy link
Member

Choose a reason for hiding this comment

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

Same, useless. And it misses empty lines around.

@@ -1554,6 +1554,10 @@ void SurfaceSdlGraphicsManager::internUpdateScreen() {
#endif
}

void SurfaceSdlGraphicsManager::setLockedScreen(bool val) {
Copy link
Member

Choose a reason for hiding this comment

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

To be removed.

@TusharGautam29
Copy link
Author

Ok, i'll look into the changes you suggested and work as per that. Thank you

@TusharGautam29
Copy link
Author

I removed it from WindowedGraphicsManager as it was causing issues if i set it to true, This is working for both cases, Also i added it to event system, but doesn't seem to be working. Any feedback on that please

Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

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

Added some notes. Please cleanup.

@@ -1294,16 +1294,15 @@ void OpenGLGraphicsManager::handleResizeImpl(const int width, const int height)
// possible and then scale it to the physical display size. This sounds
// bad but actually all recent chips should support full HD resolution
// anyway. Thus, it should not be a real issue for modern hardware.
if ( overlayWidth > (uint)OpenGLContext.maxTextureSize
|| overlayHeight > (uint)OpenGLContext.maxTextureSize) {
if (overlayWidth > (uint)OpenGLContext.maxTextureSize || overlayHeight > (uint)OpenGLContext.maxTextureSize) {
Copy link
Member

Choose a reason for hiding this comment

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

These changes are irrelevant, please remove all of them. If you want/need to do whitespace changes, those should go separately, but in this particular case, they are made on purpose, and obviously, it is just your code editor reformatting them, thus, please revert.

@@ -299,7 +299,6 @@ void OpenGLSdlGraphicsManager::initSize(uint w, uint h, const Graphics::PixelFor

return OpenGLGraphicsManager::initSize(w, h, format);
}

Copy link
Member

Choose a reason for hiding this comment

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

Please revert

@@ -914,6 +945,7 @@ bool OpenGLSdlGraphicsManager::notifyEvent(const Common::Event &event) {
return true;
}


Copy link
Member

Choose a reason for hiding this comment

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

Please revert

@@ -97,6 +101,8 @@ class OpenGLSdlGraphicsManager : public OpenGL::OpenGLGraphicsManager, public Sd
uint _graphicsScale;
bool _gotResize;

bool _lockedScreen = false; // New member variable to track lock state
Copy link
Member

Choose a reason for hiding this comment

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

What makes it "New" besides being in this PR? :)

Suggested change
bool _lockedScreen = false; // New member variable to track lock state
bool _lockedScreen = false; // Variable to track lock state

@@ -455,7 +472,6 @@ bool SdlGraphicsManager::notifyEvent(const Common::Event &event) {
return false;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Same thing. Please review all files and revert all of these whitespace changes. I will not comment on them more, just please revert.

@@ -65,7 +65,7 @@ class WindowedGraphicsManager : virtual public GraphicsManager {
_cursorY(0),
_cursorNeedsRedraw(false),
_cursorLastInActiveArea(true) {}

Copy link
Member

Choose a reason for hiding this comment

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

This is even worse: it adds a trailing tab. Please revert

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