New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Xlib by XCB (#200) #694

Merged
merged 8 commits into from Jan 7, 2015

Conversation

Projects
None yet
6 participants
@TankOs
Member

TankOs commented Aug 28, 2014

This pull request replaces the use of Xlib by XCB, a more modern library for working with X11.

Most work has been done by @lukas-w. I fixed some issues and adjusted the X11 example. Commits have been squelched.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

The FindXCB.cmake file has no copyright information. Did you write it yourself?

We had one (or more?) hacks, like the "algorithm" to properly detect and discard repeated key events. Has the equivalent code been tested?

Member

LaurentGomila commented Aug 28, 2014

The FindXCB.cmake file has no copyright information. Did you write it yourself?

We had one (or more?) hacks, like the "algorithm" to properly detect and discard repeated key events. Has the equivalent code been tested?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 28, 2014

Member

@lukas-w added and, as far as I know, wrote the file himself.

Can you point me to the relevant spots regarding the key event hacks?

Member

TankOs commented Aug 28, 2014

@lukas-w added and, as far as I know, wrote the file himself.

Can you point me to the relevant spots regarding the key event hacks?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 28, 2014

Member

Thanks!

Good catch, there's indeed a little bug. The hack assumes that the key is still announced as being pressed when the event queue is processed, so that we can discard key release events. With XCB however this is not guaranteed when you finally release the key.

So sometimes SFML will push another pair of press and release events to the queue, instead of the single final release event, i.e.:

  • User presses and holds key (with key repeat enabled)
  • KeyPressed event
  • KeyPressed event
  • KeyPressed event
  • User releases key
  • KeyReleased event
  • KeyPressed event
  • KeyReleased event

The same happens when key repeat is disabled.

I think this is related to XCB's asynchronous nature (especially because it only happens sometimes), and I fear that there's no easy way to fix it. :(

Is there a reason for SFML's behavior (discarding key release events) or is it simply defined as it is?

Member

TankOs commented Aug 28, 2014

Thanks!

Good catch, there's indeed a little bug. The hack assumes that the key is still announced as being pressed when the event queue is processed, so that we can discard key release events. With XCB however this is not guaranteed when you finally release the key.

So sometimes SFML will push another pair of press and release events to the queue, instead of the single final release event, i.e.:

  • User presses and holds key (with key repeat enabled)
  • KeyPressed event
  • KeyPressed event
  • KeyPressed event
  • User releases key
  • KeyReleased event
  • KeyPressed event
  • KeyReleased event

The same happens when key repeat is disabled.

I think this is related to XCB's asynchronous nature (especially because it only happens sometimes), and I fear that there's no easy way to fix it. :(

Is there a reason for SFML's behavior (discarding key release events) or is it simply defined as it is?

@TankOs TankOs added feature labels Aug 28, 2014

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

It's just how how the key repeat feature is defined, no intermediate KeyRelease even should be triggered.

Member

LaurentGomila commented Aug 28, 2014

It's just how how the key repeat feature is defined, no intermediate KeyRelease even should be triggered.

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Aug 28, 2014

Contributor

@lukas-w added and, as far as I know, wrote the file himself.

To be honest, I don't even remember. But as googling for a FindXCB.cmake doesn't bring up any similar files and this one has no copyright info, I suppose it's me who wrote it.

Contributor

lukas-w commented Aug 28, 2014

@lukas-w added and, as far as I know, wrote the file himself.

To be honest, I don't even remember. But as googling for a FindXCB.cmake doesn't bring up any similar files and this one has no copyright info, I suppose it's me who wrote it.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 29, 2014

Member

I fixed the key repeat stuff and updated the PR. Check the processEvents function for details.

XCB sends two events, directly after each other, in case of a key repeat interval: XCB_KEY_RELEASE and XCB_KEY_PRESS. Both events share the same timestamp and key code and are thus perfectly identifiable.

To discard release events of repeated keys, the patch holds back each key release event to wait for a possible key press event with the same timestamp and key code. In that case the held back key release event simply gets discarded. Otherwise the held back event is simply processed.

Checked with Valgrind.

Member

TankOs commented Aug 29, 2014

I fixed the key repeat stuff and updated the PR. Check the processEvents function for details.

XCB sends two events, directly after each other, in case of a key repeat interval: XCB_KEY_RELEASE and XCB_KEY_PRESS. Both events share the same timestamp and key code and are thus perfectly identifiable.

To discard release events of repeated keys, the patch holds back each key release event to wait for a possible key press event with the same timestamp and key code. In that case the held back key release event simply gets discarded. Otherwise the held back event is simply processed.

Checked with Valgrind.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 29, 2014

Member

This is exactly what was done before, if I remember correctly. Then the algorithm evolved to work around remaining bugs: instead of remembering the last key release event we check the next in the queue, we also added a small threshold to the timestamp comparison, etc.

@Bromeon did a lot of testing before we came up with the current code. So we should really test this new code a lot before releasing it.

Member

LaurentGomila commented Aug 29, 2014

This is exactly what was done before, if I remember correctly. Then the algorithm evolved to work around remaining bugs: instead of remembering the last key release event we check the next in the queue, we also added a small threshold to the timestamp comparison, etc.

@Bromeon did a lot of testing before we came up with the current code. So we should really test this new code a lot before releasing it.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Sep 2, 2014

Member

Okay, so I've read X.Org X server source code, I read libX11 (Xlib) source code, I read XCB source code. Besides from being totally shocked etc., I couldn't find the proper lines where the key repeat is being generated and sent. The sources are really confusing, and I have no idea where to look, because documentation is so...old, I don't know.. ;)

What I read from research, though, is that X does indeed send release/press pairs with exactly the same time stamp when key repeat is enabled and a key is held down. I've also taken a look at SDL2's sources, where you've taken the chunk of code from, and the only difference I can find is the "tolerance". But I can't find a single reason for that one.

@Bromeon Laurent mentioned that you did a lot of testing: Was the tolerance really required?

Member

TankOs commented Sep 2, 2014

Okay, so I've read X.Org X server source code, I read libX11 (Xlib) source code, I read XCB source code. Besides from being totally shocked etc., I couldn't find the proper lines where the key repeat is being generated and sent. The sources are really confusing, and I have no idea where to look, because documentation is so...old, I don't know.. ;)

What I read from research, though, is that X does indeed send release/press pairs with exactly the same time stamp when key repeat is enabled and a key is held down. I've also taken a look at SDL2's sources, where you've taken the chunk of code from, and the only difference I can find is the "tolerance". But I can't find a single reason for that one.

@Bromeon Laurent mentioned that you did a lot of testing: Was the tolerance really required?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Sep 4, 2014

Member

Alright, so for me this works on at least 3 systems -- all running Arch Linux, though. If someone with Debian, Ubuntu or whatever can also verify that this patch is working, we could accept this PR. ;) Might be even something for 2.x I guess.

Member

TankOs commented Sep 4, 2014

Alright, so for me this works on at least 3 systems -- all running Arch Linux, though. If someone with Debian, Ubuntu or whatever can also verify that this patch is working, we could accept this PR. ;) Might be even something for 2.x I guess.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

So, any more concerns? Otherwise I'll label this one as being accepted?

Member

TankOs commented Oct 2, 2014

So, any more concerns? Otherwise I'll label this one as being accepted?

@Mischa-Alff

This comment has been minimized.

Show comment
Hide comment
@Mischa-Alff

Mischa-Alff Oct 11, 2014

Contributor

Setting the window position moves the window, but window.getPosition() still returns the old value. Archlinux x86_64 with XFCE4 (and Xfwm4).

Minimal example reproducing the issue: https://gist.github.com/Mischa-Alff/f9798fa0f816664e7b47

This doesn't happen with the master branch.

EDIT: This also happens when moving the window around.

Contributor

Mischa-Alff commented Oct 11, 2014

Setting the window position moves the window, but window.getPosition() still returns the old value. Archlinux x86_64 with XFCE4 (and Xfwm4).

Minimal example reproducing the issue: https://gist.github.com/Mischa-Alff/f9798fa0f816664e7b47

This doesn't happen with the master branch.

EDIT: This also happens when moving the window around.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 13, 2014

Member

Thanks!

Member

TankOs commented Oct 13, 2014

Thanks!

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 13, 2014

Member

@Mischa-Alff It's fixed, feel free to test and report back.

Member

TankOs commented Oct 13, 2014

@Mischa-Alff It's fixed, feel free to test and report back.

@Mischa-Alff

This comment has been minimized.

Show comment
Hide comment
@Mischa-Alff

Mischa-Alff Oct 13, 2014

Contributor

Yep! Works now, thanks!

There's an offset between the position of the window and the position of where you set the window. This is true even without XCB, I was just wondering if it was expected behavior. (setPosition(200, 200), then getPosition() returns (205, 224), thus returning the position of the render context box thing, not the window with decorations, but when you set the window's position, you set the position of the window with decorations)

