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

GUI: Add system to automatically iterate through all resolutions in dialog dumper #5442

Merged
merged 4 commits into from Nov 17, 2023

Conversation

lunox2004
Copy link
Contributor

@lunox2004 lunox2004 commented Nov 16, 2023

Summary by CodeRabbit

  • New Features

    • Introduced a configuration option "force_resize" to control window resizing behavior.
    • Enhanced dialog dumping functionality to support multiple resolutions and languages.
  • Improvements

    • Window resizing now respects the "force_resize" setting, allowing for more flexible window management.
    • Dialog dumping process streamlined to automatically iterate through different resolutions and language settings.
  • Bug Fixes

    • Fixed an issue where window dimensions were not updating correctly when "force_resize" was enabled.

Copy link
Contributor

coderabbitai bot commented Nov 16, 2023

Walkthrough

The changes involve introducing a "force_resize" configuration setting that, when enabled, triggers a resize of the application window to specified dimensions. This setting affects both the initialization and the runtime behavior of the window size. Additionally, the GUI's dialog dumping functionality has been updated to handle multiple resolutions and languages, streamlining the process of generating dialog screenshots across different settings.

Changes

Files Change Summary
.../openglsdl/openglsdl-graphics.cpp Introduced "force_resize" check to conditionally resize the window during initialization and runtime.
gui/dump-all-dialogs.cpp Modified dumpDialogs to accept resolution parameters and iterate through resolutions and languages in dumpAllDialogs.

🐇 In the code's weave, a change takes its leave, 🍂
As autumn's breath does the window resize,
With each new setting, the dialogs align,
A rabbit's work, through the screen, does shine. 🌟


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Comment on lines 148 to 185
int original_window_width = ConfMan.getInt("last_window_width", Common::ConfigManager::kApplicationDomain);
int original_window_height = ConfMan.getInt("last_window_height", Common::ConfigManager::kApplicationDomain);
Common::List<Common::String> list = Common::getLanguageList();

const int res[] = {320, 200,
320, 240,
640, 400,
640, 480,
800, 600,
0};

ConfMan.setBool("force_resize", true, Common::ConfigManager::kApplicationDomain);
Common::FSNode dumpDir("snapshots");

if (!dumpDir.isDirectory())
dumpDir.createDirectory();

