Multi monitor support

[Foaly]

Thanks for the review and the detailed feedback!

Accessor structs look like a nifty little pattern. I've never heared about those. Thanks for the tip! I went ahead made the constructor private and added a ScreenAccess struct. I put it into the Screen.hpp because having two seperate files for such a small thing seemed like overkill to me. (I kept the constructor because I think an object should always be in a valid state and not have any bogus data, even if it is overwritten directly afterwards.)

The coexistence of get() and getCount() is not really nice indeed.

I agree with that. So what's you verdict on this? Leave it as it is?

• That was for developing. I took it out and redirected any errors to sf::err().
• Replaced.
• Done.
• Done.
• Well I thought that that would be the best behaviour in case of an error. But it could definitly be considert for an excetion.
• Do you mean here? I rewrote it a bit, but I don't really know how else to put it. What's not clear?
• I went over the style guide again and I hope I didn't miss anything.
• Same.
• Hmm I admit its pretty big, but the problem is that some calls depend on each other and some calls have to be made in a certain order, so it's not that easy to separate them. I documented everything to make it as clear as possible. But I'll look into it again.
• I took the code from sf::VideoMode::getFullscreenModes(), but yours is more elegant and efficient. Done

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

Thank you! Thats great to hear.

I rewrote the example in the documentation based on your gist.
So here is the new commit.

I'd like to know what others think about the sorting. Should it be removed or not?

I also made another commit that removes the limitation of being able to create only one fullscreen window. So you can now create a fullscreen windows on each of your screens at the same time! I left it in a seperate commit because it is easier to review this way. I'll squash them once it is reviewed.

[Laurent]

Accessor structs look like a nifty little pattern. I've never heared about those. Thanks for the tip! I went ahead made the constructor private and added a ScreenAccess struct.

That's a nice pattern, but there's no need to overuse it when it's not necessary -- it's also good if we can avoid the extra code (especially if you put it in a public header, which I guess was not what Nexus implied ). Here it doesn't hurt to have the whole sf::ScreenImpl as a friend of sf::Screen, with a simple forward-declaration of it in the public header.

I agree with that. So what's you verdict on this? Leave it as it is?

No. A function should be a verb, that's a mandatory rule of the style guide (well, in case it's not, it should). Maybe it's "get" which should be renamed? Or maybe we can simply merge them in a single accessor which returns a std::vector<sf::Screen>, like sf::VideoMode does?

[Jesper Juhl]

I kept the constructor because I think an object should always be in a valid state and not have any bogus data, even if it is overwritten directly afterwards.

But, an empty user-defined default constructor is exactly equivalent to the one the compiler will generate for you if you leave it out. With the little twist that user-defined constructors are, by definitian, never trivial (even if they are empty) but the compiler generated one is (if left out or defined as "= default;").

[Laurent]

I don't understand why you all want to remove the default constructor. Since the class has primitive type members which won't get initialized properly, to me it is mandatory to write a constructor to ensure a valid state -- which is exactly what Foaly argued.

[Hiura]

I suggested removing the ctor initially because there already is a factory method: get. Having this extra struct only add one indirection. This approach would be similar to sf::Event and pollEvent/popEvent combo.

Also, having it private prevents people from having a screen instance that gets properly setup later on, doesn't it?

[Nexus]

6. Do you mean here? I rewrote it a bit, but I don't really know how else to put it. What's not clear?

I didn't see that, I assumed the member would only be documented in the member's documentation itself. Your description is fine (except for "screens bounds" which should be "screen's bounds").

9. Hmm I admit its pretty big, but the problem is that some calls depend on each other and some calls have to be made in a certain order, so it's not that easy to separate them. I documented everything to make it as clear as possible. But I'll look into it again.

Why are dependencies and order a problem? You can of course preserve them. I'm aware that you may need to pass several arguments, but often this is still better than a massive block of code, because it makes clear which function changes what.

I don't understand why you all want to remove the default constructor. Since the class has primitive type members which won't get initialized properly, to me it is mandatory to write a constructor to ensure a valid state -- which is exactly what Foaly argued.

Yes, but it should be private. Ironically, the argument "valid state" is actually a counterargument, because the default constructor creates a semantically invalid screen instance, which refers to no real screen. The whole point behind constructors however is to initialize instances to a valid state.

Whether the attributes are in a defined but undocumented state, or in an undefined state, doesn't matter in practice. Users can't make any assumptions about uninitialized instances. All they can do is assign a valid one.

Also, having it private prevents people from having a screen instance that gets properly setup later on, doesn't it?

It does, but it's not a library writer's task to provide a null state for every object. In my opinion, this is already used far too much for convenience, with the effect that we give up class invariants. In this case, it's even worse because there is no officially documented way to check for a null state. This whole discussion has a bigger scope than just sf::Screen, as there are several classes in SFML doing the same. Maybe we should bring it up again for SFML 3...

Java designers also thought making every object reference nullable would be a good idea, with the result that NullPointerException is an omnipresent bug in Java development. The effect was so severe that various libraries began to provide half-hearted solutions (@Nullable and @NonNull annotations for static code analyzers). This is something you don't have in C++, and I'm really glad about it.

In a near-future C++ version (probably an intermediate library fundamentals technical specification), we will have std::optional which is exactly designed to provide optional state. boost::optional has already existed for ages.


A different question: Does it even make sense when the user creates copies of sf::Screen objects? There is no point in modifying one. If every screen is saved globally, wouldn't const pointers and references suffice?

[Laurent]

If every screen is saved globally, wouldn't const pointers and references suffice?

It would. But it would force users to use pointers sometimes, are you ok with that?

Unrelated comment: wouldn't a static getPrimaryScreen (or index) function be better than a isPrimary member? You usually want to get the primary display directly, not know if a given display is primary or not.

So we could imagine...

class Screen
{
public:

    static const Screen* getPrimary();

    static std::vector<const Screen*> getAll();

private:

    ...
};

I'm not sure if I prefer this design, this is just an idea...

[Nexus]

It would. But it would force users to use pointers sometimes, are you ok with that?

If there's no point in making a copy, yes. sf::Screen being noncopyable also makes clear that users are supposed to treat it as an immutable object retrieved only through SFML, not something they can instantiate and control themselves.

In other words, it's then very similar to the classes sf::Mouse, sf::Keyboard, sf::Joystick, sf::Sensor, sf::Touch and future sf::Clipboard: global access points to features that exist only once on the system, i.e. kind of singletons. The only reason why we can't make sf::Screen the same is because there can be multiple screens, so we need the get() indirection. I don't see why anybody would save a screen, if SFML stores it already and modifications to copies make no sense.

Unrelated comment: wouldn't a static getPrimaryScreen (or index) function be better than a isPrimary member? You usually want to get the primary display directly, not know if a given display is primary or not.

That sounds good. We should return references though, to signal that they're never null. People who need to be flexible can still take the address.

I'm not sure if the isPrimary variable should be left in the class. Already now, the index variable is strictly speaking redundant. But I can imagine situations where both can be handy.

Alternative method names to get would be getNth (borrowed from std::nth_element()), getAt, getByIndex... But none of them except maybe the first pleases me too much. Providing a STL container interface on the other hand allows for things like:

for (const sf::Screen* screen : sf::Screen::getAll())
{
   screen->xy;
}

auto&& screens = sf::Screen::getAll();
std::for_each(screens.begin(), screens.end(), function);

boost::for_each(sf::Screen::getAll(), function);

instead of:

for (std::size_t i = 0; i < sf::Screen::getCount(); ++i)
{
   const sf::Screen* screen = sf::Screen::getNth(i);
   screen->xy;
}

??? // No STL or Boost algorithm can be applied

[Hiura]

Commuting right now so ill make even shorter than usual

Having a custom ctor, even if it is made non copyable, doesn't allow us to enforce any invariant: the data is still publicly modifiable. The way I see it, sf::Screen is just some POD, nothing more. Which means we don't need an 'invalid' state: an 'undefined' state is enough (which is not probable). It simple enough to let users take care themselves of using valid data by calling get. Would that be really a bad design IYO?

Also, I fear that using ref/ptr/static data will make our job harder latter if we want to introduce screen events /state change /data invalidation. We should probably look more into what I mentioned in one of my first posts: the joystick API with only static function to access information (which, as a bonus, would solve the issue of invariant).

[Nexus]

Having a custom ctor, even if it is made non copyable, doesn't allow us to enforce any invariant: the data is still publicly modifiable.

No. We have no public constructor, and the static sf::Screen methods only give away const pointers and const references. Users thus cannot modify the objects.

We should probably look more into what I mentioned in one of my first posts: the joystick API with only static function to access information (which, as a bonus, would solve the issue of invariant).

Yes, but:

In other words, it's then very similar to the classes sf::Mouse, sf::Keyboard, sf::Joystick, sf::Sensor, sf::Touch and future sf::Clipboard: global access points to features that exist only once on the system, i.e. kind of singletons. The only reason why we can't make sf::Screen the same is because there can be multiple screens, so we need the get() indirection.

Also, I fear that using ref/ptr/static data will make our job harder latter if we want to introduce screen /state change /data invalidation.

Hm, really good point. What if the user plugs in another screen while the program runs?

Maybe it's not a good idea to store the current screen configuration inside sf::Screen in the first place. Instead, it would be recomputed on every call, and the user is responsible for storing it. In that case, value semantics would make sense again.

[Hiura]

What if the user plugs in another screen while the program runs?

On OS X it's even easier to change the screen configuration: we can change the "scaling factor" of the screen.

This fact is behind my whole reasoning: with static data in the implementation and indirect access (ptr/ref), even if it's read-only, we are stuck with the old dangling pointer thingy and UB. Returning copies of the data would simplify just A LOT the impl and users' life.

Just to be clearer on what I meant by the Joystick API, in case there was some misunderstanding, is that you fetch each information through a getter such as getAxisPosition(index, axis) or getIdentification(index), which returns a POD and is somewhat similar to the currently proposed sf::Screen::get(index).

