xfreerdp fullscreen enhancements #2426
Conversation
…ed. Window manager should always know about the main window. Small cleanup of passing around decorations flag. Limit PercentScreen to single monitor vs. entire desktop. IMO - this is better behavior in a multimonitor environment. Handle fullscreen windows better: 1. Ensure that size hints are set to allow resizing before setting a window to fullscreen as some window managers do not behave properly. 2. Handle fullscreen toggles without destroying and recreating window. 3. Use NET_WM_STATE_FULLSCREEN Extended Window Manager Hint for fullscreen functionality 4. Use the NET_WM_FULLSCREEN_MONITORS Extended Window Manager Hint when appropriate 5. When a single monitor fullscreen is requested - use the current monitor(as determined from mouse location) 6. Handle cases where there is no local monitor at coordinate 0,0. The Windows server expect there to be a monitor at this location, so we maintain offset if necessary between our local primary monitor and the server side primary monitor located at 0,0.
|
Can one of the admins verify this patch? |
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
|
@bjcollins what window managers did you test this with? |
|
Refer to this link for build results (access rights to CI server needed): |
|
@bmiklautz I tested against metacity on CentOS 6.6. This follows along similar thinking to Change Request 1392 and uses some of its code, but since that CR was never acted on and I am actively looking at xfreerdp again - I decided to push my own change request. I believe in using the EWMH functionality for fullscreen and multimonitor support vs. override_redirects. |
|
Can't test the patch atm, but would really be happy to see it merged (using #1392 for some clients) |
xfreerdp fullscreen enhancements
|
@bjcollins Have you tested with /smart-sizing? |
| } | ||
|
|
||
| window->fullscreen = TRUE; |
There was a problem hiding this comment.
Shouldn't this be window->fullscreen = fullscreen ?
There was a problem hiding this comment.
Yes. It looks like it should. I do not think that variable is actually used by anything, which is probably why it got overlooked and did not cause me any functional problems.
|
@nfedera I have not tested with smart-sizing(that is kind of new functionality to me and I missed it and how my changes affected it). Now that this patch has been merged, I will plan to work on a new patch to take into account smart-sizing. |
|
@bjcollins very cool! However,I think smart-sizing was already partly broken before your commit. AFAIK at commit 415a0a1 it was still in perfect shape. |
|
@bjcollins Important for smart sizing is that it works with full screen toggle. That is if you start your session with /size:1024x768, then resize the window to your likings, press ctrl+alt+enter to go into full screen, the session must be scaled accordingly. If you press ctrl+alt+enter again to get out of fullscreen the window should have the same smart-sized dimensions it had before toggeling. In addition it is also possible to specify the initial scaled size using /smart-sizing:WidthxHeight |
|
@bjcollins We do also have code for pinch/zoom and panning. Therefore, xfc->window->width is also not necessarily the width to be scaled to. We might be scaling, but the window has a fixed size (think of your smart phone's display as the window and the rdp session is the picture you're zooming). This is the reason why we need the separate xfc->scaledWidth and xfc->scaledHeight members. |
|
@nfedera thanks for the information. That all makes sense to me. I doubt I will get to it this week, but I will try to look into smart sizing soon and at the least try to submit a patch to correct my changes to also take into account smart sizing. |
|
@bjcollins I've created PR #2530 which reverts your PR and fixes the issues which were already partly broken before your commit. |
|
@bjcollins Have you tested full screen toggling (as in ctrl+alt+enter) with your patch ? |
| settings->MonitorCount = nmonitors; | ||
|
|
||
| vWidth = vHeight = 0; | ||
| settings->DesktopPosX = maxWidth - 1; |
There was a problem hiding this comment.
If that was the only real use for the variable maxWidth it should have been be removed
There was a problem hiding this comment.
Agreed. That variable is no longer used and should be completely removed to simplify the code.
|
@nfedera Yes. I tested with fullscreen toggle to be sure that I could remove the bit of code to destroy the window and create a new one every time there is a fullscreen toggle sequence. Have you seen issues with this? |
|
@bjcollins there are of course issues with smart-sizing which was initialized in xf_create_window . Probably also if the setup changes because xf_create_window now is not called anymore which did also call xf_detect_monitors before. I'm currently looking into this but the complete x11 code (not talking about your patch) is really messy and needs a proper redesign IMHO. |
|
@nfedera I agree that the X11 code is generally messy. It may make sense to put the destroy/create window bit back if the smart-sizing stuff is too intertwined into those other functions right now. I do not believe that change was a requirement for the user facing functionality in my patch, it just seemed like unnecessary steps to completely destroy and recreate the window so that is why I removed it. |
|
@bjcollins Is it just me or has your pr also broken /span and /multimon ? [edit] |
|
@nfedera I just looked at the source real quickly for icewm and it does not seem to support the EWMH fullscreen bits, so that is probably the problem there. I would not expect it to just totally not work, were you only seeing an issue in certain scenarios or could you never get it to display on more than one monitor at all? |
|
@bjcollins it seems that also simple /f is now broken too. |
|
@nfedera Testing this functionality was a big part of my PR, so I do not think it is completely broken. However, I do not doubt that that you are having a problem. In an ideal situation, you would be using Xinerama and the WM would support the EWMH(mainly NET_WM_STATE_FULLSCREEN and NET_WM_FULLSCREEN_MONITORS) and I mainly tested with that. So, to help me understand the code path that is being taken can you confirm: a) whether or not you have compiled xfreerdp with xinerama support I will look at this later today, but wanted to see if I can learn a little more about your environment. |
|
@bjcollins I'm running ubuntu 14.04 on an iMac. The native display is 2560x1440. I've connected an Asus 1920x1080 monitor via ThunderboldToDVI and positioned it left of the iMac. However, don't bother looking into it. Thanks [EDIT] |
|
@bjcollins Here it is #2547 |
Remove override redirect flag for fullscreen with keyboard grab enabled. Window manager should always know about the main window.
Small cleanup of passing around decorations flag.
Limit PercentScreen to single monitor vs. entire desktop. IMO - this is better behavior in a multimonitor environment.
Handle fullscreen windows better:
I did take some of this from another pull request from a long time ago, but added things like handling when no local monitors have a 0,0 coordinate. Please provide comments/suggestions/thoughts.