Fix return from fullscreen for linux/gnome not returning to previous size. #3004

Merged
merged 1 commit into from Aug 27, 2013

Conversation

Projects
None yet
5 participants
@nikolai-r
Contributor

nikolai-r commented Jul 24, 2013

Seems I am not the only one with this issue. Not sure the exact test cases that cause this issue but under linux/gnome when returning from fullscreen mode to window mode. There is a window resize event sent to XBMC after exiting fullscreen that sets it to the size of current desktop resolution after setting it to the previous window size, resulting in an unwanted maximized window. This simple fix just checks if the resize event is the same dimensions as the desktop resolution while NOT in fullscreen to avoid a resize. Has only been tested on linux mint 15 (cinnamon). Since this case should never happen anyways seems like a reasonable quick fix. Possible to cause resize issues with hidden task bars when maximizing the widow though, this is untested.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jul 24, 2013

Member

This should not be handled in application. Windowing has to cope with this.

Member

FernetMenta commented Jul 24, 2013

This should not be handled in application. Windowing has to cope with this.

@nikolai-r

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 1, 2013

Contributor

Code moved.

Contributor

nikolai-r commented Aug 1, 2013

Code moved.

@elupus

View changes

xbmc/Application.cpp
+ CSettings::Get().SetInt("window.width", newEvent.resize.w);
+ CSettings::Get().SetInt("window.height", newEvent.resize.h);
+ CSettings::Get().Save();
+ }

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Aug 1, 2013

Member

Wrong indentation.

@elupus

elupus Aug 1, 2013

Member

Wrong indentation.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Aug 1, 2013

Member

what I meant is that application should not bothered with special cases going just wrong on a single platform. I think the reason for this issue is SDL. It operates with three windows and WinEventsSDL may catch a ConfigureNotify event from an invisible one.

This issue might already be fixed by the drop SDL branch which you can try in a slightly modified form here:
https://github.com/FernetMenta/xbmc

In case you want to fix this for the current version, I would suggest that you filter the event in here:
https://github.com/xbmc/xbmc/blob/master/xbmc/windowing/WinEventsSDL.cpp#L364

Note that it may desired to have the windowed size the same as full screen.

Member

FernetMenta commented Aug 1, 2013

what I meant is that application should not bothered with special cases going just wrong on a single platform. I think the reason for this issue is SDL. It operates with three windows and WinEventsSDL may catch a ConfigureNotify event from an invisible one.

This issue might already be fixed by the drop SDL branch which you can try in a slightly modified form here:
https://github.com/FernetMenta/xbmc

In case you want to fix this for the current version, I would suggest that you filter the event in here:
https://github.com/xbmc/xbmc/blob/master/xbmc/windowing/WinEventsSDL.cpp#L364

Note that it may desired to have the windowed size the same as full screen.

@nikolai-r

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 1, 2013

Contributor

I do agree it was just a quick fix and that a case where it would be desired to have the resolution the same as the fullscreen while not in fullscreen is possible. It was less likely then the resize problem that currently existed. It would always be better to solve the root of the problem then filtering it out anyways.

This branch does indeed solve the problem.

Contributor

nikolai-r commented Aug 1, 2013

I do agree it was just a quick fix and that a case where it would be desired to have the resolution the same as the fullscreen while not in fullscreen is possible. It was less likely then the resize problem that currently existed. It would always be better to solve the root of the problem then filtering it out anyways.

This branch does indeed solve the problem.

@nikolai-r

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 2, 2013

Contributor

Reverted and moved to CWinEventsSDL::MessagePump as Fernet mentioned the problem was likely SDL and after a quick test this proved correct.

Contributor

nikolai-r commented Aug 2, 2013

Reverted and moved to CWinEventsSDL::MessagePump as Fernet mentioned the problem was likely SDL and after a quick test this proved correct.

@FernetMenta

View changes