// Iterate through all languages available
for (Common::String lang : list) {
// Iterating through the resolutions doesn't work so you have to manually change it here.
// TODO: Find a way to iterate through the resolutions using code.
Common::Array<int> res_to_test = {800, 640, 320};
dumpDialogs(message, res_to_test[0], lang);
// Iterate through all resolutions available
for (const int *r = res; *r; r += 2) {
int w = r[0];
int h = r[1];

// Update resolution
ConfMan.setInt("last_window_width", w, Common::ConfigManager::kApplicationDomain);
ConfMan.setInt("last_window_height", h, Common::ConfigManager::kApplicationDomain);
g_system->beginGFXTransaction();
g_system->initSize(w, h);
g_system->endGFXTransaction();

// Iterate through all langauges
for (Common::String lang : list) {

dumpDialogs(message, w, h, lang);
}

}

#ifdef USE_TRANSLATION
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [148-194]

The dumpAllDialogs function has been modified to iterate through a predefined list of resolutions and dump dialogs for each resolution and language combination. This systematic approach is efficient for testing the GUI across different screen sizes. The function also ensures that the original window size and language settings are restored after the dumping process, which is a good practice to maintain the user's configuration. However, there is a potential issue with the use of g_system->beginGFXTransaction() and g_system->endGFXTransaction() without checking the return value of beginGFXTransaction(). It's important to verify that the graphics transaction has started successfully before proceeding with the size initialization.

Additionally, the force_resize configuration setting is used to ensure that the window is resized accordingly, which is a good use of the configuration manager to handle temporary settings. The creation of the snapshots directory if it doesn't exist is also a thoughtful touch to ensure that the dumping process doesn't fail due to a missing directory.

One thing to note is that the array res is terminated with a 0, which is used as a sentinel value to indicate the end of the array. This is a C-style approach and is acceptable, but in C++, it might be more idiomatic to use a std::vector of std::pair or a similar container to represent the resolutions.

std::vector<std::pair<int, int>> resolutions = {
    {320, 200},
    {320, 240},
    // ... other resolutions ...
};

This would make the iteration more idiomatic and less error-prone since you wouldn't need to rely on a sentinel value and manual pointer arithmetic.

Comment on lines 148 to 176
int original_window_width = ConfMan.getInt("last_window_width", Common::ConfigManager::kApplicationDomain);
int original_window_height = ConfMan.getInt("last_window_height", Common::ConfigManager::kApplicationDomain);
Common::List<Common::String> list = Common::getLanguageList();

const int res[] = {320, 200,
320, 240,
640, 400,
640, 480,
800, 600,
0};

ConfMan.setBool("force_resize", true, Common::ConfigManager::kApplicationDomain);
Common::FSNode dumpDir("snapshots");

if (!dumpDir.isDirectory())
dumpDir.createDirectory();

// Iterate through all languages available
for (Common::String lang : list) {
// Iterating through the resolutions doesn't work so you have to manually change it here.
// TODO: Find a way to iterate through the resolutions using code.
Common::Array<int> res_to_test = {800, 640, 320};
dumpDialogs(message, res_to_test[0], lang);
// Iterate through all resolutions available
for (const int *r = res; *r; r += 2) {
int w = r[0];
int h = r[1];

// Update resolution
ConfMan.setInt("last_window_width", w, Common::ConfigManager::kApplicationDomain);
ConfMan.setInt("last_window_height", h, Common::ConfigManager::kApplicationDomain);
g_system->beginGFXTransaction();
g_system->initSize(w, h);
g_system->endGFXTransaction();

Copy link
Contributor

Choose a reason for hiding this comment

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

The dumpAllDialogs function now iterates through a predefined list of resolutions, updating the configuration manager with the new resolution before each dump. This is a good use of the configuration manager to handle temporary settings. However, there is a potential issue with the use of g_system->beginGFXTransaction() and g_system->endGFXTransaction() without checking the return value of beginGFXTransaction(). It's important to verify that the graphics transaction has started successfully before proceeding with the size initialization. Additionally, the array res is terminated with a 0, which is used as a sentinel value to indicate the end of the array. In C++, it might be more idiomatic to use a std::vector of std::pair or a similar container to represent the resolutions, which would make the iteration more idiomatic and less error-prone since you wouldn't need to rely on a sentinel value and manual pointer arithmetic.

// Replace the C-style array with a std::vector of std::pair
std::vector<std::pair<int, int>> resolutions = {
    {320, 200},
    {320, 240},
    {640, 400},
    {640, 480},
    {800, 600}
};

// Iterate through all resolutions available
for (const auto &resolution : resolutions) {
    int w = resolution.first;
    int h = resolution.second;
    // ... rest of the code ...
}

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 few remarsk

@@ -70,72 +70,65 @@ void handleSimpleDialog(GUI::Dialog &dialog, const Common::String &filename,Grap
dialog.close();
}

void dumpDialogs(const Common::String &message, int res, const Common::String &lang) {
void dumpDialogs(const Common::String &message, int width, int height, const Common::String &lang) {
Copy link
Member

Choose a reason for hiding this comment

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

You do not need width and height here anymore since you're getting those from g_system->getOverlayWidth() and ...Height() respectively.

Graphics::Surface surf;
surf.create(g_system->getOverlayWidth(), g_system->getOverlayHeight(), g_system->getOverlayFormat());

Common::String filename = Common::String::format("%d-%s.png", res, lang.c_str());
Common::String filename = Common::String::format("%d-%d-%s.png", width, height, lang.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

please name it with x between the dimensions, e.g. 320x200-en.png. Also, the name for this variable is not the best. Call it suffix. Also, you could even put the leading - to the suffix, so later you could write "messageDialog" + suffix instead of adding a dash after each dialog name.

800, 600,
0};

ConfMan.setBool("force_resize", true, Common::ConfigManager::kApplicationDomain);
Copy link
Member

Choose a reason for hiding this comment

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

Put a comment above, explaining the purpose

int h = r[1];

// Update resolution
ConfMan.setInt("last_window_width", w, Common::ConfigManager::kApplicationDomain);
Copy link
Member

Choose a reason for hiding this comment

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

Align this vertically, please, e.g. add one more space after width", so w is right above h

g_system->endGFXTransaction();

// Iterate through all langauges
for (Common::String lang : list) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid copying strings and write for (Common::String &lang : list)

@@ -177,6 +190,7 @@ void dumpAllDialogs(const Common::String &message) {
g_system->beginGFXTransaction();
g_system->initSize(original_window_width, original_window_height);
g_system->endGFXTransaction();
ConfMan.removeKey("force_resize", Common::ConfigManager::kApplicationDomain);
Copy link
Member

Choose a reason for hiding this comment

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

Also, please add a comment before this line, explaining its purpose

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 55448f8 and ffbbf23.
Files selected for processing (2)
  • backends/graphics/openglsdl/openglsdl-graphics.cpp (2 hunks)
  • gui/dump-all-dialogs.cpp (3 hunks)
Additional comments: 1
backends/graphics/openglsdl/openglsdl-graphics.cpp (1)
  • 279-284: The initSize function has been updated to handle the "force_resize" configuration setting. When "force_resize" is true, it calls notifyResize with the provided width and height. This change seems to be in line with the pull request's goal of facilitating automatic iteration through predefined resolutions. However, it's important to ensure that this new behavior does not interfere with the normal initialization process when "force_resize" is not set or is false.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 55448f8 and 5ad4574.
Files selected for processing (2)
  • backends/graphics/openglsdl/openglsdl-graphics.cpp (2 hunks)
  • gui/dump-all-dialogs.cpp (3 hunks)
Additional comments: 5
gui/dump-all-dialogs.cpp (4)
  • 198-198: The explanation provided in the comment is clear and helps to understand why "force_resize" is being removed. It's important to note the potential issue if the program crashes before this key is removed, as mentioned. This comment is well-placed and informative.

  • 73-134: The dumpDialogs function has been updated to include the width and height parameters directly from the system's overlay settings, which is a good change for flexibility. However, ensure that all calls to this function are updated accordingly to pass the correct parameters.

  • 148-182: The loop through resolutions and languages is a good implementation for automating the dialog dumping process. However, it's important to ensure that the g_system->beginGFXTransaction() and g_system->endGFXTransaction() calls are properly handling any potential errors and that the system can recover from any issues that might occur during the resizing process. Additionally, consider checking the return value of initSize to ensure that the resize was successful before proceeding.

  • 190-192: Restoring the original window size is a good practice after the dumping process is complete. However, similar to the previous comment, ensure that the transaction is properly handling errors and that the system can recover if initSize fails.

backends/graphics/openglsdl/openglsdl-graphics.cpp (1)
  • 279-284: The changes to initSize function to handle the "force_resize" configuration setting are correct. The function now checks if "force_resize" is enabled in the configuration manager and, if so, calls notifyResize with the provided width and height. This ensures that the window is resized to the specified dimensions when the setting is enabled.

Comment on lines 310 to 327
int currentWidth, currentHeight;
getWindowSizeFromSdl(&currentWidth, &currentHeight);
float dpiScale = _window->getSdlDpiScalingFactor();

if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
currentWidth = width;
currentHeight = height;
}

debug(3, "req: %d x %d cur: %d x %d, scale: %f", width, height, currentWidth, currentHeight, dpiScale);

if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
createOrUpdateWindow(currentWidth, currentHeight, 0);
}

handleResize(currentWidth, currentHeight);

// Remember window size in windowed mode
Copy link
Contributor

Choose a reason for hiding this comment

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

The notifyResize function has been updated to handle the "force_resize" setting. When "force_resize" is true, it sets the current width and height to the values provided by the resize event and then calls createOrUpdateWindow to apply the new dimensions. This ensures that the window is resized to the specified dimensions when the setting is enabled. Additionally, the function saves the new window size in the configuration manager if the window is not in fullscreen mode, which is useful for preserving the window size between sessions.

However, there is a potential issue with the current implementation. The currentWidth and currentHeight variables are being set directly from the width and height parameters without considering the DPI scaling factor. This could lead to incorrect window sizes on high-DPI displays. The DPI scaling should be applied to the width and height before assigning them to currentWidth and currentHeight. Here's a proposed fix:

-		currentWidth = width;
-		currentHeight = height;
+		currentWidth = static_cast<int>(width * dpiScale + 0.5f);
+		currentHeight = static_cast<int>(height * dpiScale + 0.5f);

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
int currentWidth, currentHeight;
getWindowSizeFromSdl(&currentWidth, &currentHeight);
float dpiScale = _window->getSdlDpiScalingFactor();
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
currentWidth = width;
currentHeight = height;
}
debug(3, "req: %d x %d cur: %d x %d, scale: %f", width, height, currentWidth, currentHeight, dpiScale);
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
createOrUpdateWindow(currentWidth, currentHeight, 0);
}
handleResize(currentWidth, currentHeight);
// Remember window size in windowed mode
int currentWidth, currentHeight;
getWindowSizeFromSdl(&currentWidth, &currentHeight);
float dpiScale = _window->getSdlDpiScalingFactor();
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
currentWidth = static_cast<int>(width * dpiScale + 0.5f);
currentHeight = static_cast<int>(height * dpiScale + 0.5f);
}
debug(3, "req: %d x %d cur: %d x %d, scale: %f", width, height, currentWidth, currentHeight, dpiScale);
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
createOrUpdateWindow(currentWidth, currentHeight, 0);
}
handleResize(currentWidth, currentHeight);
// Remember window size in windowed mode

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 5

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 55448f8 and d27567b.
Files selected for processing (2)
  • backends/graphics/openglsdl/openglsdl-graphics.cpp (2 hunks)
  • gui/dump-all-dialogs.cpp (3 hunks)