[Nexus]

This fact is behind my whole reasoning: with static data in the implementation and indirect access (ptr/ref), even if it's read-only, we are stuck with the old dangling pointer thingy and UB. Returning copies of the data would simplify just A LOT the impl and users' life.

I totally agree. Initially, I didn't think of configurations changing at runtime.

Just to be clearer on what I meant by the Joystick API, in case there was some misunderstanding, is that you fetch each information through a getter such as getAxisPosition(index, axis) or getIdentification(index), which returns a POD and is somewhat similar to the currently proposed sf::Screen::get(index).

Okay... However this is one step back from object-oriented programming, when users are not able to treat attributes in a combined way.

Classes with only static functions are nothing more than namespaces. That's not bad per se, but sometimes singletons would be more flexible. sf::Mouse::getInstance() for example would allow things like lazy initialization, and it could be fitted with state that is not purely global but depending on the object (e.g. for multiple mice). Same for joystick, it might have been more intuitive to provide sf::Joystick::get() and let the user access actual sf::Joystick instances. But that's another discussion...

It is however important to see that analogous to the sf::Joystick API would not be sf::Screen::get(index) but rather sf::Screen::getName(index), sf::Screen::getDpi(index), sf::Screen::getFullscreenModes(index), etc.

I'm not sure whether we should use the sf::Joystick API for sf::Screen. I think it mostly depends on whether a user is supposed to store some of the values (which would be much easier if there were an object), or should we merely provide a global access point for currently connected screens, with their current configuration? Maybe that's not the worst idea, as that would make sure users retrieve always the latest, most up-to-date information.

[Hiura]

Yeah, I agree, the usage is different between Screen and Joystick: in the latter case you are interested most of the time in one information only (one button, the identification data of the device, ...), while you pretty much want to know everything about your screen settings. Having one getter that returns a POD with name, dpi and whatnot, like we actually do with Joystick::getInformation, would make it easier for the user to compare screens.

or should we merely provide a global access point for currently connected screens, with their current configuration?

Like the getAll() function Laurent mentioned? I think that would be good: the impl and the users wouldn't have to deal with screen reconfigurations between calls to get(index), even though it's relatively unlikely. But more importantly, the user will probably loop on the Screen to find the best one anyway.

[Nexus]

I think that would be good: the impl and the users wouldn't have to deal with screen reconfigurations between calls to get(index), even though it's relatively unlikely.

Good point again. I wonder however what happens if a screen is reconfigured during such a call? I don't think there's a way to provide atomicity at this level...

So far, I like Laurent's API the most. Taking into account that screen configurations may change during the program, it may really be worthwhile to work with copies and not cache any data within sf::Screen.

class Screen
{
public:
    static Screen getPrimary();
    static std::vector<Screen> getAll();
};

The alternative is that the sf::Screen implementation has a means of noticing configuration changes, and can update its internal structures (automatically or on demand). This adds more complexity to the implementation and still introduces usability issues ("is the pointer I have saved still valid?"), so I'm not sure if we should go with that.

Interestingly, the issue of changing configuration in-use exists for sf::Joystick, too. So far, probably no one was unlucky enough to experience it

[Foaly]

First of all I'd like to say that it's great to see an active discussion on this topic. I'm not a library designer but I'll still throw in some of my thoughts.

I like the idea that you can use getAll() and a range based for loop to iterate over the screens. Although I can't think of many use cases where you'd want to use STL algorithms on screens. Never the less I think it's good practice to support them.

getPrimary() is also a good idea, because right now there are already some spots in the code where sf::Screens::get(0) is used to get the primary screen. getPrimary() is definitly more expressive and that's what people will use most of the time, because most people have one screen.

I think the data should be cached internally. It makes screens a lighter resource. And when you think about it screen changes are unlikely to happen while the program runs. Usually you set your screens up, before you start a program. So if a screen change event occurs you could simply set a flag and recompute the internal screen data at the next use. So we have a resource that doesn't change very often and we can easily be notified about change once we have screen events. These are ideal conditions for caching. If you only hand out copys or references you also don't have problems with invalid pointers.

To keep a getByIndex() function would be helpful I think. I can think of cases where users want to get a screen from it's index (for example get the screen corresponding to a video mode), instead of copying the whole vector and the only pick one element. Also we do that internally when switching to fullscreen.

There are two other things I'd like to discuss. First is I'd like to suggest a sf::Screen::contains(sf::Vector2) function. I can think of use cases where the user wants a window to go fullscreen on the monitor it's currently on. Right now there is no way to do it. Also this function would be usefull here (construct a window from handle) to get the bits per pixel of the correct monitor (right now it just takes the primary one).
The other thing is a commit I made, that removes the restriction that you can only have one fullscreen window at a time. That made sense when we didn't have multi screen support, but I think we can savely remove it now. I made a seperate commit, so it's easier to review. I'll squash them once it's reviewed.