Skip to content

X11: Fix panic when no monitors are available#1158

Merged
Osspial merged 7 commits intorust-windowing:masterfrom
murarth:fix-no-monitor
Sep 23, 2019
Merged

X11: Fix panic when no monitors are available#1158
Osspial merged 7 commits intorust-windowing:masterfrom
murarth:fix-no-monitor

Conversation

@murarth
Copy link
Copy Markdown
Contributor

@murarth murarth commented Sep 11, 2019

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@murarth
Copy link
Copy Markdown
Contributor Author

murarth commented Sep 11, 2019

Fixes #705 by returning a dummy MonitorHandle when no monitors are available. This seemed like the most straightforward fix, as the platform-independent facade would not permit returning an Option<MonitorHandle> from Window::current_monitor. The dummy MonitorHandle has an id value of 0 (an invalid XID value) and a few checks are added to avoid caching this dummy handle or using it as though it were valid.

I was unable to reproduce the original issue myself, so I hope that someone still experiencing the issue will test this and let me know if any new bugs occur as a result of this change.

@elinorbgr
Copy link
Copy Markdown
Contributor

Overall, what would happen if a user unknowingly tries to use this dummy monitor handle as it were the main monitor for their app, by passing it to set_fullscreen, or by calling the methods of MonitorHandle to get its name / position / dpi factor?

@murarth
Copy link
Copy Markdown
Contributor Author

murarth commented Sep 12, 2019

@vberger There's an explicit check for the dummy monitor's ID value in Window::set_fullscreen. If the user requests fullscreen on the dummy monitor, no action is taken.

These are the dummy monitor's fields:

MonitorHandle {
id: 0,
name: "<dummy monitor>".into(),
hidpi_factor: 1.0,
dimensions: (0, 0),
position: (0, 0),
primary: true,
rect: util::AaRect::new((0, 0), (0, 0)),
video_modes: Vec::new(),
}

The size of the monitor is zero. I suppose this may cause an application to divide by zero if, e.g., trying to center the window on the current monitor. I could instead give the dummy monitor a size of (1, 1).

@Osspial
Copy link
Copy Markdown
Contributor

Osspial commented Sep 12, 2019

cc @jarredbarber (of #1007) and @chutz (of #705). Does this branch fix the issues you reported?

@murarth
Copy link
Copy Markdown
Contributor Author

murarth commented Sep 17, 2019

Replaced a return Err with a default DPI factor of 1.0 to avoid another panic. This should address #1007.

@goddessfreya
Copy link
Copy Markdown
Contributor

Should be good to merge when CI is done?

@murarth
Copy link
Copy Markdown
Contributor Author

murarth commented Sep 23, 2019

@zegentzy Yes, I think so.

@Osspial Osspial merged commit 472eddc into rust-windowing:master Sep 23, 2019
@murarth murarth deleted the fix-no-monitor branch September 23, 2019 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants