Multi monitor support

[Foaly]

OK I am not sure if I understand it correctly. Do you mean I should have the members (name, bounds, etc.) public and non-const and have Screen::get() return a const reference? Are the member of a object obtain by get not changeable in this case?

[Laurent]

Do you mean I should have the members (name, bounds, etc.) public and non-const and have Screen::get() return a const reference?

Yes. Or a copy, it doesn't really matter.

Are the member of a object obtain by get not changeable in this case?

You can't modify a const [reference/pointer to an] object, that's what this keyword is made for

[Foaly]

Thanks for the clarification! I sometimes get a little confused with the const rules
Just to be sure here: you also think that public members instead of getters are the better design, right? If so I'll change that tonight.

[Laurent]

you also think that public members instead of getters are the better design, right?

Yes.

[Foaly]

Alright! I implemented the change. The new commit can be found here.

Updated testing code:

(click to show/hide)

#include <SFML/Window.hpp>
#include <iostream>

int main()
{
    // query screen count
    std::size_t totalMonitors = sf::Screen::count();
    std::cout << "# of connected Screens: " << totalMonitors << std::endl;

    // enumerate over all screens
    for(std::size_t id = 0; id < totalMonitors; ++id)
    {
        sf::Screen screen = sf::Screen::get(id);
        const std::vector<sf::VideoMode> modes = screen.fullscreenModes;
        std::cout << "Valid resolutions for screen " << modes[0].screenId << std::endl << std::endl;

        // fullscreen modes
        for(std::size_t i = 0; i < modes.size(); ++i)
        {
            sf::VideoMode mode = modes[i];
            std::cout << "Mode #" << i << ": "
            << mode.width << "x" << mode.height << " - "
            << mode.bitsPerPixel << " bpp" << std::endl;
        }
        std::cout << std::endl;

        // desktop mode
        sf::VideoMode mode = screen.desktopMode;
        std::cout << "Desktop mode for screen #" << id << ": "
        << mode.width << "x" << mode.height << " - "
        << mode.bitsPerPixel << " bpp" << std::endl << std::endl;

        // screen information
        const sf::IntRect bounds = screen.bounds;
        const sf::IntRect workingArea = screen.workingArea;
        std::wcout << "Monitor " << screen.id << " name: " << screen.name << std::endl
        << "x: " << bounds.left << " y: " << bounds.top << " width: " << bounds.width << " height: " << bounds.height << std::endl
        << "x: " << workingArea.left << " y: " << workingArea.top << " width: " << workingArea.width << " height: " << workingArea.height << std::endl
        << "refreshRate: " << screen.refreshRate << "Hz"
        << " dpi: " << screen.dpi.x << "x" << screen.dpi.y << std::endl << std::endl << std::endl;
    }

    // backward compatibility with depreaction note
    sf::VideoMode::getDesktopMode();

    // open a fullscreen window on the second screen
    sf::VideoMode videoMode(1024, 768, 32, 1);
    sf::Window window(videoMode, "Multi Monitor Example!" , sf::Style::Fullscreen);
    window.setFramerateLimit(sf::Screen::get(videoMode.screenId).refreshRate);

    // run the program as long as the window is open
    while (window.isOpen()) {
        // check all the windows events
        sf::Event event;
        while (window.pollEvent(event)) {
            // "close requested" event: we close the window
            if (event.type == sf::Event::Closed)
                window.close();

            else if (event.type == sf::Event::KeyReleased) {
                // close if escape key was pressed
                if (event.key.code == sf::Keyboard::Escape) {
                    window.close();
                }
                if (event.key.code == sf::Keyboard::Return) {
                    window.create(videoMode, "Multi Monitor Example", sf::Style::Resize);
                    window.setFramerateLimit(sf::Screen::get(videoMode.screenId).refreshRate);
                }
            }
        }

        // display the windows content
        window.display();
    }

    return 0;
}

It would be cool if someone with win7 or older could test this (even with 1 monitor) to see if the dpi is working correctly. Maybe also someone with a high dpi mointor. As always I am open for feedback

[Nexus]

First, thanks a lot for your effort on suggesting a new API and writing the code. I like the overall approach, but I have some comments regarding your implementation:

Please keep the deprecation of existing features separate from your feature's pull request. We haven't fully decided what approach to choose yet, as can be seen from this discussion. I'll try to come up with some actual code on the weekend, as there are meanwhile quite a few features requiring deprecation. If people agree, you could then base your code on that

If you really need to move <SFML/Graphics/Rect.hpp> to <SFML/System/Rect.hpp>, make sure Git recognizes it as a rename and not add+delete, so that the history of the file is preserved. And do it in a separate commit, too.

#warning is not a portable preprocessor directive.

Furthermore, a public constructor with 9 parameters is not useful. It's too long, and calls don't look expressive at all, as the arguments are not named. Why is it even public? I think users are supposed to access sf::Screen instances only via static get() method?

The name Screen::count() is not consistent with other SFML functions returning an array size.

What is the "Id" (should be "ID" in the doc, "Id" in code is fine) of the screen? The documentation should make this more clear. Are the integers continuous, starting from 0?

Top-level const qualifiers in return types are useless:

const std::wstring getDisplayDeviceFromId(unsigned int screenId);

[Laurent]

If IDs are actually indices, I think they should be named "index", this makes it clearer that they are in range [0 .. count()[ and not some random unique identifiers.

Why std::wstring? It's the least defined/portable/usable thing in the C++ world, and we have sf::String.

[Foaly]

Thanks for the review and the feedback! It's nice to hear my work is appreciated

I must have missed that there was a separate discussion for that. I left a post with my code there. Of course that belongs in a separate commit! I'll remove it, leave a comment in the doc and wait until something proper has been added.

Since sf::IntRect is needed for the sf::Screen class, I thought it would be the best to move Rect.hpp into the System class. It seemed to be most fitting since sf::Vector is already there. But of course I will rework it into a separate commit using git mv, so the history isn't lost.
I know #warning isn't a standard preprocessor directive, but all the available compilers support it and I didn't know any other way to issue a compile time warning.

I know a constructor with that many arguments is ugly and not expressive, but I didn't really know how else to do it. Do you have a suggestion? I can make it private again, but the way I understood Laurent it's OK if the user can construct and manipulate a sf::Screen instance as they wish as long as the ones returned by SFML are immutable.

What do you suggest for a consistent naming? The sf::Shape classes have getPointCount(), sf::SoundBuffer has getSampleCount(), etc. but I think that was chosen because it's not clear what sf::Shape::count() or sf::SoundBuffer::count() is supposed return. For sf::Screen::count() it's quiet clear I'd say. But of course I am open for a name that fits better into the consisting API. Also returning std::size_t would be a more consistent right?

I haven't written all of the documentation yet, because I didn't want to rewrite everything while the API is still changing. The idea behind the ID is that you can find the screen belonging to a videomode (to get the screen from mode and to verify the screen supports the mode). The ID's are continuous in range [0 .. count()[. The primary screen is always at index [0] and the rest is sorted from left to right in virtual screen space. I will add some documentation about that. What do others think is index the better name for this?

About const std::wstring ... That used to be a reference, so the const made sense back then. I'll remove it. Of course std::wstring doesn't make sense in the public API, since there is sf::String for that. It's also a unfinished left over and I'll change that as well.

[Hiura]

I know a constructor with that many arguments is ugly and not expressive, but I didn't really know how else to do it. Do you have a suggestion? I can make it private again, but the way I understood Laurent it's OK if the user can construct and manipulate a sf::Screen instance as they wish as long as the ones returned by SFML are immutable.

What about keeping only the default constructor and setting manually the attributes? For such data record it feels alright (nice tradeoff between simplicity and cleanness). Also it's consistent with VideoMode which has a default ctor and public, mutable attributes.

and the rest is sorted from left to right in virtual screen space.

I would remove this requirement for simplicity and avoiding ambiguities (e.g. all screens are vertically stacked).

[Jesper Juhl]

Other options (not saying they are good/bad, just pointing them out) would be:

- a constructor with many arguments but many of them with a default (I would actually consider this bad, but it's an option).
- a constructor taking a single struct rather than many individual arguments.
- multiple constructors taking various number of arguments and then having the object being limited in what it can "answer" based on what ctor was called.

Someone else can hopefully provide other/better options, but I just wanted to point out that there are many ways to "cut the cake"

[Foaly]

Alright I found some time to implement the discussed changes. New commit.
The deprecation macro and std::wstring are gone. I added quiet a bit of documentation (mostly about the sf::Screen and sf::VideoMode class). If someone could proof read it, that would be great!
It finally made click in my head and I understood that the sf::Screen object returned by SFML through get() is protected by being a const reference. So there is no need have a seperate constructor. I removed it like Hiura suggested. Something was scrambly there in my head
I made a seperate commit for moving sf::Rect, but it didn't go as expected. Eventough I used git mv include/SFML/Graphics/Rect.hpp include/SFML/System/Rect.hpp and created a new file, it still looks like I edited the files content on GitHub. Apparently git doesn't like it if you move and create a file with the same name. Also GitHub doesn't show a moved files history. If I use git log --follow include/SFML/System/Rect.hpp the history is all there, but on GitHub there is nothing to see. I asked on SO, but there seems to be no solution.
About the sorting: I think we should provide a consistent order in which the monitors are returned. Otherwise it will be dependend on the OS or even be at random, which I don't think is very clean. Since most setups on a desk are side by side, I thought sorting them from left to right would be the way to go. But of course I can also add a check for sorting them top to bottom incase they have the same x value.

If some more people could do some testing and check if everything works like they'd expect, that would be awesome. Even if you only have 1 monitor or if you have a DPI screen or a win7 or a TV/projector connected as a second screen. Anything would help

[Nexus]

Now sf::Screen still has a public default constructor... Why is that needed? Let's not just add it for convenience -- if we can enforce class invariants (initialized state) with the get() method, let's do that. We can still provide constructors later, if we clearly see a need for it. Don't hesitate to use friend to increase encapsulation. Contrary to popular belief, friendship is not bad design per se.

About count(), as stated earlier, it's inconsistent naming -- it is a verb that counts the number of screens, as opposed to getCount() which returns the count (=number, a noun).

There's quite a few unneeded sf:: prefixes.

I'll try to have a look at the rest and the functionality later.

[Foaly]

Well the public constructor is there mostly for convenience and better encapsulation in sf::ScreenImpl. The ensureScreenListInitilized() is implemented as a function in an anonymous namespace in src/SFML/Window/Win32/ScreenImpl.cpp. So if sf::Screens constructor was private I would have to move ensureScreenListInitilized() to a private method in sf::ScreenImpl and make the two classes friends. priv::getDisplayDeviceFromId() needs access to ensureScreenListInitilized so I would have to move that into sf::ScreenImpl too. But it's a windows specific function, so I don't know if that's very clean... Do you think that would be better? I agree that it doesn't make much sense to let users create their own sf::Screen objects.

I think sf::Screen::count() makes sense and is expressive, but if it's inconsistent with the rest of the API I will change it. Do other think the same?

I'll removed all the unnecessary sf:: I could find. Tell me if I missed any.

Awesome! I'll be glad to hear some feedback.

Here is the new commit.

[Hiura]

About sorting: it can be added later for all OSes in sf::Screen::get if we really want to. The thing is, monitors don't need to be aligned so a top-down, left-right ordering doesn't always results in something meaningful.

Well the public constructor is there mostly for convenience and better encapsulation in sf::ScreenImpl.

How so? `sf::Screen` is currently empty as far as I can tell so removing it completely should not be an issue.

Think of the get function as a factory method that will properly set up all attributes. Even if they are initially initialized to garbage (i.e. with the compiler generated constructor) that's not important since they are then properly updated.

[Nexus]

Well the public constructor is there mostly for convenience and better encapsulation in sf::ScreenImpl.

The encapsulation is actually worse: you expose a method to the public API that is only needed by the implementation.

friend is really the way to go here. There's a nice idiom that avoids additional dependencies just for friendship: accessor structs.

namespace priv
{
    struct ScreenAccess
    {
        static Screen construct();
    };
}

class Screen
{
    friend struct ScreenAccess;
};

I think sf::Screen::count() makes sense and is expressive, but if it's inconsistent with the rest of the API I will change it. Do other think the same?

The coexistence of get() and getCount() is not really nice indeed. There are not many APIs of this style in SFML, so it's a bit difficult to compare... We should keep in mind that in the future, we would need to name others similarly, too.

Awesome! I'll be glad to hear some feedback.

I tested the API on Windows 10 with two screens, it works great. Thanks a lot for your contribution.

I created a Gist based on your code, which shows all the screens' capabilities. Everybody can use that for future testing.

Feedback -- some tips are really minor and not too relevant for this example, but maybe they'll help you also in other code:

• There's hidden console output that must be removed. Any error output must go to sf::err(), not standard streams.
• As mentioned by Laurent, the screen's index should be called "index" and not "id", as that indicates continuous integer ranges starting at 0.
• I wouldn't use std::wstring unless really necessary. We have sf::String.
• "initilized" -> "initialized"
• Currently the window constructor falls back to the default screen if the specified screen index is not supported. This is probably okay regarding that we have no error reporting mechanism, but for SFML 3 we might consider an exception.
• The difference between bounds and working area (without task bar) is not clear from the documentation.
• if conditions with a single statement should not be wrapped in {}, according to SFML's code style.
• Sometimes two empty lines, "if(" and other minor syntactical mistakes.
• ensureScreenListInitialized() is huge, what about splitting it into multiple functions? Then we can initialize different sf::Screen attributes right ahead, instead of copying everything at the end of the function.
• Instead of if (!std::find(/* videoMode */)) modes.push_back(videoMode) you can simply push_back() all modes, and use std::sort() + std::unique() + std::remove().