Dual monitor fix #1238

bloodsword · 2017-06-05T11:04:48Z

SFML Tasks

Test and confirm fix by multiple people/distros/window managers

This should fix #1226.

XRRSetCrtcConfig is now used to set video mode instead of XRRSetScreenConfig.

MarioLiebisch · 2017-06-05T11:16:24Z

Ah, very cool. Found a similar solution but couldn't be bothered trying to figure out the correct types etc. :)

mantognini

Just a quick note on style: indentation is broken in a few places, there are extra spaces at the end of lines and some parenthesis are missing (see here). Otherwise, I can't judge much the patch as I can't test it. I'll trust @MarioLiebisch for that.

That being said, thanks for the PR! If you manage to fix the issues and squash all that in a nice commit that'd be awesome! :-)

Commit says "Mouse still can't be used in the second screen.". Do you have some ideas to fix that as well?

mantognini · 2017-06-05T12:01:31Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    // Check XRandR version, 1.2 required
+    if(!XRRQueryVersion(m_display, &xRandRMajor, &xRandRMinor) || xRandRMajor < 1 || xRandRMinor < 2 )
+    {
+        err() << "XRandR is to old" << std::endl;


s/to/too/

Check will fail once v.2 is release (although it's not for tomorrow).

MarioLiebisch · 2017-06-05T12:15:36Z

I know it's buggy in its current form and I think this is the right way to go, but I don't have a multi monitor setup to actually test this. However, running a program in fullscreen typically means you can't interact with anything on the second screen/desktop since it has exclusive access. At least that's how it's working on Windows. You can't force the ability to continue working on a secondary display.

eXpl0it3r · 2017-06-05T12:37:14Z

Can this be tested in a VM?

MarioLiebisch · 2017-06-05T12:52:58Z

In theory, but never got multiple screens to work properly. I can set two displays, but "Displays" still only lists a single one.

bloodsword · 2017-06-05T22:49:22Z

I changed XRandr check to accept a potential v2.
I fixed code style and squashed my commits.

The message "Mouse still can't be used in the second screen." was inaccurate, actually it's possible but you have to alt+tab.

mantognini

Thanks for the update. I think X11 is tricky enough to be a bit picky about the implementation in order to make it easily maintainable. :-)

mantognini · 2017-06-10T13:13:42Z

src/SFML/Window/Unix/WindowImplX11.cpp

@@ -56,7 +56,7 @@
    #include <SFML/Window/Unix/GlxContext.hpp>
    typedef sf::priv::GlxContext ContextType;
 #endif
-
+    


extra spaces

mantognini · 2017-06-10T13:14:38Z

src/SFML/Window/Unix/WindowImplX11.cpp

+        output = XRRGetOutputPrimary(m_display, rootWindow);
+
+        // Check if output return is valid, otherwise use the first screen
+        if(output == None)


missing space after if

mantognini · 2017-06-10T13:15:59Z

src/SFML/Window/Unix/WindowImplX11.cpp

-    m_oldVideoMode = XRRConfigCurrentConfiguration(config, &currentRotation);
+    // If version > 1.2 get the primary screen else take the first screen
+    RROutput output;
+    if ((xRandRMajor == 1 && xRandRMinor >= 3) || xRandRMajor > 1)


If we want to follow strictly our style rules, more parentheses are needed. But I'd argue it's more readable as is. Do others agree? (It's also the case in a few other places.)

mantognini · 2017-06-10T13:16:34Z

src/SFML/Window/Unix/WindowImplX11.cpp

+        XRRFreeScreenResources(res);
+
+        // If outputInfo->connection == RR_Disconnected, free output info
+        if(!outputInfo)


missing space

mantognini · 2017-06-10T13:21:12Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    // Free  output info
+    XRRFreeOutputInfo(outputInfo);
+
+    // Free crtc info


I think the // Free <stuff> comment should be removed -- they don't bring much.

mantognini · 2017-06-10T13:23:11Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    }
+
+    // Check XRandR version, 1.2 required
+    if (!XRRQueryVersion(m_display, &xRandRMajor, &xRandRMinor) || xRandRMajor < 1 || (xRandRMajor == 1 && xRandRMinor < 2 ))


mantognini · 2017-06-10T13:23:33Z

src/SFML/Window/Unix/WindowImplX11.cpp

 } // namespace priv

