Allow the use of multiple status banners by jill-cardamon · Pull Request #2747 · mapbox/mapbox-navigation-ios

jill-cardamon · 2020-11-24T15:01:03Z

Description

This PR allows the use of multiple status banners by refactoring StatusView.swift to manage the display statuses.

Create Status struct to represent a status, store them in a statuses array, and refactor as necessary
Refactor how statuses are shown and hidden by priority
How will we deal with special values associated with a Status? (e.g.: simulation status banner is interactive, allows for changes in simulation speed)
Fix #2723 (hide "enable precise location" banner when precise location is switched on)
Test (StatusViewTests.swift)
Update Changelog

Implementation

Screenshots or Gifs

1ec5 · 2020-11-25T16:35:57Z

CocoaPods integration tests are failing:

[!] CocoaPods could not find compatible versions for pod "Mapbox-iOS-SDK":
  In snapshot (Podfile.lock):
    Mapbox-iOS-SDK (= 6.2.2, ~> 6.0)

CocoaPods picked up the edit to v6.3.0 in mapbox/mapbox-gl-native-ios#549 (comment) but apparently not the earlier edit to v6.2.2 in mapbox/mapbox-gl-native-ios#549 (comment):

  CDN: trunk Relative path: Specs/a/5/9/Mapbox-iOS-SDK/6.3.0/Mapbox-iOS-SDK.podspec.json modified during this run! Returning local
  CDN: trunk Relative path downloaded: all_pods_versions_3_5_2.txt, save ETag: W/"5fbd8072-29e"

This is basically CocoaPods/CocoaPods#10229. We could work around it by forcing the CI job to start with a fresh cache, but at this point, it might be easier to update the integration test fixture by running these commands:

mapbox-navigation-ios/scripts/update-version.sh

Lines 35 to 37 in 72d7845

    
           cd MapboxCoreNavigationTests/CocoaPodsTest/PodInstall/ 
        
           pod update 
        
           cd -

MaximAlien · 2021-01-06T01:02:53Z

Is this PR ready to be reviewed (I can see that it's marked as Draft)? Also it'd be good to fix outstanding conflicts with main branch.

jill-cardamon · 2021-01-06T15:10:38Z

Yes. Ready to be reviewed. I have two concerns if you could address them in your review:

Do I delete method calls in other functions and add a deprecated flag to the definition only?
Advice on timer related issue: used a dispatch queue to make things happen when I want them to, but when I was testing, I realized that hiding (which uses a dispatch queue) takes a while. Not sure if it’s because of the loop I implemented or something else. But it takes a while for it to disappear (longer than the delay duration). I did not notice this issue in the example with the current statuses we use.

ShanMa1991 · 2021-01-06T20:04:44Z

        return navigationComponents.allSatisfy { $0.navigationServiceShouldDisableBatteryMonitoring(service) }
    }
-
+        


Nit: removing extra whitespaces and cleaning the code would be great.

ShanMa1991 · 2021-01-06T20:06:18Z

