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

Fix ios9 #15658

Merged
merged 4 commits into from Mar 17, 2019

Conversation

Projects
None yet
3 participants
@Memphiz
Copy link
Member

commented Mar 3, 2019

  1. Fixes a regression for 32bit jailbroken devices (needs old ldid for fake
  2. Fixes a regression for some older devices / iOS combinations were eagl bounds does indeed return the non rotated bounds. (The old tvout fix resulted in wrong touch coordinates on those devices.

Memphiz added some commits Mar 1, 2019

Revert "[ios/WinSystem] - instead of using the eaglbounds for determi…
…ning the size of the internal screen - simply use the scerensize from UIScreen and exchange width with height (because the screen is installed 90° rotated). This fixes traversing back from external to internal screen which resulted in getting the eaglbounds from the external screen * screenscale from the internal (and lead to non-working touch input)"

This reverts commit acf176f.
[ios/windowing] - cache internal touch screen resolution because when…
… going from external to internal screen - display settings want to know these before actually switching to internal. As we rely on eaglbounds we can't tell the internal resolution before switching - so we have to give back the cached results. - fixes touch screen issues due to wrong resolution for older ios9 32bit devices like ipad2 - while still maintaining the fix for external screens
LDID64="$NATIVEPREFIX/bin/ldid64"
LDID=${LDID32}

if [ "${CURRENT_ARCH}" == "arm64" ]; then

This comment has been minimized.

Copy link
@Rechi

Rechi Mar 3, 2019

Member

CURRENT_ARCH isn't defined anywhere. This will break fake signing arm64 packages again, as the else branch is always executed.

This comment has been minimized.

Copy link
@Memphiz

Memphiz Mar 3, 2019

Author Member

This script is an Xcode build phase script. Xcode sets this.

This comment has been minimized.

Copy link
@Rechi

Rechi Mar 3, 2019

Member

Look into jenkins log, it's not set.

This comment has been minimized.

Copy link
@Memphiz

Memphiz Mar 3, 2019

Author Member

Mhh i guess you mean in the case were Makefiles are used? Is this the case on Jenkins? If so whatever sets WRAPPER_NAME (which is also normally set by Xcode) needs to set CURRENT_ARCH aswell.

This comment has been minimized.

Copy link
@Rechi

Rechi Mar 3, 2019

Member

Yes jenkins uses the cmake makefile generator.
If you mean the following it supports makefiles and xcode.

if(CMAKE_GENERATOR MATCHES "Unix Makefiles" OR CMAKE_GENERATOR STREQUAL Ninja)
set(wrapper_obj cores/dll-loader/exports/CMakeFiles/wrapper.dir/wrapper.c.o)
elseif(CMAKE_GENERATOR MATCHES "Xcode")
set(wrapper_obj cores/dll-loader/exports/kodi.build/$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)/wrapper.build/Objects-$(CURRENT_VARIANT)/$(CURRENT_ARCH)/wrapper.o)
else()
message(FATAL_ERROR "Unsupported generator in core_link_library")
endif()

This comment has been minimized.

Copy link
@Memphiz

Memphiz Mar 3, 2019

Author Member

Why again do we use a different build system on Jenkins then during development? When using Xcode for development something like this is very likely to stay unnoticed for a while. In my opinion this is suboptimal. We used xcodebuild before cmake introduction - but now it’s Makefiles I guess.

This comment has been minimized.

Copy link
@Memphiz

Memphiz Mar 3, 2019

Author Member

That’s not what I meant - in the codesign script the variable WRAPPER_NAME is used. Something in cmake must set this for the Makefiles target - I will figure it out. Thx for catching that - would have been a pita to have this unnoticed.

This comment has been minimized.

Copy link
@Rechi

Rechi Mar 3, 2019

Member

"WRAPPER_NAME=${APP_NAME}.app"

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

@Rechi that last commit should work - what do you think?

@Rechi

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Yes should work, because of the following code.

if(WITH_ARCH)
set(ARCH ${WITH_ARCH})
else()
if(CPU STREQUAL armv7)
set(CMAKE_OSX_ARCHITECTURES ${CPU})
set(ARCH arm)
set(NEON True)
elseif(CPU STREQUAL arm64)
set(CMAKE_OSX_ARCHITECTURES ${CPU})
set(ARCH aarch64)
set(NEON True)
else()
message(SEND_ERROR "Unknown CPU: ${CPU}")
endif()
endif()

@Rechi

Rechi approved these changes Mar 3, 2019

Copy link
Member

left a comment

Depends and sigining changes are okay.
Can't say anything about the ios windowing code.

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

User confirmed on on ios9 32bit - I tested in iPad mini, iPhone se iOS 12.x with tvout to ensure the reverted scenario is fixed with the new fix aswell.

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

@MartijnKaijser may i merge?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Sure

@Memphiz

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

“Pull requests that have a failing status can’t be merged on a phone.” WTF

@Memphiz Memphiz merged commit 4ecc969 into xbmc:master Mar 17, 2019

1 check failed

default Sorry, building this PR failed. Please check the logs for the errors.
Details

@Memphiz Memphiz deleted the Memphiz:fix_ios9 branch Mar 17, 2019

@Rechi Rechi added this to the Leia 18.2-rc1 milestone Mar 17, 2019

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