-} // namespace sf
+}// namespace sf


space was removed.

mantognini · 2017-06-10T13:30:27Z

src/SFML/Window/Unix/WindowImplX11.cpp


-            // Set "this" as the current fullscreen window
-            fullscreenWindow = this;
+    for (int i = 0; i < res->nmode; i++)


I know it was like that before, but it would be much more elegant IMO to have i < res->nmode && !modeFound as a loop condition and remove the break statement, wouldn't it?

mantognini · 2017-06-10T13:32:22Z

src/SFML/Window/Unix/WindowImplX11.cpp

+        return;
+    }
+
+    // Get crtc info from crtc


X11 & cie being far from trivial, I'd add in the comment what kind of concrete info we're trying to get from CRTC. (screen dimension and rotation?) Otherwise the comment seems of no additional interest than the next function call.

mantognini · 2017-06-10T13:33:50Z

src/SFML/Window/Unix/WindowImplX11.hpp

+    /// \return True if a valid XRandR version found, false otherwise
+    ///
+    ////////////////////////////////////////////////////////////
+    bool checkXRandR(int &xRandRMajor, int &xRandRMinor);


Should be int& x and not int &x.

bloodsword · 2017-06-12T20:57:50Z

I pushed a new revision.
Sorry for code style. I hope I did not forget anything :)

mantognini

Thanks for the update!

Regarding XRRGetOutputPrimary, looking at https://cgit.freedesktop.org/xorg/lib/libXrandr/refs/tags it seems the function is first tagged in 1.3. It should simply fail to compile with older version, unless I missed something, so we could remove the runtime check altogether.

If, for some X11 specific reason I'm not aware of, the runtime check is best/okay, then I've no complain. Otherwise, it would be actually a good news as the code would be simpler: we just need a version requirement in CMake and that's it. (v1.3 is 8 years old so it should not be an issue to assume it's available on SFML supported system, hence no need to have conditional code for backward compatibility IMO.)

mantognini · 2017-06-13T07:10:17Z

src/SFML/Window/Unix/WindowImplX11.cpp

-    // Get the available screen sizes
-    int nbSizes;
-    XRRScreenSize* sizes = XRRConfigSizes(config, &nbSizes);
+    // If version > 1.2 get the primary screen else take the first screen


minor: should be >=

It's actually version > 1.2, but maybe >= 1.3 is more readable.

mantognini · 2017-06-13T07:24:42Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    RROutput output;
+    if ((xRandRMajor == 1 && xRandRMinor >= 3) || xRandRMajor > 1)
+    {
+        output = XRRGetOutputPrimary(m_display, rootWindow);


I'm actually wondering: is this function define in xrandr 1.2? Isn't it instead defined in 1.3+?

bloodsword · 2017-06-13T19:20:43Z

You are absolutely right about XRRGetOutputPrimary.
We can do a compile time check, then no additional version requirement will be needed in CMake.
As you said 1.3 is quite old now, so it's not really a big deal, but it does not require a lot of changes to do a compile time check, so I pushed a new revision.

mantognini

It would be interesting to have feedback from others at this point I think. Some things are beyond my knowledge. @bloodsword, it goes without saying that your point of view is also welcome on those issues!

mantognini · 2017-06-15T07:31:36Z

src/SFML/Window/Unix/WindowImplX11.cpp

-    if (!XQueryExtension(m_display, "RANDR", &version, &version, &version))
+    // Check if the XRandR extension
+    int xRandRMajor, xRandRMinor;
+    if (!checkXRandR(xRandRMajor, xRandRMinor))


If we don't care about the actual version, but just the presence of the extension, then I'd remove the two parameters of checkXRandR. Having the function is fine, but the simpler it is the better.

I'm not enough familiar with X11 & cie, but do we need to actually check for the extension? Or can't we assume that if we were able to link against xrandr the extension will be available at runtime? @binary1248, since you edited those line before, could you clarify that for me. Thanks. :-)

