[ios] Review fixes

Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

iphone/Chart/Chart/Views/ChartLineView.swift

@@ -67,7 +67,7 @@ class ChartLineView: UIView {
   }
 
   func setY(min: CGFloat, max: CGFloat, animationStyle: ChartAnimation = .none) {
-    assert(min < max)
+    assert(min <= max)
     minY = min
     maxY = max
     updateGraph(animationStyle: animationStyle)

iphone/CoreApi/CoreApi/PlacePageData/Common/PlacePagePreviewData.mm

@@ -53,7 +53,6 @@ static PlacePageDataSchedule convertOpeningHours(std::string_view rawOH)
 - (instancetype)initWithTrackInfo:(TrackInfo * _Nonnull)trackInfo {
   self = [super init];
   if (self) {
-    // TODO: (KK) Replace separator with a shared static constant.
     NSString * kSeparator = @" • ";
     _title = [@[trackInfo.duration, trackInfo.distance] componentsJoinedByString:kSeparator];
   }

iphone/CoreApi/CoreApi/PlacePageData/ElevationProfile/ElevationProfileData.mm

@@ -59,7 +59,7 @@ static ElevationDifficulty convertDifficulty(uint8_t difficulty) {
                                                                                   andAltitude:point.m_point.GetAltitude()];
     [pointsArray addObject:elevationPoint];
   }
-  return [pointsArray copy];
+  return pointsArray;
 }
 
 @end

iphone/Maps/Classes/CustomViews/MapViewControls/TrackRecordingButtonViewController.swift

