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

Fix setting window size oddities #993

Merged
merged 1 commit into from Mar 13, 2019
Merged

Fix setting window size oddities #993

merged 1 commit into from Mar 13, 2019

Conversation

MortimerGoro
Copy link
Contributor

Fixes #990

@ghost ghost assigned MortimerGoro Mar 8, 2019
@ghost ghost added the in progress label Mar 8, 2019
Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

I still think we need to rethink the display and max display settings.

Copy link
Contributor

@philip-lamb philip-lamb left a comment

Choose a reason for hiding this comment

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

This fixes the incorrect size when a width of 800 is set, but not that changing the width still doesn't fix the actual size of the web content pane; it only changes the height (so that the aspect ratio is correct).

@bluemarvin
Copy link
Contributor

I think there is some confusion over what the window size field represent. This is actually the default window size. So when the user clicks the 1x button on the resize button bar, this is the size the window should return to.

@MortimerGoro
Copy link
Contributor Author

I agree that it's confusing. I understand it as the window pixel size for 1x world size: similar to a zooming feature, show more or less pixels in the same world meters size.

Should we just remove these settings? What's the use case to have them?

@bluemarvin
Copy link
Contributor

I think it should at least be renamed. I don't see anything wrong with allowing users to set the default size of a window.

@philip-lamb
Copy link
Contributor

OK, let's get this change in first and we can consider labelling change separately.

@MortimerGoro MortimerGoro merged commit 7ec6316 into master Mar 13, 2019
@ghost ghost removed the in progress label Mar 13, 2019
@bluemarvin bluemarvin deleted the edit_setting_bug branch March 13, 2019 17:11
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.

None yet

3 participants