-
Notifications
You must be signed in to change notification settings - Fork 104
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
Improvement: Update progress and recording indicators for Live TV and Radio #1220
base: master
Are you sure you want to change the base?
Conversation
fa7b7a1
to
4481374
Compare
d4c8c4f
to
194f497
Compare
Renamed new class to |
Refactored this by introducing new class |
sounds good |
It really paid off to not just do a minimal intrusive change as I started from, but re-implement some parts from scratch. |
Let me know once you're good with the implementation. I would then proceed with squashing and updating the PR description. |
XBMC Remote/ProgressBarView.m
Outdated
CGRect frame = progressBar.frame; | ||
frame.size.width = progress * CGRectGetWidth(self.frame); | ||
progressBar.frame = frame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing it manually, you could simply call [self setNeedsLayout]
to let the system know that you want to re-lay out your view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. But to my understanding it would conflict with the way I plan to use it for the playlist progress bar:
- (void)setProgress:(CGFloat)progress {
if (progressBarView.progress == 0 && progress > 0) {
[UIView animateWithDuration:0.5
animations:^{
progressBarView.progress = progress;
}];
}
else {
progressBarView.progress = progress;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how it conflicts. The code you posted would go into BroadcastProgressView, and the comment is about ProgressBarView.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this code goes into the caller's setProgress
method. But if progressBarView.progress
just triggers setNeedsLayout
it does not animate the progress bar anymore. Also confirmed by testing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes, you're absolutely right. You could try inserting layoutIfNeeded
call inside the animation block.
otherwise, just extract obtaining new bar width according to the percentage param into a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a more proper way to handle custom view updates. View has some state (which the view depends on of course) and you have a way to describe how view looks in any state, the rest is done by the system.
BTW that's basically how SwiftUI (or any other declarative ui framework) works. Although in UIKit we have to give hints that we want a re-render ASAP.
Not enforcing such solution, but I suggest that you try it at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes, you're absolutely right. You could try inserting
layoutIfNeeded
call inside the animation block.
This works (layoutIfNeeded
in animation block and at the same time setNeedsLayout
in setProgress
). Will add another fixup commit to use setNeedsLayout
. layoutIfNeeded
will be added in #1226.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw an improvement over your setProgress:
API of progressBarView
would be adding animated
boolean parameter (similar to the system APIs) - this way you can hide these details including calling layoutIfNeeded / setNeedsLayout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I did something similar, but I dropped it as only used for the playlist progress bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and squashed all the fixup commits and updated the commit messages.
Update the indicators for progress and recording to a progress bar. The progress bar is located on the left side, uses dark gray background and Kodi blue for the progress. Total runtime is located right aligned and under the bar. The recording indicator is located in a reserved area left of the runtime. In the EPG the highlighting is done by changing the cell background and letting the start time label use a bigger font size and label color. Introduces the class ProgressBarView for future use by other views as well. This is used as subview by the new class BroadcastProgressView which implements the layout described above and replaces the xib-defined progress view.
Consistently use timerView as variable name. Only get progressView and timerView once in cellForRowAtIndexPath.
Avoids potential mismatch.
d97aa04
to
1d8a308
Compare
Description
This PR modernizes the way of visualizing a broadcast's progress and recording state. The current pie chart and its runtime label are hard to read. The recording icon shown in the middle of the pie chart is not recognizable well in channel list view. To improve this, the new classes
ProgressBarView
andBroadcastProgressView
are introduced, the formerly usedProgressPieView
is removed.New classes
ProgressBarView
ProgressBarView
can use user-defined width and height. Internally, it is built from two subviews, first is representing the track and masking the second one, which shows the progress. Track color can be set viasetTrackColor
and progress can be set via the propertyprogress
. The layout updates are only done inlayoutSubviews
which is also triggered on updates ofprogress
.This is a generic class for progress bars which can be used by other views as well. Compared to iOS'
UIProgressView
, it gives more freedom for the layout (especially working withheight
andcornerRadius
).BroadcastProgressView
BroadcastProgressView
is built fromProgressBarView
and the labelbarLabel
, which is placed right-aligned under the progress bar. Progress updates can be set bysetProgress
, label can be updated via the propertybarLabel
. The layout updates are only done inlayoutSubviews
.The recoding/timer icon must be placed in
BroadcastProgressView
's parent to set its visibility independent of the progress view (use case: timer was set for a future broadcast). The methodgetReservedCenter
provides the center of the reserved left of the label to support correct placement.Layout
Horizontal Progress Bar
The new visualization uses a horizontal progress bar, which is placed on the left side and uses the same width as the thumbnail or label under which it is placed. The progress bar has a dark gray background and uses Kodi blue to show the progress.
Runtime and Recording State
Total runtime is located right aligned under the progress bar. The recording indicator is located in a reserved area left of the runtime.
EPG
In the EPG the highlighting of the current active broadcast is done by changing the cell background and letting the start time label use a bigger font size and label color.
Impression
Screenshots (left pair = new, right pair = old):
https://ibb.co/zmXZ1FY
Summary for release notes
Improvement: Update progress and recording indicators for Live TV and Radio