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

Replace iOS-GPX-Framework with CoreGPX #65

Merged
merged 14 commits into from
Oct 28, 2023

Conversation

2kai2kai2
Copy link
Contributor

@2kai2kai2 2kai2kai2 commented Oct 17, 2023

Resolves #64

Changes

Essentially, this eliminates the need for cocoapods and cocoapods-patch
We do the following:

  1. Replace iOS-GPX-Framework (Obj-C, cocoapods) with CoreGPX (Swift, Swift Package Manager)
  2. Replace custom extensions implementation (Obj-C) in apps/ios/patches/iOS-GPX-Framework+0.0.2.diff with implementation (Swift) in apps/ios/GuideDogs/Code/App/Framework Extensions/Geo Extensions/GPXExtensions.swift (the previous version used a patch on the Obj-C extension, which also made it a more complex dependency system)
  3. Remove all remaining references to cocoapods and the Pods-soundscape build

In the new implementation of our custom GPX extensions, I've implemented them as views rather than intercepting and adding custom parsing for GPX elements. This system may be slightly less efficient due to accessing the extension's GPX tree, but this should not be a major issue since our main use case (loading AuthoredActivityContent) can discard the GPX object after loading. Additionally, it has the advantage of simplifying and reducing the surface area with the dependency.

Testing

This pull request excludes the automated testing which can be seen here: https://github.com/2kai2kai2/soundscape
Said testing (more so the build systems) does not seem to be stable with the previous legacy dependencies.

I intend to open a second, separate pull request for the full testing system after this, but can combine them if that is preferred.

EDIT: XCTest is now included in this pull request.

@steinbro steinbro self-assigned this Oct 19, 2023
Copy link
Member

@steinbro steinbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Really appreciate your attention to detail in updating the docs as well. I've left a few comments for you to address, largely because I think at this point you understand how this component fits into the larger codebase better than I do.

docs/ios-client/onboarding.md Outdated Show resolved Hide resolved
docs/ios-client/onboarding.md Outdated Show resolved Hide resolved
@2kai2kai2
Copy link
Contributor Author

2kai2kai2 commented Oct 24, 2023

Just overwrote with the full changes including tests. There are a copy of these tests running here: https://github.com/2kai2kai2/soundscape/actions/runs/6632231863/job/18017464213

It looks like the tests did not pass due to the AudioEngine test. Not sure why, but overall the fact they are running is good news.

@steinbro
Copy link
Member

Once the tests are passing, your expertise would be helpful in getting Fastlane working again (#58). Probably for a separate pull request.

Basic tests setup. Currently has just one demo test.
Tests for the main location-related sensor manager class
Needs audio beacon tests
- Also include code coverage
It's currently in the wrong place.
Successfully builds and passes tests
(though no tests use the GPX system)

This is a major step towards resolving issue soundscape-community#64
- Removed `cocoapods` and `cocoapods-patch`
- Replaced the outdated cocoapods dependency `iOS-GPX-Framework` (Objective-C) with `CoreGPX` (Swift, Swift Package Manager)
- Rewrote GPX Extensions in Swift from Objective-C (note: if there are GPX bugs, look here first)
@2kai2kai2
Copy link
Contributor Author

Test issue seems to be due to the lack of installed TTS voices which are used in testing. Nevertheless, it has been partially resolved by the inclusion of updated service URLs in main.

I believe this pull request is ready to merge.

@steinbro
Copy link
Member

Thanks! Amazing work. Do you have any idea what's going on with the TTS voices in the simulator? I noticed this even before your PR -- I also don't seem to have any iOS voices installed in my simulator, and the Settings app is crashing immediately after launch. Is this a bug with the new iOS 17 simulator?

@steinbro steinbro merged commit 37c54b6 into soundscape-community:main Oct 28, 2023
1 check passed
@steinbro steinbro mentioned this pull request Oct 28, 2023
@2kai2kai2 2kai2kai2 deleted the GPX branch October 29, 2023 07:21
@steinbro steinbro mentioned this pull request Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency cocoapods-patch incompatible with newest version of cocoapods
2 participants