Skip to content
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

Changed WinEventsSDL.cpp to better support Mac Native Full Screen #10681

Merged
merged 1 commit into from Oct 15, 2016

Conversation

@YairSc
Copy link
Contributor

commented Oct 11, 2016

Description

A Linux code check should not run on Mac - see below

Motivation and Context

On Mac, when switching to Full screen using the native Mac method, the content of Kodi does not resize to fill the screen.

How Has This Been Tested?

Tested on my Mac

Screenshots (if appropriate):

Windowed:
screen shot 2016-10-11 at 21 09 58

Mac native full screen with bug:
screen shot 2016-10-11 at 21 10 26

Mac native full screen after fix:
screen shot 2016-10-11 at 21 11 55

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

Thx for your contribution. The commits of this pull request look messed up. I assume only the added ifdef is the real change? If so - just completly remove that code part. Osx is the only platform using sdl atm so no ifdef required. Would be great if you could manage to fix this pull request so that it only has the 1 needed commit in it. You can fix it on your local repo and then simply force push to your remote repo which will automarically update this pull request...
On which osx version did you test this?

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

@MartijnKaijser we need something like mentionbot in the future - only saw this pr by accident...

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

@Memphiz yep i know but there's a bug in mention-bot and i already saw this PR and building one for @keithah to test as he has similar issue

@Rechi
Rechi approved these changes Oct 12, 2016
@YairSc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2016

@Rechi does the fact you approved means it will get merged automatically, or do I need to manually do something? Also, @Memphiz suggested I redo the commits as they ended up a bit messy, plus he suggested removing the code altogether rather than #ifdef'ing it out. Should I make those changes - it if's already merged, then there's not much point.

@YairSc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2016

Off topic - how do I update my fork (YairSc/xbmc) to the main xbmc/xbmc? Are they on the same repository? couldn't get 'git rebase' to work... thanks.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

Well i am confused too about that new "approved" github Feature tbh. I am not yet sure what it means if someone hits that button ;). For this PR it means nothing yet. It will not be automatically merged or something. Also i want to have the commit mess solved before considering a merge.

to fix your PR (using git from the terminal):
$ cd < your working Directory >
$ git remote add xbmc git://github.com/xbmc/xbmc.git -f
$ git reset --hard xbmc/upstream

Now you are on the master of our xbmc repo. Please do the Change in WinEventsSDL.cpp (removing that code) and save the changes

$ git commit -a -m "[OSX/WinEventsSDL] - Mac supports native full screen mode - it enlarges the application to full screen in a new screen. This is possibly more convenient than Kodi's full screen mode, which changes resolution, frame rate, etc.
However, the code has some check related to Linux causing the resize event to be ignored when switching to a window size equal to the screen size. This should not be done on Mac."
$ git push origin master

After that this PR should Show only once commit with the wanted change

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

Ha - just realised that Rechi is not member of Team-Kodi. So its just a bad joke that he approves anything here (and just shows how stupid that github Feature is implemented). You could aswell approve some pull requests in the Linux github repo if you want ...

@Rechi

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

@Memphiz I just wanted to let you know it has fixed the issue for me.
I think if a teammember approves something the check mark is green otherwise grey.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

@Rechi i think i only see green checkmarks atm. Also i don't even find the approve button anymore. Seems the fact that you approved makes the Feature invisible to me somehow.

Also please let me know your osx Version you tested on please (same for @YairSc )

@Rechi

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

@Memphiz tested with osx 10.11.6

Mac supports native full screen mode - it enlarges the application to full screen in a new screen. This is possibly more convenient than Kodi's full screen mode, which changes resolution, frame rate, etc.
However, the code has some check related to Linux causing the resize event to be ignored when switching to a window size equal to the screen size. This should not be done on Mac.
@YairSc YairSc force-pushed the YairSc:master branch from 807e77c to b339291 Oct 12, 2016
@YairSc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2016

@Memphiz - cool! I wasn't familiar with 'git remote add'.
Did all the work for this pull request.
I tried my build on macOS 10.12

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 12, 2016

Will try on osx 10.9

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 14, 2016

Mhhh my tests are inconclusive because on my rig (1600x900) windowed mode seems b0rked with and without that change.

@YairSc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2016

@Memphiz Can you please attach some screenshots?
Also, just to make sure - you switch to full screen with the little button on the window top left (next to the 'x' for quit and '-' for minimize), not using the ⌘F keys, right?

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

@YairSc well first i need to go into windowed mode of course to even have the title bar. And it simply doesn't resize the framebuffer. The window is there and i still have the fullscreenfarmebuffer in it (which means gui is cut off). I don't think that this PR will do anything bad or good to this. So i don't really care in the end. Its broken as is imo and it will be with this PR too (which has some other intentions anyway).

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

jenkins build and merge

@jenkins4kodi jenkins4kodi merged commit b708a79 into xbmc:master Oct 15, 2016
2 of 4 checks passed
2 of 4 checks passed
default I've found some spare time so building this now
Details
jenkins4kodi Yeah yeah I'll get to it when i have some time
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hudokkow hudokkow added this to the Krypton 17.0-beta4 milestone Oct 15, 2016
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

going back from fullscreen to windowed mode does still not work here on my MacBook.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

Our fullscreen or the osx fullscreen feature this pr is about?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

start kodi windowed or switch to windowed mode, go full screen, then back to windowed mode. osx window is much too big in size. as a result Kodi is not usable. it is kind of hard to resize this window to a proper size. then things are back to normal.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

Yeah - and do it a second time and the window might be fine. Thats what i see too - but thats not related omthe osx fullscreen mode. Its something else - just screw that sdl shit - not the nerves to dig into something i don't understand at all.

@YairSc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2016

I'll try to have a look at those mode switching and see if I can resolve any of those

@Memphiz

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

Cool @YairSc that would be awesome - thx :)

@Memphiz

This comment has been minimized.

Copy link
Member

commented on b339291 Dec 3, 2016

just noticed that this is indeed needed on osx too. Now exactly the issue that was described in the comment is showing up here. You go from windowed to fullscreen via cmd + f and when you go back from fullscreen to windowed the window size is screwed up (its even bigger then my screen). We need a different approach for the native osx fullscreen i fear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.