@@ -41,10 +41,7 @@ final class TrackRecordingButtonViewController: MWMViewController {
 
   override func viewDidLoad() {
     super.viewDidLoad()
-    // async is for smoother appearance
-    DispatchQueue.main.asyncAfter(deadline: .now() + kDefaultAnimationDuration) {
-      self.setState(self.state, completion: nil)
-    }
+    setState(self.state, completion: nil)
   }
 
   // MARK: - Public methods

iphone/Maps/Classes/MapViewController.mm

@@ -892,7 +892,7 @@ NSString *const kSettingsSegue = @"Map2Settings";
                                             TrackInfo * _Nonnull trackInfo,
                                             ElevationProfileData * _Nonnull (^ _Nullable elevationData) ()) {
     __strong __typeof(weakSelf) self = weakSelf;
-
+    if (!self) return;
     switch (state) {
       case TrackRecordingStateInactive:
         [self stopObservingTrackRecordingUpdates];

iphone/Maps/Core/TrackRecorder/TrackRecordingManager.swift

@@ -9,15 +9,18 @@ enum TrackRecordingAction {
   case stopAndSave(name: String)
 }
 
-enum TrackRecordingError: Error {
+enum LocationError: Error {
   case locationIsProhibited
-  case trackIsEmpty
-  case systemError(Error)
 }
 
-enum TrackRecordingActionResult {
+enum StartTrackRecordingResult {
   case success
-  case error(TrackRecordingError)
+  case failure(Error)
+}
+
+enum StopTrackRecordingResult {
+  case success
+  case trackIsEmpty
 }
 
 @objc
@@ -44,8 +47,6 @@ typealias TrackRecordingStateHandler = (TrackRecordingState, TrackInfo, Elevatio
 @objcMembers
 final class TrackRecordingManager: NSObject {
 
-  typealias CompletionHandler = (TrackRecordingActionResult) -> Void
-
   fileprivate struct Observation {
     weak var observer: AnyObject?
     var recordingStateDidChangeHandler: TrackRecordingStateHandler?
@@ -111,22 +112,47 @@ final class TrackRecordingManager: NSObject {
     recordingState == .active
   }
 
-  func processAction(_ action: TrackRecordingAction, completion: (CompletionHandler)? = nil) {
+  func start(completion: ((StartTrackRecordingResult) -> Void)? = nil) {
     do {
-      switch action {
-      case .start:
-        try startRecording()
-      case .stopAndSave(let name):
-        stopRecording()
-        try checkIsTrackNotEmpty()
-        saveTrackRecording(name: name)
+      switch recordingState {
+      case .inactive:
+        try checkIsLocationEnabled()
+        subscribeOnTrackRecordingProgressUpdates()
+        trackRecorder.startTrackRecording()
+        notifyObservers()
+        do {
+          try activityManager?.start(with: trackRecordingInfo)
+        } catch {
+          LOG(.warning, "Failed to start activity manager")
+          handleError(error)
+        }
+      case .active:
+        break
       }
       completion?(.success)
     } catch {
-      handleError(error, completion: completion)
+      handleError(error)
+      completion?(.failure(error))
     }
   }
 
+  func stopAndSave(withName name: String = "", completion: ((StopTrackRecordingResult) -> Void)? = nil) {
+    unsubscribeFromTrackRecordingProgressUpdates()
+    trackRecorder.stopTrackRecording()
+    trackRecordingInfo = .empty()
+    activityManager?.stop()
+    notifyObservers()
+
+    guard !trackRecorder.isTrackRecordingEmpty() else {
+      Toast.show(withText: L("track_recording_toast_nothing_to_save"))
+      completion?(.trackIsEmpty)
+      return
+    }
+
+    trackRecorder.saveTrackRecording(withName: name)
+    completion?(.success)
+  }
+
   // MARK: - Private methods
 
   private func subscribeOnTheAppLifecycleEvents() {
@@ -136,20 +162,12 @@ final class TrackRecordingManager: NSObject {
                                            object: nil)
   }
 
-  private func checkIsLocationEnabled() throws(TrackRecordingError) {
+  private func checkIsLocationEnabled() throws {
     if locationService.isLocationProhibited() {
-      throw TrackRecordingError.locationIsProhibited
+      throw LocationError.locationIsProhibited
     }
   }
 
-  private func checkIsTrackNotEmpty() throws(TrackRecordingError) {
-    if trackRecorder.isTrackRecordingEmpty() {
-      throw TrackRecordingError.trackIsEmpty
-    }
-  }
-
-  // MARK: - Handle track recording process
-
   private func subscribeOnTrackRecordingProgressUpdates() {
     trackRecorder.setTrackRecordingUpdateHandler { [weak self] info in
       guard let self else { return }
@@ -163,52 +181,15 @@ final class TrackRecordingManager: NSObject {
     trackRecorder.setTrackRecordingUpdateHandler(nil)
   }
 
-  // MARK: - Handle Start/Stop event and Errors
-
-  private func startRecording() throws(TrackRecordingError) {
-    switch recordingState {
-    case .inactive:
-      try checkIsLocationEnabled()
-      subscribeOnTrackRecordingProgressUpdates()
-      trackRecorder.startTrackRecording()
-      notifyObservers()
-      do {
-        try activityManager?.start(with: trackRecordingInfo)
-      } catch {
-        LOG(.warning, "Failed to start activity manager")
-        handleError(.systemError(error))
-      }
-    case .active:
-      break
-    }
-  }
-
-  private func stopRecording() {
-    unsubscribeFromTrackRecordingProgressUpdates()
-    trackRecorder.stopTrackRecording()
-    trackRecordingInfo = .empty()
-    activityManager?.stop()
-    notifyObservers()
-  }
-
-  private func saveTrackRecording(name: String) {
-    trackRecorder.saveTrackRecording(withName: name)
-  }
-
-  private func handleError(_ error: TrackRecordingError, completion: (CompletionHandler)? = nil) {
+  private func handleError(_ error: Error) {
     switch error {
-    case TrackRecordingError.locationIsProhibited:
+    case LocationError.locationIsProhibited:
       // Show alert to enable location
       locationService.checkLocationStatus()
-    case TrackRecordingError.trackIsEmpty:
-      Toast.show(withText: L("track_recording_toast_nothing_to_save"))
-    case TrackRecordingError.systemError(let error):
+    default:
       LOG(.error, error.localizedDescription)
       break
     }
-    DispatchQueue.main.async {
-      completion?(.error(error))
-    }
   }
 }
 

iphone/Maps/Tests/Core/TrackRecorder/TrackRecordingManagerTests.swift

@@ -51,7 +51,7 @@ final class TrackRecordingManagerTests: XCTestCase {
     mockLocationService.locationIsProhibited = false
     mockTrackRecorder.trackRecordingIsEnabled = false
 
-    trackRecordingManager.processAction(.start)
+    trackRecordingManager.start()
 
     XCTAssertTrue(mockTrackRecorder.startTrackRecordingCalled)
     XCTAssertTrue(mockActivityManager.startCalled)
@@ -60,20 +60,21 @@ final class TrackRecordingManagerTests: XCTestCase {
 
   func test_GivenStartRecording_WhenLocationDisabled_ThenShouldFail() {
     mockLocationService.locationIsProhibited = true
-
-    trackRecordingManager.processAction(.start) { result in
+    let expectation = expectation(description: "Location is prohibited")
+    trackRecordingManager.start() { result in
       switch result {
       case .success:
         XCTFail("Should not succeed")
-      case .error(let error):
+      case .failure(let error):
         switch error {
-        case .locationIsProhibited:
-          XCTAssertTrue(true)
+        case LocationError.locationIsProhibited:
+          expectation.fulfill()
         default:
           XCTFail("Unexpected error: \(error)")
         }
       }
     }
+    wait(for: [expectation], timeout: 1.0)
     XCTAssertFalse(self.mockTrackRecorder.startTrackRecordingCalled)
     XCTAssertTrue(trackRecordingManager.recordingState == .inactive)
   }
@@ -82,12 +83,12 @@ final class TrackRecordingManagerTests: XCTestCase {
     mockTrackRecorder.trackRecordingIsEnabled = true
     mockTrackRecorder.trackRecordingIsEmpty = false
 
-    trackRecordingManager.processAction(.stopAndSave(name: "Test Track")) { result in
+    trackRecordingManager.stopAndSave(withName: "Test Track") { result in
       switch result {
       case .success:
         XCTAssertTrue(true)
-      case .error(let error):
-        XCTFail("Unexpected error: \(error)")
+      case .trackIsEmpty:
+        XCTFail("Track should not be empty")
       }
     }
     XCTAssertTrue(mockTrackRecorder.stopTrackRecordingCalled)
@@ -99,20 +100,16 @@ final class TrackRecordingManagerTests: XCTestCase {
   func test_GivenStopRecording_WhenTrackIsEmpty_ThenShouldFail() {
     mockTrackRecorder.trackRecordingIsEnabled = true
     mockTrackRecorder.trackRecordingIsEmpty = true
-
-    trackRecordingManager.processAction(.stopAndSave(name: "Test Track")) { result in
+    let expectation = expectation(description: "Track recording should be empty")
+    trackRecordingManager.stopAndSave(withName: "Test Track") { result in
       switch result {
       case .success:
         XCTFail("Should not succeed")
-      case .error(let error):
-        switch error {
-        case .trackIsEmpty:
-          XCTAssertTrue(true)
-        default:
-          XCTFail("Unexpected error: \(error)")
-        }
+      case .trackIsEmpty:
+        expectation.fulfill()
       }
     }
+    wait(for: [expectation], timeout: 1.0)
     XCTAssertFalse(mockTrackRecorder.saveTrackRecordingCalled)
     XCTAssertTrue(trackRecordingManager.recordingState == .inactive)
   }

iphone/Maps/UI/BottomMenu/Menu/BottomMenuInteractor.swift

@@ -76,13 +76,19 @@ extension BottomMenuInteractor: BottomMenuInteractorProtocol {
   }
 
   func toggleTrackRecording() {
+    close()
     switch trackRecorder.recordingState {
     case .active:
       break
     case .inactive:
-      trackRecorder.processAction(.start)
+      trackRecorder.start { result in
+        switch result {
+        case .success:
+          MapViewController.shared()?.showTrackRecordingPlacePage()
+        case .failure:
+          break
+        }
+      }
     }
-    close()
-    MapViewController.shared()?.showTrackRecordingPlacePage()
   }
 }

iphone/Maps/UI/PlacePage/Components/ElevationProfile/ElevationProfilePresenter.swift

@@ -76,12 +76,12 @@ extension ElevationProfilePresenter: ElevationProfilePresenterProtocol {
   }
 
   func configure() {
+    view?.isChartViewHidden = false
+
     let kMinPointsToDraw = 3
     guard let profileData, let chartData, chartData.points.count >= kMinPointsToDraw else {
-      view?.isChartViewHidden = true
       return
     }
-    view?.isChartViewHidden = false
     view?.setChartData(ChartPresentationData(chartData, formatter: formatter))
     view?.reloadDescription()
 

iphone/Maps/UI/PlacePage/PlacePageInteractor.swift

@@ -251,11 +251,11 @@ extension PlacePageInteractor: ActionBarViewControllerDelegate {
       showTrackDeletionConfirmationDialog()
     case .saveTrackRecording:
       // TODO: (KK) pass name typed by user
-      TrackRecordingManager.shared.processAction(.stopAndSave(name: "")) { [weak self] result in
+      TrackRecordingManager.shared.stopAndSave() { [weak self] result in
         switch result {
         case .success:
           break
-        case .error:
+        case .trackIsEmpty:
           self?.presenter?.closeAnimated()
         }
       }