Contributor

Mischa-Alff commented Oct 13, 2014

Yep! Works now, thanks!

There's an offset between the position of the window and the position of where you set the window. This is true even without XCB, I was just wondering if it was expected behavior. (setPosition(200, 200), then getPosition() returns (205, 224), thus returning the position of the render context box thing, not the window with decorations, but when you set the window's position, you set the position of the window with decorations)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 14, 2014

Member

At least I don't think it should be expected behavior, because to me it looks inconsistent. When I set (100, 100), I expect getPosition to return (100, 100) as well. Will look into it, thanks.

Member

TankOs commented Oct 14, 2014

At least I don't think it should be expected behavior, because to me it looks inconsistent. When I set (100, 100), I expect getPosition to return (100, 100) as well. Will look into it, thanks.

@Mischa-Alff

This comment has been minimized.

Show comment
Hide comment
@Mischa-Alff

Mischa-Alff Oct 14, 2014

Contributor

This happens with GLFW, and the GLFW devs claim this is normal, expected behavior. Even though it is counterintuitive.

Contributor

Mischa-Alff commented Oct 14, 2014

This happens with GLFW, and the GLFW devs claim this is normal, expected behavior. Even though it is counterintuitive.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 14, 2014

Member

Yes, it is "normal", because there is magic going on in the window managers. I checked AwesomeWM source code to validate, and I'm again annoyed by how things are working.

  • When you create a new window under the root window, the window manager will re-parent it to a window that contains window decoration.
  • From that point on, requesting your window's position will return the position relative to its parent, i.e. window decoration frame, which is for my WM (AwesomeWM) always 0, 0.
  • If you ask X11 to translate the position, you get the absolute position of your window, which is effectively the position of the window decoration frame + relative position of the client area. Up to here, everything makes perfect sense!
  • If you however set the window's position (called "configuring a window"), the window manager normally takes over and will redirect the request to the "top level" window (there's no such concept actually; what I mean are windows that are directly under the root window), thus effectively setting another window's position.