As strange as it sounds, I've experienced implementations where not calling XQueryExtension would cause the extension to fail even if it should be supported. It seemed like calling XQueryExtension would load the extension or something. So even if it should be supported, performing the runtime "check" is still the safer variant.

mantognini · 2017-06-15T07:32:22Z

src/SFML/Window/Unix/WindowImplX11.cpp

-    // Check if the XRandR extension is present
-    int version;
-    if (!XQueryExtension(m_display, "RANDR", &version, &version, &version))
+    // Check if the XRandR extension


The comment was truncated, leaving out the end of the sentence.

mantognini · 2017-06-15T07:39:50Z

src/SFML/Window/Unix/WindowImplX11.cpp

-    int nbSizes;
-    XRRScreenSize* sizes = XRRConfigSizes(config, &nbSizes);
+    // Compile time check : if version >= 1.3 get the primary screen else take the first screen
+    #if (RANDR_MAJOR == 1 && RANDR_MINOR >= 3) || RANDR_MAJOR > 1


The question I'm asking myself is, do really want to support older versions? If not, it would make the code much simpler without those conditional compilation macros. What do @binary1248, @Bromeon and @MarioLiebisch (or anyone who's familiar with Linux actually) think?

I would prefer runtime version checking. It is done in the above code already anyway, and X on Linux doesn't exactly make it trivial to detect library versions during compile time. X works based on Client-Server so it might even be possible that the header you compile against on the system doesn't match what the X server is actually running.

binary1248 · 2017-07-22T14:09:02Z

src/SFML/Window/Unix/WindowImplX11.cpp

-    if (!XQueryExtension(m_display, "RANDR", &version, &version, &version))
+    // Check if the XRandR extension
+    int xRandRMajor, xRandRMinor;
+    if (!checkXRandR(xRandRMajor, xRandRMinor))


As strange as it sounds, I've experienced implementations where not calling XQueryExtension would cause the extension to fail even if it should be supported. It seemed like calling XQueryExtension would load the extension or something. So even if it should be supported, performing the runtime "check" is still the safer variant.

binary1248 · 2017-07-22T14:09:50Z