xbmc/windowing/WinEventsSDL.cpp
- ret |= g_application.OnEvent(newEvent);
- g_windowManager.MarkDirty();
- break;
+ if (!g_advancedSettings.m_fullScreen) {

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Aug 2, 2013

Member

we use gnu style: curly brackets next line, then two white space indentation.

I think CGraphicContext::IsFullScreenRoot() is better than using advanced settings helper flag.
EDIT: or even better g_windowing.IsFullScreen

@FernetMenta

FernetMenta Aug 2, 2013

Member

we use gnu style: curly brackets next line, then two white space indentation.

I think CGraphicContext::IsFullScreenRoot() is better than using advanced settings helper flag.
EDIT: or even better g_windowing.IsFullScreen

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 2, 2013

Contributor

changed as per request

On Fri, Aug 2, 2013 at 12:21 PM, Rainer Hochecker
notifications@github.comwrote:

In xbmc/windowing/WinEventsSDL.cpp:

@@ -363,13 +365,18 @@ bool CWinEventsSDL::MessagePump()
}
case SDL_VIDEORESIZE:
{

  •    XBMC_Event newEvent;
    
  •    newEvent.type = XBMC_VIDEORESIZE;
    
  •    newEvent.resize.w = event.resize.w;
    
  •    newEvent.resize.h = event.resize.h;
    
  •    ret |= g_application.OnEvent(newEvent);
    
  •    g_windowManager.MarkDirty();
    
  •    break;
    
  •     if (!g_advancedSettings.m_fullScreen) {
    

we use gnu style: curly brackets next line, then two white space
indentation.

I think CGraphicContext::IsFullScreenRoot() is better than using advanced
settings helper flag.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004/files#r5556515
.

@nikolai-r

nikolai-r Aug 2, 2013

Contributor

changed as per request

On Fri, Aug 2, 2013 at 12:21 PM, Rainer Hochecker
notifications@github.comwrote:

In xbmc/windowing/WinEventsSDL.cpp:

@@ -363,13 +365,18 @@ bool CWinEventsSDL::MessagePump()
}
case SDL_VIDEORESIZE:
{

  •    XBMC_Event newEvent;
    
  •    newEvent.type = XBMC_VIDEORESIZE;
    
  •    newEvent.resize.w = event.resize.w;
    
  •    newEvent.resize.h = event.resize.h;
    
  •    ret |= g_application.OnEvent(newEvent);
    
  •    g_windowManager.MarkDirty();
    
  •    break;
    
  •     if (!g_advancedSettings.m_fullScreen) {
    

we use gnu style: curly brackets next line, then two white space
indentation.

I think CGraphicContext::IsFullScreenRoot() is better than using advanced
settings helper flag.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004/files#r5556515
.

@MartijnKaijser

This comment has been minimized.

Show comment Hide comment
@MartijnKaijser

MartijnKaijser Aug 2, 2013

Member

could you please squash all commits to keep a clean merge history

Member

MartijnKaijser commented Aug 2, 2013

could you please squash all commits to keep a clean merge history

@FernetMenta

View changes

xbmc/windowing/WinEventsSDL.cpp
@@ -363,6 +365,12 @@ bool CWinEventsSDL::MessagePump()
}
case SDL_VIDEORESIZE:
{
+ if(!g_graphicsContext.IsFullScreenRoot())
+ {
+ if((event.resize.w == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iWidth) &&

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Aug 2, 2013

Member

did you try the maximize button on the window? does it still work?

@FernetMenta

FernetMenta Aug 2, 2013

Member

did you try the maximize button on the window? does it still work?

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 2, 2013

Contributor

yes it does, this would only fail on the condition that the platform it is
running on has borderless windows and no panel/taskbar.

On Fri, Aug 2, 2013 at 1:59 PM, Rainer Hochecker
notifications@github.comwrote:

In xbmc/windowing/WinEventsSDL.cpp:

@@ -363,6 +365,12 @@ bool CWinEventsSDL::MessagePump()
}
case SDL_VIDEORESIZE:
{

  •    if(!g_graphicsContext.IsFullScreenRoot())
    
  •    {
    
  •      if((event.resize.w == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iWidth) &&
    

did you try the maximize button on the window? does it still work?


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004/files#r5558706
.

@nikolai-r

nikolai-r Aug 2, 2013

Contributor

yes it does, this would only fail on the condition that the platform it is
running on has borderless windows and no panel/taskbar.

On Fri, Aug 2, 2013 at 1:59 PM, Rainer Hochecker
notifications@github.comwrote:

In xbmc/windowing/WinEventsSDL.cpp:

@@ -363,6 +365,12 @@ bool CWinEventsSDL::MessagePump()
}
case SDL_VIDEORESIZE:
{

  •    if(!g_graphicsContext.IsFullScreenRoot())
    
  •    {
    
  •      if((event.resize.w == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iWidth) &&
    

did you try the maximize button on the window? does it still work?


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004/files#r5558706
.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Aug 2, 2013

Member

if you add a comment to the code why this is needed and squash your commits, I am fine with it.

@elupus @davilla what do you think?

Member

FernetMenta commented Aug 2, 2013

if you add a comment to the code why this is needed and squash your commits, I am fine with it.

@elupus @davilla what do you think?

@nikolai-r

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 2, 2013

Contributor

commented and squashed

On Fri, Aug 2, 2013 at 2:49 PM, Rainer Hochecker
notifications@github.comwrote:

if you add a comment to the code why this is needed and squash your
commits, I am fine with it.

@elupus https://github.com/elupus @davilla https://github.com/davillawhat do you think?


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004#issuecomment-22025602
.

Contributor

nikolai-r commented Aug 2, 2013

commented and squashed

On Fri, Aug 2, 2013 at 2:49 PM, Rainer Hochecker
notifications@github.comwrote:

if you add a comment to the code why this is needed and squash your
commits, I am fine with it.

@elupus https://github.com/elupus @davilla https://github.com/davillawhat do you think?


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004#issuecomment-22025602
.

@elupus

View changes

xbmc/windowing/WinEventsSDL.cpp
+ {
+ if((event.resize.w == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iWidth) &&
+ (event.resize.h == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iHeight))
+ break;

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Aug 4, 2013

Member

Resolution can be different for different screens. You need to get the resolution for the active screen.

@elupus

elupus Aug 4, 2013

Member

Resolution can be different for different screens. You need to get the resolution for the active screen.

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Aug 4, 2013

Member

g_Windowing.DesktopResolution(g_Windowing.GetCurrentScreen()) returns the resolution index.

@elupus

elupus Aug 4, 2013

Member

g_Windowing.DesktopResolution(g_Windowing.GetCurrentScreen()) returns the resolution index.

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 4, 2013

Contributor

done, let me know if all is acceptable and I will squash again for merge

On Sun, Aug 4, 2013 at 2:01 PM, Joakim Plate notifications@github.comwrote:

In xbmc/windowing/WinEventsSDL.cpp:

@@ -363,6 +364,15 @@ bool CWinEventsSDL::MessagePump()
}
case SDL_VIDEORESIZE:
{

  •    // Under linux returning from fullscreen, SDL sends an extra event to resize to the desktop
    
  •    // resolution causing the previous window dimensions to be lost. This is needed to rectify
    
  •    // that problem.
    
  •    if(!g_Windowing.IsFullScreen())
    
  •    {
    
  •      if((event.resize.w == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iWidth) &&
    
  •          (event.resize.h == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iHeight))
    
  •        break;
    

g_Windowing.DesktopResolution(g_Windowing.GetCurrentScreen()) returns the
resolution index.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004/files#r5570115
.

@nikolai-r

nikolai-r Aug 4, 2013

Contributor

done, let me know if all is acceptable and I will squash again for merge

On Sun, Aug 4, 2013 at 2:01 PM, Joakim Plate notifications@github.comwrote:

In xbmc/windowing/WinEventsSDL.cpp:

@@ -363,6 +364,15 @@ bool CWinEventsSDL::MessagePump()
}
case SDL_VIDEORESIZE:
{

  •    // Under linux returning from fullscreen, SDL sends an extra event to resize to the desktop
    
  •    // resolution causing the previous window dimensions to be lost. This is needed to rectify
    
  •    // that problem.
    
  •    if(!g_Windowing.IsFullScreen())
    
  •    {
    
  •      if((event.resize.w == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iWidth) &&
    
  •          (event.resize.h == CDisplaySettings::Get().GetResolutionInfo(RES_DESKTOP).iHeight))
    
  •        break;
    

g_Windowing.DesktopResolution(g_Windowing.GetCurrentScreen()) returns the
resolution index.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/3004/files#r5570115
.

@nikolai-r

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 6, 2013

Contributor

@FernetMenta @elupus @davilla everyone ok with this then? if so i will squash again

Contributor

nikolai-r commented Aug 6, 2013

@FernetMenta @elupus @davilla everyone ok with this then? if so i will squash again

@nikolai-r

This comment has been minimized.

Show comment Hide comment
@nikolai-r

nikolai-r Aug 27, 2013

Contributor

squashed, ready for merge if accepted

Contributor

nikolai-r commented Aug 27, 2013

squashed, ready for merge if accepted

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Aug 27, 2013

Contributor

I'm fine, we can always fix any issues that pop up regarding osx.

Contributor

davilla commented Aug 27, 2013

I'm fine, we can always fix any issues that pop up regarding osx.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Aug 27, 2013

Member

I'm fine too

Member

FernetMenta commented Aug 27, 2013

I'm fine too

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Aug 27, 2013

Contributor

/me summons @MartijnKaijser

Contributor

davilla commented Aug 27, 2013

/me summons @MartijnKaijser

@MartijnKaijser

This comment has been minimized.

Show comment Hide comment
@MartijnKaijser

MartijnKaijser Aug 27, 2013

Member

Can't push the button yourself? It's a fix right :)

Member

MartijnKaijser commented Aug 27, 2013

Can't push the button yourself? It's a fix right :)

davilla added a commit that referenced this pull request Aug 27, 2013

Merge pull request #3004 from nikolai-r/master
Fix return from fullscreen for linux/gnome not returning to previous size.

@davilla davilla merged commit c4622ca into xbmc:master Aug 27, 2013

+ // that problem.
+ if(!g_Windowing.IsFullScreen())
+ {
+ int RES_SCREEN = g_Windowing.DesktopResolution(g_Windowing.GetCurrentScreen());

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Sep 1, 2013

Member

Hmm.. RES_SCREEN. I really thought that was a macro first due to the uppercase.

@elupus

elupus Sep 1, 2013

Member

Hmm.. RES_SCREEN. I really thought that was a macro first due to the uppercase.

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