Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Ensure crop points are current #50

Closed
wants to merge 2 commits into from
Closed

Ensure crop points are current #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13eeae1
Ensure crop points are current
marcisme May 3, 2019
3b591c5
Rename method params
marcisme May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions ScienceJournal/UI/CropRangeViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class CropRangeViewController: UIViewController, EditTimestampViewControllerDele
weak var delegate: CropRangeViewControllerDelegate?

/// The crop range of the trial.
var trialCropRange: ChartAxis<Int64>?
private var trialCropRange: ChartAxis<Int64>?

private var alertPresentation: TimestampAlertPresentation?
private let trialRecordingRange: ChartAxis<Int64>
Expand All @@ -47,10 +47,8 @@ class CropRangeViewController: UIViewController, EditTimestampViewControllerDele
/// Designated initializer
///
/// - Parameters:
/// - trialCropRange: The trial's crop range.
/// - trialRecordingRange: The trial's recording range.
init(trialCropRange: ChartAxis<Int64>?, trialRecordingRange: ChartAxis<Int64>) {
self.trialCropRange = trialCropRange
init(trialRecordingRange: ChartAxis<Int64>) {
self.trialRecordingRange = trialRecordingRange
cropValidator = CropValidator(trialRecordingRange: trialRecordingRange)
super.init(nibName: nil, bundle: nil)
Expand All @@ -62,11 +60,12 @@ class CropRangeViewController: UIViewController, EditTimestampViewControllerDele

/// Shows an alert that allows the user to manually input a timestamp for the given crop marker.
///
/// - Parameter marker: A crop marker type.
func showTimestampEditAlert(forCropMarkerType markerType: CropOverlayView.MarkerType) {
guard let trialCropRange = trialCropRange else {
return
}
/// - Parameters
/// - markerType: A crop marker type.
/// - trialCropRange: The trial's crop range.
func showTimestampEditAlert(for markerType: CropOverlayView.MarkerType,
trialCropRange: ChartAxis<Int64>) {
self.trialCropRange = trialCropRange
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting this here feels a little weird to me, but I can't think of a better way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this CL but: CropRangeViewController is very odd. Looking at this again it, it should ideally have one instance per presentation of the crop so you could just initialize and present at once, rather than initializing and calling the show methods repeatedly. It doesn't even have it's own view. Makes me think it should just be a coordinator object or combine with the EditTimestampViewController class which already fits the "one instance per action" pattern.


let editTimestampVC = EditTimestampViewController()
editTimestampVC.delegate = self
Expand Down
17 changes: 11 additions & 6 deletions ScienceJournal/UI/TrialDetailViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,7 @@ class TrialDetailViewController: MaterialHeaderViewController,
self.preferenceManager = preferenceManager
self.sensorDataManager = sensorDataManager
cropValidator = CropValidator(trialRecordingRange: trial.recordingRange)
cropRangeController = CropRangeViewController(trialCropRange: trial.cropRange,
trialRecordingRange: trial.recordingRange)
cropRangeController = CropRangeViewController(trialRecordingRange: trial.recordingRange)

// MDCCollectionViewFlowLayout currently has a bug that breaks sectionHeadersPinToVisibleBounds
// so we need to use a UICollectionViewFlowLayout instead until it's fixed. See issue:
Expand Down Expand Up @@ -1128,8 +1127,6 @@ class TrialDetailViewController: MaterialHeaderViewController,
recordingRange: trial.recordingRange)
}
}

cropRangeController.trialCropRange = editingCropRange
}

private func endCropping() {
Expand Down Expand Up @@ -1296,17 +1293,25 @@ class TrialDetailViewController: MaterialHeaderViewController,

// Edit start time.
actions.append(PopUpMenuAction(title: String.editCropStartTime, isEnabled: true) { _ in
self.cropRangeController.showTimestampEditAlert(forCropMarkerType: .start)
self.showTimestampEditAlert(for: .start)
})

// Edit end time.
actions.append(PopUpMenuAction(title: String.editCropEndTime, isEnabled: true) { _ in
self.cropRangeController.showTimestampEditAlert(forCropMarkerType: .end)
self.showTimestampEditAlert(for: .end)
})

return actions
}

private func showTimestampEditAlert(for cropMarkerType: CropOverlayView.MarkerType) {
guard let trialCropRange = self.currentPlaybackViewController?.1.cropRange else {
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this should ever be possible. I considered doing a fatalError here, but this seemed safer, given the uncertainty.

}
self.cropRangeController.showTimestampEditAlert(for: cropMarkerType,
trialCropRange: trialCropRange)
}

/// Creates trial data for export.
///
/// - Parameter completion: Called when complete with a Bool indicating success, and the trial
Expand Down