And that's what will bring the things out of sync, i.e. the position you set won't be the one you request.

I worked around that issue by fetching the "top level" window's position, by climbing up the ladder up to the root window.

I have also fixed some memory leaks. ;)

Member

TankOs commented Oct 14, 2014

Yes, it is "normal", because there is magic going on in the window managers. I checked AwesomeWM source code to validate, and I'm again annoyed by how things are working.

  • When you create a new window under the root window, the window manager will re-parent it to a window that contains window decoration.
  • From that point on, requesting your window's position will return the position relative to its parent, i.e. window decoration frame, which is for my WM (AwesomeWM) always 0, 0.
  • If you ask X11 to translate the position, you get the absolute position of your window, which is effectively the position of the window decoration frame + relative position of the client area. Up to here, everything makes perfect sense!
  • If you however set the window's position (called "configuring a window"), the window manager normally takes over and will redirect the request to the "top level" window (there's no such concept actually; what I mean are windows that are directly under the root window), thus effectively setting another window's position.

And that's what will bring the things out of sync, i.e. the position you set won't be the one you request.

I worked around that issue by fetching the "top level" window's position, by climbing up the ladder up to the root window.

I have also fixed some memory leaks. ;)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 14, 2014

Member

Oh, and by the way: I invite everybody to test this! Just use Mischa's test code (https://gist.github.com/Mischa-Alff/f9798fa0f816664e7b47), press T and validate that the console output reads "Current window position: 200, 200".

Member

TankOs commented Oct 14, 2014

Oh, and by the way: I invite everybody to test this! Just use Mischa's test code (https://gist.github.com/Mischa-Alff/f9798fa0f816664e7b47), press T and validate that the console output reads "Current window position: 200, 200".

@Mischa-Alff

This comment has been minimized.

Show comment
Hide comment
@Mischa-Alff

Mischa-Alff Oct 15, 2014

Contributor

Thanks for the patch!

This works on Archlinux XFCE4 (Xfwm4). Whether it's normal behavior or not, all that matters is that window.setPosition(window.getPosition()) does not move the window, and that the behavior is the same across platforms.

Contributor

Mischa-Alff commented Oct 15, 2014

Thanks for the patch!

This works on Archlinux XFCE4 (Xfwm4). Whether it's normal behavior or not, all that matters is that window.setPosition(window.getPosition()) does not move the window, and that the behavior is the same across platforms.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 20, 2014

Member

Needs a rebase and heavy testing.

Member

eXpl0it3r commented Dec 20, 2014

Needs a rebase and heavy testing.

@TankOs TankOs self-assigned this Dec 29, 2014

@TankOs TankOs removed the s:unassigned label Dec 29, 2014

lukas-w and others added some commits Nov 25, 2012

Replaced Xlib by XCB implementation.
* Added FindXCB.cmake script
* Added AutoPointer wrapper for automatically free'ing pointers
* Huge commit: Ported linux implementation of sfml-window to xcb
* Xcb is now used for window creation, event loop etc
* As GLX is linked to Xlib, that part of the implementation
  still uses Xlib.
* Also, some keyboard related (such as XLookupString) stuff
  is still Xlib, as xcb does not have it (yet?).
* Replaced some enums with the xcb equivalents
Adjusted and fixed XCB patch.
* Adjusted xcb_icccm calls (for recent XCB versions).
* Fixed wrong parameter order in xcb_icccm_set_wm_protocols call.
* Fixed XCB_BUTTON_RELEASE spawning a MouseButtonPressed event.
* Moved files from obsolete Linux/ to Unix/ directory.
* Added m_useSizeHints fix.
* setTitle() converts to UTF-8 before passing to XCB -> Unicode window title
  support.
* Added XCB-util dependency.
* Replaced XSelectInput. Obtaining XCB connection when taking window handle.
* Adjusted X11 example for XCB.
* Removed AutoPointer, replaced by direct XCB and free() calls.
* Added key repeat workaround.
@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Dec 29, 2014

Member

Rebased and tested, works flawlessly for me. Let's merge this into master so that others will test it as well.
👍 🐈

Member

TankOs commented Dec 29, 2014

Rebased and tested, works flawlessly for me. Let's merge this into master so that others will test it as well.
👍 🐈

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 29, 2014

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Dec 29, 2014

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@TankOs TankOs added s:accepted and removed s:undecided labels Dec 29, 2014

@Mischa-Alff

This comment has been minimized.

Show comment
Hide comment
@Mischa-Alff

Mischa-Alff Dec 29, 2014

Contributor

How did SFML's Linux additions since this was written (such as #613 ) make it into this?

Contributor

Mischa-Alff commented Dec 29, 2014

How did SFML's Linux additions since this was written (such as #613 ) make it into this?

@jcowgill

This comment has been minimized.

Show comment
Hide comment
@jcowgill

jcowgill Dec 30, 2014

Contributor

Some warnings when compiled with GCC 4.9.2 with '-Wall -Wextra'