src/SFML/Window/Unix/WindowImplX11.cpp

    {
-        // XRandR extension is not supported: we cannot use fullscreen mode


There is no reason to remove this comment.

binary1248 · 2017-07-22T14:15:47Z

src/SFML/Window/Unix/WindowImplX11.cpp

-    int nbSizes;
-    XRRScreenSize* sizes = XRRConfigSizes(config, &nbSizes);
+    // Compile time check : if version >= 1.3 get the primary screen else take the first screen
+    #if (RANDR_MAJOR == 1 && RANDR_MINOR >= 3) || RANDR_MAJOR > 1


I would prefer runtime version checking. It is done in the above code already anyway, and X on Linux doesn't exactly make it trivial to detect library versions during compile time. X works based on Client-Server so it might even be possible that the header you compile against on the system doesn't match what the X server is actually running.

binary1248 · 2017-07-22T14:22:48Z

src/SFML/Window/Unix/WindowImplX11.cpp


-        if ((sizes[i].width  == static_cast<int>(mode.width)) && (sizes[i].height == static_cast<int>(mode.height)))
+        // If outputInfo->connection == RR_Disconnected, free output info
+        if (!outputInfo)


This should probably be outputInfo and single line if blocks omit brackets.

binary1248 · 2017-07-22T14:24:53Z

src/SFML/Window/Unix/WindowImplX11.cpp

+    if (!modeFound)
+    {
+        err() << "Failed to find a matching RRMode for fullscreen mode, switching to window mode" << std::endl;
+        return;


Resources not freed here.

eXpl0it3r · 2017-08-01T15:15:59Z

@bloodsword Can you follow up on @binary1248's review?

bloodsword · 2017-08-01T20:14:39Z

@eXpl0it3r yes. I will try to push something this week.

eliasdaler · 2018-03-29T21:08:04Z

Can someone provide a sample code for testing and what the expected/buggy behaviour looks like?
I'll be happy to test it out on my dual-monitor Linux setup. :)

bloodsword · 2018-03-29T22:31:51Z

I don't know what the buggy behaviour looks like since my last commit.
Prior my last commit, @binary1248 describes some problems, it should be fixed, but except my own test I have no feedback.
@eXpl0it3r reports that some users had problems, but I have no more details.
If you manage to produce a bug with my pull request version (https://github.com/bloodsword/SFML/tree/dual-monitor-fix),
please tell me what is your setup, maybe I will manage to reproduce it and to fix it.

Here is a sample code to test (press escape to quit) :

#include <SFML/Graphics.hpp>

int main()
{  
    sf::RenderWindow window( sf::VideoMode::getDesktopMode(), "SFML test", sf::Style::Fullscreen);

    while (window.isOpen())
    {        
        sf::Event event;
        while (window.pollEvent(event))
        {  
            if ((event.type == sf::Event::Closed) || 
               ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Escape)))
            {
                window.close();
                break;
            }
        } 
        window.clear();
        window.display();
        sf::sleep(sf::milliseconds(16));
    }

    return EXIT_SUCCESS;
}

The normal behaviour should be a black fullscreen on one of your screen, your other screen should stay active.
Mouse cursor should be locked in the fullscreen monitor except if you alt+tab.

binary1248 · 2018-04-09T00:32:13Z

I attribute the test failure during my last test in part to my "exotic" screen setup. I can't test with 2 screens of the same resolution because... well... I simply don't have 2 screens with the same resolution. If others who can test using screens with the same resolution report that this is still an improvement over the state without it, I am willing to accept it as a partial solution on the way to something more complete.

bloodsword · 2018-04-09T19:34:20Z

I have no problem with same resolution screens (1280x1024 x 2),
and since my last commit (01 November 2017) I have no problem with different screens either(1680x1050 + 1280x1024).
@binary1248 did you try the latest commit ? If you did, any improvement since your 16 October review ?

danieljpetersen · 2018-04-09T20:14:03Z

edit: Apologies. See the next post.

danieljpetersen · 2018-04-09T20:36:29Z

Actually, I take all of that back. I did a double check and the damn thing was still linking to the current version of master. e18b9d2 behaves correctly for me (1920x1080 dual monitor setup).

bloodsword · 2018-04-09T20:58:00Z

No problem. When I booted my Mint trying to reproduce the behaviour you describe I did the same mistake :)
Thanks for your feedback 👍

fahlmant · 2018-08-17T14:31:11Z

EDIT: I misunderstood the intention of this PR. Works for me, Fedora 28, KDE. +1

eliasdaler · 2018-08-17T21:19:33Z

Tested on Ubuntu 18.10, NVIDIA driver 396.51. Both "master" and this branch work as expected for me...

One monitor is 1366x768. The other is 1920x1080. The fullscreen shows up on the primary monitor.

Based on an updated diff

bloodsword · 2018-08-31T20:02:34Z

Nice to read your feedbacks.
Tell me if you need me to rebase the PR on master.

eXpl0it3r · 2018-09-08T17:03:22Z

Merged in 46ce05c

MarioLiebisch added bug m:sfml-window p:linux s:accepted labels Jun 5, 2017

mantognini requested changes Jun 5, 2017

View reviewed changes

eXpl0it3r added this to the 2.5 milestone Jun 5, 2017

eXpl0it3r added this to Discussion in SFML 2.5.0 Jun 5, 2017

eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.5.0 Jun 5, 2017

bloodsword force-pushed the dual-monitor-fix branch from e10a5c3 to 4d18bc5 Compare June 5, 2017 22:24

mantognini requested changes Jun 10, 2017

View reviewed changes

bloodsword force-pushed the dual-monitor-fix branch from b635dce to 24cf017 Compare June 12, 2017 20:38

mantognini reviewed Jun 13, 2017

View reviewed changes

bloodsword force-pushed the dual-monitor-fix branch 2 times, most recently from bed993d to 03610d4 Compare June 13, 2017 19:18

mantognini reviewed Jun 15, 2017

View reviewed changes

mantognini added s:undecided and removed s:accepted labels Jul 1, 2017

eXpl0it3r requested a review from binary1248 July 18, 2017 10:06

binary1248 requested changes Jul 22, 2017

View reviewed changes

eXpl0it3r moved this from Review & Testing to Requires Adjustments in SFML 2.5.0 Aug 1, 2017

bloodsword force-pushed the dual-monitor-fix branch from 8ec0b62 to bd631d8 Compare August 1, 2017 21:26

eXpl0it3r modified the milestones: 2.5, 2.6 Mar 23, 2018

eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Aug 13, 2018

fahlmant approved these changes Aug 17, 2018

View reviewed changes

eXpl0it3r added s:accepted and removed s:undecided labels Aug 17, 2018

eXpl0it3r added this to Discussion in SFML 2.5.1 via automation Aug 20, 2018

eXpl0it3r removed this from Review & Testing in SFML 2.6.0 Aug 20, 2018

eXpl0it3r modified the milestones: 2.6, 2.5.1 Aug 20, 2018

eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.5.1 Aug 30, 2018

eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.1 Sep 8, 2018

eXpl0it3r approved these changes Sep 8, 2018

View reviewed changes

eXpl0it3r closed this Sep 8, 2018

SFML 2.5.1 automation moved this from Ready to Done Sep 8, 2018

eXpl0it3r mentioned this pull request Sep 8, 2018

Fullscreen runs on second monitor and closes the main one in dual monitor setup, Linux #1226

Closed

SFML deleted a comment from eXpl0it3r Dec 31, 2022

		{
		// XRandR extension is not supported: we cannot use fullscreen mode

Dual monitor fix #1238

Dual monitor fix #1238

Conversation

bloodsword commented Jun 5, 2017 • edited

MarioLiebisch commented Jun 5, 2017

mantognini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarioLiebisch commented Jun 5, 2017

eXpl0it3r commented Jun 5, 2017

MarioLiebisch commented Jun 5, 2017

bloodsword commented Jun 5, 2017

mantognini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bloodsword commented Jun 12, 2017

mantognini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bloodsword commented Jun 13, 2017

mantognini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eXpl0it3r commented Aug 1, 2017

bloodsword commented Aug 1, 2017

eliasdaler commented Mar 29, 2018

bloodsword commented Mar 29, 2018

binary1248 commented Apr 9, 2018

bloodsword commented Apr 9, 2018

danieljpetersen commented Apr 9, 2018 • edited

danieljpetersen commented Apr 9, 2018 • edited

bloodsword commented Apr 9, 2018

fahlmant commented Aug 17, 2018 • edited

eliasdaler commented Aug 17, 2018

bloodsword commented Aug 31, 2018

eXpl0it3r commented Sep 8, 2018

bloodsword commented Jun 5, 2017 •

edited

danieljpetersen commented Apr 9, 2018 •

edited

danieljpetersen commented Apr 9, 2018 •

edited

fahlmant commented Aug 17, 2018 •

edited