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

Add CarPlay Actions and general improvements #2517

Merged
merged 24 commits into from
Jan 16, 2024
Merged

Conversation

bgoncal
Copy link
Member

@bgoncal bgoncal commented Jan 9, 2024

Summary

  • Replace CarPlay navigation by tabs
  • Add iOS Actions

Screenshots

Screenshot 2024-01-09 at 15 12 21

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

@bgoncal bgoncal requested a review from zacwest January 9, 2024 14:13
@bgoncal bgoncal self-assigned this Jan 9, 2024
@bgoncal bgoncal marked this pull request as ready for review January 9, 2024 14:14
@bgoncal bgoncal force-pushed the carplay-actions-2 branch from de9b5ff to 2682987 Compare January 9, 2024 14:31
Sources/App/Scenes/CarPlaySceneDelegate.swift Outdated Show resolved Hide resolved
HomeAssistant.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
Sources/App/Scenes/CarPlaySceneDelegate.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CPServerListTemplate.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CPServerListTemplate.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CPActionsTemplate.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CPActionsTemplate.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CPActionsTemplate.swift Outdated Show resolved Hide resolved
@home-assistant
Copy link

home-assistant bot commented Jan 9, 2024

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 January 9, 2024 15:40
@bgoncal bgoncal marked this pull request as ready for review January 10, 2024 10:24
@home-assistant home-assistant bot requested a review from zacwest January 10, 2024 10:24
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 591 lines in your changes are missing coverage. Please review.

Comparison is base (111fc2f) 27.96% compared to head (0e223b3) 27.73%.

❗ Current head 0e223b3 differs from pull request most recent head 923e004. Consider uploading reports for the commit 923e004 to get more accurate results

Files Patch % Lines
...Vehicle/Templates/CarPlayDomainsListTemplate.swift 0.00% 94 Missing ⚠️
.../Vehicle/Templates/CarPlayAreasZonesTemplate.swift 0.00% 90 Missing ⚠️
...hicle/Templates/CarPlayPaginatedListTemplate.swift 0.00% 77 Missing ⚠️
...ehicle/Templates/CarPlayEntitiesListTemplate.swift 0.00% 76 Missing ⚠️
...ces/Vehicle/Templates/CarPlayActionsTemplate.swift 0.00% 65 Missing ⚠️
.../Vehicle/Templates/CarPlayServerListTemplate.swift 0.00% 59 Missing ⚠️
...es/Shared/Common/Extensions/HAEntity+CarPlay.swift 0.00% 49 Missing ⚠️
Sources/App/Scenes/CarPlaySceneDelegate.swift 0.00% 27 Missing ⚠️
...rces/Shared/CarPlay/Responses/HAAreaResponse.swift 0.00% 24 Missing ⚠️
...urces/Vehicle/Templates/CarPlayNoServerAlert.swift 0.00% 13 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2517      +/-   ##
==========================================
- Coverage   27.96%   27.73%   -0.23%     
==========================================
  Files         291      298       +7     
  Lines       30951    31227     +276     
==========================================
+ Hits         8655     8661       +6     
- Misses      22296    22566     +270     

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

Copy link
Member

@zacwest zacwest left a comment

Choose a reason for hiding this comment

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

mostly for this i think the scene delegate needs a little bit of abstraction (making the providers an array, perhaps). otherwise i think this is definitely in the right direction.

Sources/App/Scenes/CarPlaySceneDelegate.swift Outdated Show resolved Hide resolved
Sources/Shared/API/ServerManager.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CarPlayTemplateProvider.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CarPlayActionsTemplate.swift Outdated Show resolved Hide resolved
Sources/Vehicle/Templates/CarPlayActionsTemplate.swift Outdated Show resolved Hide resolved
Sources/App/Scenes/CarPlaySceneDelegate.swift Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft January 11, 2024 04:28
Sources/App/Scenes/CarPlaySceneDelegate.swift Outdated Show resolved Hide resolved

// Prevent unecessary update and UI glitch for non-touch screen CarPlay
let entitiesIds = entitiesSorted.map(\.entityId).sorted()
guard entitiesIdsCurrentlyInList != entitiesIds else {
Copy link
Member

Choose a reason for hiding this comment

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

does this ever match when pagination is required?

i think it may be easier to match the entities here in a dictionary to a CPListItem, e.g.

// at a class level
var entityToListItem: [String: CPListItem]

// in this method
for entity in entities {
  let listItem: CPListItem
  if let existing = entityToListItem[entity.entityId] {
    listItem = existing
  } else {
    listItem = CPListItem()
    entityToListItem[entity.entityId] = listItem
  }
  // do updates / configure

}

i think you would need to move the "and change the items displayed" into the paginator to really invalidate/reset the template

Copy link
Member Author

Choose a reason for hiding this comment

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

That is sort of what is happening already, the the list of entityIds change it means that some entity was added or removed, which means the paginated list needs an update, otherwise if the entityIds list is the same it means the state of an or multiple entities were updated, then those specific items are updated IF they are visible at that moment.

in CPListTemplate there is also not the concept of inserting a new item, so when we have a new entity we have to update the whole section

Copy link
Member

Choose a reason for hiding this comment

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

I see the intent but I think pagination is making this "if" never work for cases where pagination happens at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

This check happens before everything is paginated, it's goal is just to prevent to recreate the list when the only thing that has changed was the state of entities. Let's say a new entity is added to your HA instance, then entitiesIds will have one more item and therefore different from entitiesIdsCurrentlyInList (previously set), which then will lead to the list being recreated.

Sources/Vehicle/Templates/CarPlayServerListTemplate.swift Outdated Show resolved Hide resolved
Sources/App/Scenes/CarPlaySceneDelegate.swift Show resolved Hide resolved
@bgoncal bgoncal marked this pull request as ready for review January 15, 2024 11:04
@home-assistant home-assistant bot requested a review from zacwest January 15, 2024 11:04
@bgoncal
Copy link
Member Author

bgoncal commented Jan 15, 2024

Current state
Screenshot 2024-01-15 at 12 05 39

@bgoncal bgoncal marked this pull request as draft January 15, 2024 13:35
@bgoncal bgoncal marked this pull request as ready for review January 15, 2024 14:33
@bgoncal bgoncal merged commit 135b764 into master Jan 16, 2024
5 checks passed
@bgoncal bgoncal deleted the carplay-actions-2 branch January 16, 2024 20:23
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