[ 34%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/InputImpl.cpp.o
/home/james/workspace/SFML/src/SFML/Window/Unix/InputImpl.cpp:182:48: warning: unused parameter ‘visible’ [-Wunused-parameter]
 void InputImpl::setVirtualKeyboardVisible(bool visible)
                                                ^
[ 36%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/SensorImpl.cpp.o
[ 37%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/VideoModeImpl.cpp.o
/home/james/workspace/SFML/src/SFML/Window/Unix/VideoModeImpl.cpp: In static member function ‘static sf::VideoMode sf::priv::VideoModeImpl::getDesktopMode()’:
/home/james/workspace/SFML/src/SFML/Window/Unix/VideoModeImpl.cpp:142:38: warning: unused variable ‘Rotation’ [-Wunused-variable]
                 xcb_randr_rotation_t Rotation = (xcb_randr_rotation_t)config->rotation;
                                      ^
[ 38%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/WindowImplX11.cpp.o
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In constructor ‘sf::priv::WindowImplX11::WindowImplX11(sf::WindowHandle)’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:130:49: warning: narrowing conversion of ‘{anonymous}::eventMask’ from ‘long unsigned int’ to ‘const uint32_t {aka const unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
         const uint32_t value_list[] = {eventMask};
                                                 ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In constructor ‘sf::priv::WindowImplX11::WindowImplX11(sf::VideoMode, const sf::String&, long unsigned int, const sf::ContextSettings&)’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:191:57: warning: narrowing conversion of ‘{anonymous}::eventMask’ from ‘long unsigned int’ to ‘const uint32_t {aka const unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
     const uint32_t value_list[] = {fullscreen, eventMask};
                                                         ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:188:17: warning: variable ‘visualInfo’ set but not used [-Wunused-but-set-variable]
     XVisualInfo visualInfo = ContextType::selectBestVisual(m_display, mode.bitsPerPixel, settings);
                 ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In member function ‘virtual void sf::priv::WindowImplX11::processEvents()’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:381:50: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     while(event = xcb_poll_for_event(m_connection))
                                                  ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In member function ‘virtual void sf::priv::WindowImplX11::setPosition(const Vector2i&)’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:462:48: warning: narrowing conversion of ‘position.sf::Vector2<int>::x’ from ‘const int’ to ‘uint32_t {aka unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
     uint32_t values[] = {position.x, position.y};
                                                ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:462:48: warning: narrowing conversion of ‘position.sf::Vector2<int>::y’ from ‘const int’ to ‘uint32_t {aka unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: At global scope:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:71:10: warning: ‘int {anonymous}::checkEvent(Display*, XEvent*, XPointer)’ defined but not used [-Wunused-function]
     Bool checkEvent(::Display*, XEvent* event, XPointer userData)
          ^
Contributor

jcowgill commented Dec 30, 2014

Some warnings when compiled with GCC 4.9.2 with '-Wall -Wextra'

[ 34%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/InputImpl.cpp.o
/home/james/workspace/SFML/src/SFML/Window/Unix/InputImpl.cpp:182:48: warning: unused parameter ‘visible’ [-Wunused-parameter]
 void InputImpl::setVirtualKeyboardVisible(bool visible)
                                                ^
[ 36%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/SensorImpl.cpp.o
[ 37%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/VideoModeImpl.cpp.o
/home/james/workspace/SFML/src/SFML/Window/Unix/VideoModeImpl.cpp: In static member function ‘static sf::VideoMode sf::priv::VideoModeImpl::getDesktopMode()’:
/home/james/workspace/SFML/src/SFML/Window/Unix/VideoModeImpl.cpp:142:38: warning: unused variable ‘Rotation’ [-Wunused-variable]
                 xcb_randr_rotation_t Rotation = (xcb_randr_rotation_t)config->rotation;
                                      ^
[ 38%] Building CXX object src/SFML/Window/CMakeFiles/sfml-window.dir/Unix/WindowImplX11.cpp.o
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In constructor ‘sf::priv::WindowImplX11::WindowImplX11(sf::WindowHandle)’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:130:49: warning: narrowing conversion of ‘{anonymous}::eventMask’ from ‘long unsigned int’ to ‘const uint32_t {aka const unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
         const uint32_t value_list[] = {eventMask};
                                                 ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In constructor ‘sf::priv::WindowImplX11::WindowImplX11(sf::VideoMode, const sf::String&, long unsigned int, const sf::ContextSettings&)’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:191:57: warning: narrowing conversion of ‘{anonymous}::eventMask’ from ‘long unsigned int’ to ‘const uint32_t {aka const unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
     const uint32_t value_list[] = {fullscreen, eventMask};
                                                         ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:188:17: warning: variable ‘visualInfo’ set but not used [-Wunused-but-set-variable]
     XVisualInfo visualInfo = ContextType::selectBestVisual(m_display, mode.bitsPerPixel, settings);
                 ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In member function ‘virtual void sf::priv::WindowImplX11::processEvents()’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:381:50: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     while(event = xcb_poll_for_event(m_connection))
                                                  ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: In member function ‘virtual void sf::priv::WindowImplX11::setPosition(const Vector2i&)’:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:462:48: warning: narrowing conversion of ‘position.sf::Vector2<int>::x’ from ‘const int’ to ‘uint32_t {aka unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
     uint32_t values[] = {position.x, position.y};
                                                ^
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:462:48: warning: narrowing conversion of ‘position.sf::Vector2<int>::y’ from ‘const int’ to ‘uint32_t {aka unsigned int}’ inside { } is ill-formed in C++11 [-Wnarrowing]
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp: At global scope:
/home/james/workspace/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:71:10: warning: ‘int {anonymous}::checkEvent(Display*, XEvent*, XPointer)’ defined but not used [-Wunused-function]
     Bool checkEvent(::Display*, XEvent* event, XPointer userData)
          ^
Code adjustments to fix warnings.
Change-Id: Iba40752c6c5baaadc2a1b6a0fd03cbb0e3cde8a3
@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 5, 2015

Member

@Mischa-Alff
Just tested, it works. However, I will have to adjust the window focus code to use XCB to be fully compatible. We knew that this is going to happen, because XCB wasn't accepted for 2.2. 🐈

@jcowgill
Thank you, I fixed some more spots. However, I'm not quite sure about the warning level anyway. I usually compile with -Wall -Wextra -Wshadow -Wconversion -pedantic, and that spits out tons of warnings. At least the "commonly used" triggers are quiet now. ;-)

@LaurentGomila
Thanks. The proper line was dropped as it wasn't even used.

Member

TankOs commented Jan 5, 2015

@Mischa-Alff
Just tested, it works. However, I will have to adjust the window focus code to use XCB to be fully compatible. We knew that this is going to happen, because XCB wasn't accepted for 2.2. 🐈

@jcowgill
Thank you, I fixed some more spots. However, I'm not quite sure about the warning level anyway. I usually compile with -Wall -Wextra -Wshadow -Wconversion -pedantic, and that spits out tons of warnings. At least the "commonly used" triggers are quiet now. ;-)

@LaurentGomila
Thanks. The proper line was dropped as it wasn't even used.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 6, 2015

Member

Just tested, it works. However, I will have to adjust the window focus code to use XCB to be fully compatible.

Do you want to do this before or after merging this branch?

Member

eXpl0it3r commented Jan 6, 2015

Just tested, it works. However, I will have to adjust the window focus code to use XCB to be fully compatible.

Do you want to do this before or after merging this branch?

Adjusted window focus changes to be XCB-compatible.
Change-Id: I0fe2c7d1698bce23b81f5c6a9db018f7a3fe49d8
@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 6, 2015

Member

@eXpl0it3r Before, of course. Otherwise the XCB support is not complete.

Member

TankOs commented Jan 6, 2015

@eXpl0it3r Before, of course. Otherwise the XCB support is not complete.

Removed link to Xlib. Replaced more Xlib calls by XCB calls.
Change-Id: I05d8b24508e88b604f7cc76622cc8af695204990
@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 6, 2015

Member

I'm done. This needs heavy testing, please merge. 👍 🐈

Member

TankOs commented Jan 6, 2015

I'm done. This needs heavy testing, please merge. 👍 🐈

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 6, 2015

Member

This PR has been added again to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Jan 6, 2015

This PR has been added again to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r merged commit f464e25 into master Jan 7, 2015

@eXpl0it3r eXpl0it3r deleted the feature/xcb branch Jan 7, 2015

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 7, 2015

Member

🐈

Member

TankOs commented Jan 7, 2015

🐈

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Jan 7, 2015

Contributor

🚲

Contributor

lukas-w commented Jan 7, 2015

🚲

@TankOs TankOs removed their assignment Apr 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment