Ensure crop points are current #50
Conversation
Previously setting the crop points via the text entry dialog would update the sliders, but moving the sliders would not update the pre-populated values in the text entry dialog. This was due to the text field being populated with the `Trial` values, which aren't updated until the crop changes are saved. This change uses the crop values from the current playback view controller, which is the same value that the sliders present and modify. Fixes googlearchive#47
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| /// - markerType: A crop marker type. | ||
| func showTimestampEditAlert(forTrialCropRange trialCropRange: ChartAxis<Int64>, | ||
| andCropMarkerType markerType: CropOverlayView.MarkerType) { | ||
| self.trialCropRange = trialCropRange |
There was a problem hiding this comment.
Setting this here feels a little weird to me, but I can't think of a better way to do it.
There was a problem hiding this comment.
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.
| }) | ||
|
|
||
| return actions | ||
| } | ||
|
|
||
| private func showTimestampEditAlert(for cropMarkerType: CropOverlayView.MarkerType) { | ||
| guard let trialCropRange = self.currentPlaybackViewController?.1.cropRange else { | ||
| return |
There was a problem hiding this comment.
I don't think this should ever be possible. I considered doing a fatalError here, but this seemed safer, given the uncertainty.
| /// - trialCropRange: The trial's crop range. | ||
| /// - markerType: A crop marker type. | ||
| func showTimestampEditAlert(forTrialCropRange trialCropRange: ChartAxis<Int64>, | ||
| andCropMarkerType markerType: CropOverlayView.MarkerType) { |
There was a problem hiding this comment.
We could possibly improve this method per our conversation and align it better with the other change you made:
showTimestampEditAlert(for: .start, trialCropRange: trialCropRange)
| /// - markerType: A crop marker type. | ||
| func showTimestampEditAlert(forTrialCropRange trialCropRange: ChartAxis<Int64>, | ||
| andCropMarkerType markerType: CropOverlayView.MarkerType) { | ||
| self.trialCropRange = trialCropRange |
There was a problem hiding this comment.
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.
Checklist
Motivation and Context
Previously setting the crop points via the text entry dialog would update the sliders, but moving the sliders would not update the pre-populated values in the text entry dialog. This was due to the text
field being populated with the
Trialvalues, which aren't updated until the crop changes are saved.Fixes #47
Description
This change uses the crop values from the current playback view controller, which is the same value that the sliders present and modify.
There do not appear to be tests for this part of the app, so I did not add or update any. I tested the change manually by modifying the crop points through both the sliders and the text input dialog and ensuring that the changes were reflected in the corresponding UI.