+    @available(*, deprecated, message: "Add a status using addNewStatus instead")
    public func showStatus(title: String, spinner spin: Bool = false, duration: TimeInterval, animated: Bool = true, interactive: Bool = false) {
-        show(title, showSpinner: spin, interactive: interactive)
+        // show(title, showSpinner: spin, interactive: interactive)


Nit: removing the unnecessary comment would be better.

MaximAlien · 2021-01-06T18:07:07Z

 PODFILE CHECKSUM: d4084fe664fbacd0cffd88ab45eaed1ad907ad1d

-COCOAPODS: 1.10.0
+COCOAPODS: 1.9.3


I think we should keep CocoaPods 1.10.0 as it's used on CircleCI as well.

MaximAlien · 2021-01-06T18:14:58Z

 		AED6285522CBE4CE00058A51 /* ViewController+GuidanceCards.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = "ViewController+GuidanceCards.swift"; path = "Example/ViewController+GuidanceCards.swift"; sourceTree = "<group>"; };
 		AEF2C8F12072B603007B061F /* routeWithTunnels_9thStreetDC.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = routeWithTunnels_9thStreetDC.json; sourceTree = "<group>"; };
-		B430D2F925534FDC0088CC23 /* UserHaloCourseView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UserHaloCourseView.swift; sourceTree = "<group>"; };
+		C3D225502587F411007DBCDF /* StatusViewTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StatusViewTests.swift; sourceTree = "<group>"; };		B430D2F925534FDC0088CC23 /* UserHaloCourseView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UserHaloCourseView.swift; sourceTree = "<group>"; };


Was this file manually changed to resolve conflicts? Seems that it now has custom format, to fix it C3D225502587F411007DBCDF /* StatusViewTests.swift */ and B430D2F925534FDC0088CC23 /* UserHaloCourseView.swift */ should be in different lines.

MaximAlien · 2021-01-07T02:36:42Z

    /**
     Shows the status view with an optional spinner.
     */
-    public func show(_ title: String, showSpinner: Bool, interactive: Bool = false) {


It'd be better to extend existing APIs instead of replacing existing ones. If you feel that newly added API is more concise previous one can be deprecated/or left without any changes.

MaximAlien · 2021-01-07T02:37:20Z

+        var priority: Priority
+    }
+
+    struct Priority: RawRepresentable {


It'd be better to use enum for status priority. If Status meant to be used publicly, Priority should be made public as well.

I think the idea was to allow for arbitrary priorities, similar to constraint priorities. But making the type public would make sense.

MaximAlien · 2021-01-07T02:42:54Z

 public protocol NavigationStatusPresenter: class {
    /**
-     Shows the status view for a specified amount of time.
+     Shows a Status for a specified amount of time.


I think such comment would be accurate for func addNewStatus(status: StatusView.Status). I think it'd better to comment with - parameter statements in this case. Please also add docs for addNewStatus(status:) and hideStatus(status:).

MaximAlien · 2021-01-07T02:44:41Z

     */
    func showStatus(title: String, spinner: Bool, duration: TimeInterval, animated: Bool, interactive: Bool)
+
+    func addNewStatus(status: StatusView.Status)


I'd also rename addNewStatus(status:) to func show(_ status: StatusView.Status) and func hideStatus(using: StatusView.Status) to hide(_ status: StatusView.Status).

MaximAlien · 2021-01-07T02:49:26Z

    public func navigationService(_ service: NavigationService, willEndSimulating progress: RouteProgress, becauseOf reason: SimulationIntent) {
        guard reason == .manual else { return }
        statusView.hide(delay: 0, animated: true)
+        // statusView.hide("simulating")


Is commented part needed? If not, please remove it.

1ec5 · 2021-03-02T03:40:35Z

Looks like the PR branch got munged up somehow, as if it got merged then rebased. If git reflog says merging was the last thing you did, you could try hard-resetting to before that merge then performing just a rebase and finally force-pushing.

jill-cardamon · 2021-03-04T14:05:35Z

The last thing there is a rebase; I didn't use a merge at all.

1ec5 · 2021-03-05T18:22:36Z

I think this branch was rebased onto an old copy of the main branch.

…arted new show/hide implementation and subsequent status management

…ing and hiding banners

…) without dispatch

…t one assertion fails

…o it needs to be fixed

…false to navigationServiceDidChangeAuthorization so that halo view disappears when precise location is enabled so halo disappears

Upgraded to MapboxNavigationNative v31.0.1. Removed explicit routing tile version discovery now that the navigator can automatically choose the most recent version.

…false to navigationServiceDidChangeAuthorization so that halo view disappears when precise location is enabled so halo disappears

1ec5

This is tantalizingly close – I think the remaining tasks will be pretty small in nature.

1ec5 · 2021-03-11T16:31:38Z


 * MapboxCoreNavigation can now be installed using Swift Package Manager. ([#2771](https://github.com/mapbox/mapbox-navigation-ios/pull/2771))
-* The CarPlay guidance panel now shows lane guidance. ([#2798](https://github.com/mapbox/mapbox-navigation-ios/pull/2798))
+* The CarPlay guidance panel now shows lane guidance. ([#1885](https://github.com/mapbox/mapbox-navigation-ios/pull/1885))


This change looks like a bad merge; can you revert it? Thanks!

1ec5 · 2021-03-11T16:41:00Z

+    public struct Priority: RawRepresentable {
+        public typealias RawValue = Int


Since no specific values are defined, it would be simpler to define a type alias than a raw representable type alias inside a struct:

public typealias Priority = Int

1ec5

This is good to go once the struct goes away per #2747 (comment) and the changelog is fixed per #2747 (comment).

1ec5 · 2021-03-11T20:08:43Z

     */
    public struct Status {
+        /**
+        `identifier` is a unique string identifier for a `Status`


FYI, it isn’t necessary to explicitly name the property in the property’s own documentation comment. For example, this could say “A string that uniquely identifies this status.”

1ec5 · 2021-03-11T20:09:45Z

+    public struct Priority {
+        public typealias Priority = Int


Replace the struct with the typealias. The value property will go away: you’ll be able to use an Int as if it were a Priority.

The previous version with the raw-representable Priority would’ve been appropriate if we wanted to define some preset values like high and low that were tied to specific values. But only a bare typealias is necessary if we’re treating it as just an ordinary number with special semantics.

jill-cardamon added op-ex Refactoring, Tech Debt or any other operational excellence work. UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. iOS labels Nov 24, 2020

jill-cardamon self-assigned this Nov 24, 2020

Rmkasun-8 approved these changes Nov 25, 2020

View reviewed changes

1ec5 modified the milestones: f (android-v1.6.0 / ios-v1.4.0), duck (android-v1.4.0 / ios-v1.2.0) Dec 17, 2020

jill-cardamon marked this pull request as ready for review January 6, 2021 15:55

MaximAlien requested a review from a team January 6, 2021 17:52

ShanMa1991 reviewed Jan 6, 2021

View reviewed changes

MaximAlien reviewed Jan 7, 2021

View reviewed changes

1ec5 modified the milestones: duck (android-v1.4.0 / ios-v1.2.0), e (android-v1.5.0 / ios-v1.3.0) Jan 23, 2021

jill-cardamon force-pushed the jill-enable-precise-location-tailwork-2723 branch from 7460c54 to e187179 Compare February 15, 2021 13:55

1ec5 reviewed Feb 26, 2021

View reviewed changes

Jill Cardamon added 3 commits March 5, 2021 15:02

created struct to represent showStatus, refactored relevant files, st…

008c70e

…arted new show/hide implementation and subsequent status management

statuses append correctly and don't duplicate, continued work on show…

2c91aa9

…ing and hiding banners

added show and hide to manageStatuses() method

ff37fac

Jill Cardamon and others added 19 commits March 5, 2021 15:02

made test cases more resiliant; need to investigate using hideStatus(…

c77e74a

…) without dispatch

trying to use dispatch queue in hide

c77e3f6

contained all delays within hide(), doesn't work

eee4c9f

works with only one dispatch queue, need to rerun tests

d989ead

deprecated old showStatus method, tests are better written and all bu…

6cad500

…t one assertion fails

updated test cases, wait for expectation fails but it's supposed to s…

81f30c2

…o it needs to be fixed

fixed failing test, removed debugging statements, clean up code

1b73921

incorporating feedback

1b0d4e0

made priority public, added inline doc for addNewStatus()

069922c

update changelog, clean up code, add reducedAccuracyActivatedMode == …

53cd613

…false to navigationServiceDidChangeAuthorization so that halo view disappears when precise location is enabled so halo disappears

MapboxNavigationNative v31.0.1

d6c4bcf

Upgraded to MapboxNavigationNative v31.0.1. Removed explicit routing tile version discovery now that the navigator can automatically choose the most recent version.

Add Changelog entry.

8790a4c

Fix CHANGELOG entry to mention CarPlay

a673fe2

Typo

5e41168

update changelog, clean up code, add reducedAccuracyActivatedMode == …

b519916

…false to navigationServiceDidChangeAuthorization so that halo view disappears when precise location is enabled so halo disappears

incorporated feedback, update changelog

057974e

remove unnecessary doc comments from previous commit

d948241

fix changelog issue

7847b34

fix status initialization in StatusViewTests

515dab5

jill-cardamon force-pushed the jill-enable-precise-location-tailwork-2723 branch from bbac3ca to 515dab5 Compare March 5, 2021 20:48

1ec5 reviewed Mar 11, 2021

View reviewed changes

Jill Cardamon added 2 commits March 11, 2021 13:39

switch to typealias being priority instead of rawrepresentable

8c5719d

add doc comments, fix naming for priority value

572b1b1

1ec5 approved these changes Mar 11, 2021

View reviewed changes

clean up doc comments, remove Priority struct entirely

fdd9ca6

jill-cardamon merged commit fd1daf4 into main Mar 11, 2021

jill-cardamon deleted the jill-enable-precise-location-tailwork-2723 branch March 11, 2021 21:03

1ec5 mentioned this pull request Mar 19, 2021

Automatically hide "enable precise location" banner #2723

Closed

		return navigationComponents.allSatisfy { $0.navigationServiceShouldDisableBatteryMonitoring(service) }
		}

		public struct Priority: RawRepresentable {
		public typealias RawValue = Int

Conversation

jill-cardamon commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Implementation

Screenshots or Gifs

Uh oh!

1ec5 commented Nov 25, 2020

Uh oh!

MaximAlien commented Jan 6, 2021

Uh oh!

jill-cardamon commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximAlien Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximAlien Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

1ec5 commented Mar 2, 2021

Uh oh!

jill-cardamon commented Mar 4, 2021

Uh oh!

1ec5 commented Mar 5, 2021

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

1ec5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jill-cardamon commented Nov 24, 2020 •

edited

Loading

jill-cardamon commented Jan 6, 2021 •

edited

Loading

MaximAlien Jan 6, 2021 •

edited

Loading

MaximAlien Jan 7, 2021 •

edited

Loading

1ec5 left a comment •

edited

Loading