[X11, SDL] Get Display Position/Size of One Monitor (Done Right!)

Reporter: time-killer-games  |  Status: closed  |  Last Modified: February 16, 2020, 04:26:55 PM
fundies  
its crashing in the ci cause you're using stoi invalidly. but aside from that I doubt we want to call xrandr & awk to get this info
codecov[bot]  

Codecov Report

Merging #1886 into master will increase coverage by 0.24%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1886      +/-   ##
==========================================
+ Coverage   25.16%   25.41%   +0.24%     
==========================================
  Files         177      178       +1     
  Lines       16860    16918      +58     
==========================================
+ Hits         4243     4299      +56     
- Misses      12617    12619       +2
Impacted Files Coverage Δ
ENIGMAsystem/SHELL/Platforms/General/PFwindow.h 100% <ø> (ø) ⬆️
ENIGMAsystem/SHELL/Platforms/General/PFwindow.cpp 36.84% <100%> (ø) ⬆️
ENIGMAsystem/SHELL/Platforms/xlib/XLIBmain.cpp 37.03% <100%> (+2.42%) ⬆️
...GMAsystem/SHELL/Platforms/xlib/XDisplayGetters.cpp 96.29% <96.29%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87aac66...a35b49e. Read the comment docs.

JoshDreamland  

To expound,

  1. The code in this change is very clean. It's a marked improvement in terms of readability. You used a helper to avoid repeating yourself, and it's crystal clear what you're trying to get from randr and how you're handling that result.
  2. It's not clear that calling randr is what we want to do, although your code provides a backup strategy, so it's not exactly harmful (if comparatively slow). We can't rely on the xrandr utility being available on host systems. It's not installed by default on non-Debian, as far as I can tell, so it's not the silver bullet you think it is.

I haven't found an explanation for when we can expect X11 extensions to be installed, but if the real feature we want is the DE's choice of primary display, we might opt to prefer the current display instead. If X11 doesn't provide a utility for that, it's normally just the display that has the mouse on it.

time-killer-games  

@JoshDreamland Good news and bad news. The good news is, I am no longer relying on the cli to do this and I'm only using C++. The bad news, this runs almost exactly just as slow as the cli using native code, so I had to add back the static caching to prevent this, at the same price to pay.
time-killer-games  

@JoshDreamland Added SDL support and placeholders for Windows support. SDL works differently than X11. X11 measures the position/size of the primary monitor. SDL only has a way to measure the monitor which has the center of the window on it, according to the docs. Also this is ready for review.
time-killer-games  

@JoshDreamland now passing and ready to be merged.
Please sign in to post comments, or you can view this issue on GitHub.