Additional comments: 4
gui/dump-all-dialogs.cpp (3)
  • 70-70: Closing the dialog after taking a snapshot is a good practice to ensure resources are released properly.

  • 165-182: The nested loops for iterating through resolutions and languages are clear and straightforward. This is a good use of nested loops to cover the permutations of resolutions and languages.

  • 191-193: Restoring the original window size after the dumping process is a good practice to ensure the user's environment is left unchanged.

backends/graphics/openglsdl/openglsdl-graphics.cpp (1)
  • 279-287: The changes here introduce a check for the "force_resize" configuration setting and call notifyResize with the provided width and height if the setting is true. This is part of the new feature to force the window to resize based on configuration settings. The logic seems correct, but it's important to ensure that the notifyResize function is robust and can handle being called at this point in the initialization process.

0};

// "HACK: Pass info to backend to force window resize
ConfMan.setBool("force_resize", true, Common::ConfigManager::kApplicationDomain);
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment added here is good for explaining the purpose of setting "force_resize" to true. However, it would be beneficial to also explain the context in which this configuration is used, such as it being part of an automated process for dumping dialogs at various resolutions.

// HACK: Set 'force_resize' to true to inform the backend to forcibly resize the window.
// This is used in the context of an automated process for capturing GUI dialogs across different resolutions.
ConfMan.setBool("force_resize", true, Common::ConfigManager::kApplicationDomain);


// Clean up the temporary flag.Since we are still within the same method where we added, there is no need to flush config to the disk

ConfMan.removeKey("force_resize", Common::ConfigManager::kApplicationDomain);
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment should be added before this line to explain why "force_resize" is being removed, which is crucial for understanding the lifecycle of this configuration setting and its impact on the system.

// Remove 'force_resize' to prevent unintended forced resizing after the dumping process
ConfMan.removeKey("force_resize", Common::ConfigManager::kApplicationDomain);

Comment on lines 148 to +156
int original_window_width = ConfMan.getInt("last_window_width", Common::ConfigManager::kApplicationDomain);
int original_window_height = ConfMan.getInt("last_window_height", Common::ConfigManager::kApplicationDomain);
Common::List<Common::String> list = Common::getLanguageList();

const int res[] = {320, 200,
320, 240,
640, 400,
640, 480,
800, 600,
0};
Copy link
Contributor

Choose a reason for hiding this comment

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

The array of resolutions is hardcoded, which is fine for a specific set of target resolutions. However, if the list of resolutions might change or grow, consider externalizing this to a configuration file or making it dynamically configurable.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 55448f8 and f01038d.
Files selected for processing (2)
  • backends/graphics/openglsdl/openglsdl-graphics.cpp (2 hunks)
  • gui/dump-all-dialogs.cpp (3 hunks)
Additional comments: 4
backends/graphics/openglsdl/openglsdl-graphics.cpp (1)
  • 279-284: The changes here correctly implement the check for the "force_resize" configuration setting and call notifyResize if it is set to true. This will force the window to resize to the specified dimensions when initializing the size. This is consistent with the pull request's goal to allow forced resizing based on configuration settings.
gui/dump-all-dialogs.cpp (3)
  • 70-134: The changes in this hunk reflect the removal of the int res parameter from the dumpDialogs function and the addition of resolution and language to the filename suffix. This is a logical change that aligns with the new functionality of dumping dialogs at various resolutions and languages. The code appears to be correctly generating the filename suffix based on the current resolution and language settings.

---end hunk 0---

---start hunk 1---

  • 148-185: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [148-198]

The loop through resolutions and languages is a key part of the new functionality. The resolutions are hardcoded in an array, which is fine for a fixed set of target resolutions. However, if flexibility is desired in the future, this could be refactored to read from a configuration file or another external source.

The use of ConfMan to set and reset the last_window_width and last_window_height is appropriate for changing the resolution settings. The force_resize flag is used to communicate with the backend to force a window resize, which is a necessary part of the automated dialog dumping process.

The cleanup of the force_resize flag at the end of the dumpAllDialogs function is important to prevent unintended side effects after the dumping process is complete. The comments added as per previous review comments are helpful for understanding the purpose of these operations.

One thing to note is that the beginGFXTransaction and endGFXTransaction calls are used to bracket the initSize call, which is good practice to ensure that the graphics system is in a consistent state during the resize operation.

Overall, the changes in this hunk are logical and seem to be correctly implemented for the intended functionality.

---end hunk 1---

---start hunk 2---

  • 191-198: This hunk correctly restores the original window dimensions after the dialog dumping process is complete. The beginGFXTransaction and endGFXTransaction calls are used appropriately here as well. The removal of the force_resize flag is crucial to ensure that the system's state is correctly reset after the operation. The comment explaining why there is no need to flush the configuration to disk is informative and justifies the absence of such a call here.

---end hunk 2---

Comment on lines 310 to +317
int currentWidth, currentHeight;
getWindowSizeFromSdl(&currentWidth, &currentHeight);
float dpiScale = _window->getSdlDpiScalingFactor();

if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
currentWidth = width;
currentHeight = height;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous comments about DPI scaling are still valid. The code sets currentWidth and currentHeight directly from the width and height parameters when "force_resize" is true, but it does not consider the DPI scaling factor, which could lead to incorrect window sizes on high-DPI displays. The proposed fix to apply DPI scaling should be implemented to ensure correct behavior across all displays.

-		currentWidth = width;
-		currentHeight = height;
+		currentWidth = static_cast<int>(width * dpiScale + 0.5f);
+		currentHeight = static_cast<int>(height * dpiScale + 0.5f);

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
int currentWidth, currentHeight;
getWindowSizeFromSdl(&currentWidth, &currentHeight);
float dpiScale = _window->getSdlDpiScalingFactor();
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
currentWidth = width;
currentHeight = height;
}
int currentWidth, currentHeight;
getWindowSizeFromSdl(&currentWidth, &currentHeight);
float dpiScale = _window->getSdlDpiScalingFactor();
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
currentWidth = static_cast<int>(width * dpiScale + 0.5f);
currentHeight = static_cast<int>(height * dpiScale + 0.5f);
}

Comment on lines 319 to 327
debug(3, "req: %d x %d cur: %d x %d, scale: %f", width, height, currentWidth, currentHeight, dpiScale);

if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
createOrUpdateWindow(currentWidth, currentHeight, 0);
}

handleResize(currentWidth, currentHeight);

// Remember window size in windowed mode
Copy link
Contributor

Choose a reason for hiding this comment

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

The debug statement and the subsequent logic appear to be correctly placed to handle the forced resize. The createOrUpdateWindow call inside the conditional block ensures that the window is updated with the new dimensions. The handleResize call is used to update the internal state of the graphics manager. The code to remember the window size in windowed mode is outside the conditional block, which is appropriate as it should only execute when not in fullscreen mode. However, the DPI scaling factor should be applied to currentWidth and currentHeight before they are used to update the configuration manager's settings, as per the previous fix.

	if (!_wantsFullScreen) {
-		currentWidth = (int)(currentWidth / dpiScale + 0.5f);
-		currentHeight = (int)(currentHeight / dpiScale + 0.5f);
+		// DPI scaling is already applied if force_resize is true, so no need to apply it again here.
+		if (!ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
+			currentWidth = (int)(currentWidth / dpiScale + 0.5f);
+			currentHeight = (int)(currentHeight / dpiScale + 0.5f);
+		}
		// ... (rest of the code remains unchanged)
	}

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
debug(3, "req: %d x %d cur: %d x %d, scale: %f", width, height, currentWidth, currentHeight, dpiScale);
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
createOrUpdateWindow(currentWidth, currentHeight, 0);
}
handleResize(currentWidth, currentHeight);
// Remember window size in windowed mode
debug(3, "req: %d x %d cur: %d x %d, scale: %f", width, height, currentWidth, currentHeight, dpiScale);
if (ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
createOrUpdateWindow(currentWidth, currentHeight, 0);
}
handleResize(currentWidth, currentHeight);
if (!_wantsFullScreen) {
// DPI scaling is already applied if force_resize is true, so no need to apply it again here.
if (!ConfMan.getBool("force_resize", Common::ConfigManager::kApplicationDomain)) {
currentWidth = (int)(currentWidth / dpiScale + 0.5f);
currentHeight = (int)(currentHeight / dpiScale + 0.5f);
}
// ... (rest of the code remains unchanged)
}
// Remember window size in windowed mode

@sev-
Copy link
Member

sev- commented Nov 17, 2023

Thanks!

@sev- sev- merged commit 5ce451f into scummvm:master Nov 17, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants