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 to _desktopwidth, _desktopheight, _screenicon #60

Merged
merged 4 commits into from
May 17, 2022
Merged

Fix to _desktopwidth, _desktopheight, _screenicon #60

merged 4 commits into from
May 17, 2022

Conversation

SteveMcNeill
Copy link
Member

Fixes the issue as brought up on the forums here: https://qb64phoenix.com/forum/showthread.php?tid=408

Also added a small set of logic so we don't end up inside an endless loop if the screen is hidden (via _SCREENHIDE), or if it doesn't exist for whatever reason.

Fixes the issue as brought up on the forums here: https://qb64phoenix.com/forum/showthread.php?tid=408

Also added a small set of logic so we don't end up inside an endless loop if the screen is hidden (via _SCREENHIDE), or if it doesn't exist for whatever reason.
@SteveMcNeill SteveMcNeill added the bug Something isn't working label May 16, 2022
@SteveMcNeill SteveMcNeill requested review from mkilgore and a user May 16, 2022 11:57
while (!window_exists) {
Sleep(100);
}
//while (!window_exists) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't leave commented code in the file :) If someone wants to see what was there before they can check the git history.


int32 waited_enough = 20;

while (!window_handle&&waited_enough>0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right that glutGet() for these parameters doesn't require an open window. We should see if we can add some test cases for that, but either way I'd rather we just drop the check instead of add a timeout.

}
# endif
if (waited_enough == 0)
return; // if the screen isn't created and doesn't have a handle after two seconds, then just return and don't try to minimize the window.
glutIconifyWindow();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the actual underlying problem is that we're trying to call glutIconifyWindow() from outside the main thread using GLUT. Google indicates that FreeGLUT itself is not thread safe, and a quick look at the source code seems to back that up. So with that being the case, we cannot call glutIconifyWindow() here anyway, it doesn't matter if the window is setup or not. It might work, but it could easily have problems if the thread doing the GLUT work happens to be doing the wrong thing at the time.

The proper way to solve this problem would be to set some kind of flag or otherwise notify the thread GLUT is being used on to call glutIconifyWindow() itself. That has the added benefit that notifying the other thread does not require us to wait for the GLUT thread to do the work, so the busy waiting problem being addressed here also goes away. Notifying the thread can also be done regardless of the current state of GLUT itself and whether a window has already been opened or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created #66 for this.

@mkilgore mkilgore added this to Review in progress in Backlog/In Progress May 16, 2022
Having windows call GetSystemMetrics without relying on glutGet, gets rid of the seg fault that can occur at program start up.  screenicon was restored to it's previous state so that larger issues with it can be addressed at a future date.
return glutGet(GLUT_SCREEN_WIDTH);
#else
# ifdef QB64_WINDOWS
#ifdef QB64_WINDOWS
return GetSystemMetrics(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a small thing, but there are constants you're intended to use for GetSystemMetrics() rather than hard-coded values. You should try swapping 0 for SM_CXSCREEN and 1 for SM_CYSCREEN, it makes it much easier to know what's going on. Hopefully those constants are already available in this file via the #includes, if they can't be found then I'm fine leaving it as-is :)

Copy link
Contributor

@mkilgore mkilgore left a comment

Choose a reason for hiding this comment

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

I'd like you to give the SM_CXSCREEN and SM_CYSCREEN thing a try if you don't mind, but otherwise looks good to me :)

@SteveMcNeill SteveMcNeill merged commit b3e97de into QB64-Phoenix-Edition:main May 17, 2022
Backlog/In Progress automation moved this from Review in progress to Done May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants