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

Migrate LocationHistoryList to SwiftUI #3468

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

banaslee
Copy link

@banaslee banaslee commented Feb 27, 2025

Summary

In this PR I set to migrate the LocationHistoryList to SwiftUI, to reduce the dependency in Eureka. This screen was picked at random.

As this screen is presented from two different places, SettingsDetail and Debug screens, also updated the code in the corresponding classes.

The LocationHistory list reacts to changes.

LocationHistoryDetail

LocationHistoryDetailViewController also gained a SwiftUI wrapper in order for it to be presented from the new LocationHistoryListView.
This wrapper syncs the navigation items between the wrapped View Controller and the parent.
Move functionality also got migrated.

Misc changes

  • Support for M4 added to the Gemfile.lock (added automatically)
  • New extension for safe subscripting in arrays added.

Screenshots

LocationHistoryList item
Screenshot 2025-02-27 at 23 39 32

Empty LocationHistoryList
Screenshot 2025-02-27 at 23 40 56

Present it from SettingsDetail and Debug screens;
Add wrapper to LocationHistoryDetailViewController.
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @banaslee

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft February 27, 2025 23:41
@banaslee banaslee marked this pull request as ready for review February 27, 2025 23:43
@banaslee banaslee marked this pull request as draft February 27, 2025 23:50
@banaslee
Copy link
Author

Moving back to draft as the Move functionality was missed.

…nHistoryDetail;

Add syncing between navigation items;
Remove unnecessary Eureka import and TypedRowControllerType conformance.
@banaslee banaslee marked this pull request as ready for review February 28, 2025 01:53
@banaslee
Copy link
Author

Move functionality has been restored. Works differently than before by reusing the same LocationHistoryDetailViewController instance and updating the entry and, consequently, the UI.

…screen to avoid moveDelegate from being released too early;

Pass AppEnvironment so LocationHistoryDetail can observe changes to the data set.
@home-assistant home-assistant bot marked this pull request as draft February 28, 2025 10:44
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.33%. Comparing base (6c34210) to head (9e20c5a).
Report is 549 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3468       +/-   ##
===========================================
+ Coverage   27.54%   45.33%   +17.78%     
===========================================
  Files         311      221       -90     
  Lines       31699    13309    -18390     
===========================================
- Hits         8733     6033     -2700     
+ Misses      22966     7276    -15690     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bgoncal
Copy link
Member

bgoncal commented Feb 28, 2025

Thanks for your contribution, I'll run the app today to give it a try, but in general it looks good, are you going to migrate LocationHistoryDetailViewController afterwards?

Also, sorry for the timin, but I am leaving on vacations until March 13th, so if you don't hear from me, thats the reason.

@bgoncal
Copy link
Member

bgoncal commented Feb 28, 2025

For linting bundle exec fastlane autocorrect

@banaslee
Copy link
Author

Thanks for your contribution, I'll run the app today to give it a try, but in general it looks good, are you going to migrate LocationHistoryDetailViewController afterwards?

Yes, that's my plan. It's not clear how the map support is in SwiftUI for minimum supported version but will see when I get there. I don't think I'll be blocked.

I ran it in the iOS and iPad simulators against my instance and looked good. Can you please validate if the current location annotation stays after moving between entries?

Also, sorry for the timin, but I am leaving on vacations until March 13th, so if you don't hear from me, thats the reason.

No worries. Enjoy your time off.

@banaslee banaslee marked this pull request as ready for review February 28, 2025 15:07
@home-assistant home-assistant bot requested a review from bgoncal February 28, 2025 15:07
@banaslee
Copy link
Author

@bgoncal applied your requested changes plus improved the previews by introducing more mock data and names.

@bgoncal
Copy link
Member

bgoncal commented Feb 28, 2025

Can you add a small test for the safe subscript extension you added? Also one snapshot test for the view, I started with snapshot tests very recently so probably the only place you will find example is WidgetsSnapshotTests

Im running on my phone now and it all looks good, I will let you know in case I spot something

Rearranged Utilities tests;
Add missing super call in ClientEventTests
@banaslee
Copy link
Author

banaslee commented Feb 28, 2025

@bgoncal I'm going to push snapshots in light mode for now.

How interesting would it be if I pushed a change in a new PR to always generate and assert in dark mode as well?

@bgoncal
Copy link
Member

bgoncal commented Feb 28, 2025

That's would be nice! We should aim for that indeed

@banaslee
Copy link
Author

banaslee commented Mar 1, 2025

Heads up @bgoncal, the test may need to be updated to use the latest SDK version, 18.2. At least it's failing to find a valid platform on the CI

[20:59:20]: ▸ xcodebuild: error: Unable to find a destination matching the provided destination specifier:
[20:59:20]: ▸ 		{ platform:iOS Simulator, OS:18.1, name:iPhone 16 }
[20:59:20]: ▸ 	Available destinations for the "Tests-Unit" scheme:
[20:59:20]: ▸ 		{ platform:macOS, arch:arm64, variant:Mac Catalyst, id:0000FE00-0D2881097313F0FA, name:My Mac }
[20:59:20]: ▸ 		{ platform:macOS, arch:x86_64, variant:Mac Catalyst, id:0000FE00-0D2881097313F0FA, name:My Mac }
[20:59:20]: ▸ 		{ platform:macOS, variant:Mac Catalyst, name:Any Mac }
[20:59:20]: ▸ 	Ineligible destinations for the "Tests-Unit" scheme:
[20:59:20]: ▸ 		{ platform:iOS, id:dvtdevice-DVTiPhonePlaceholder-iphoneos:placeholder, name:Any iOS Device, error:iOS 18.2 is not installed. To use with Xcode, first download and install the platform }

banaslee added 2 commits March 1, 2025 14:03
- update OS to the current base version (18.2)
- change assertion in ClientEventTests to address flakyness
@banaslee
Copy link
Author

That's would be nice! We should aim for that indeed

Cool, once we merge this one I have two more lined up with improvements to the snapshot tests tooling.

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

Successfully merging this pull request may close these issues.

2 participants