Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wutschel
Copy link
Collaborator

@wutschel wutschel commented Dec 27, 2024

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 and BroadcastProgressView are introduced, the formerly used ProgressPieView 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 via setTrackColor and progress can be set via the property progress. The layout updates are only done in layoutSubviews which is also triggered on updates of progress.

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 with height and cornerRadius).

BroadcastProgressView

BroadcastProgressView is built from ProgressBarView and the label barLabel, which is placed right-aligned under the progress bar. Progress updates can be set by setProgress, label can be updated via the property barLabel. The layout updates are only done in layoutSubviews.

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 method getReservedCenter 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

XBMC Remote/DetailViewController.m Outdated Show resolved Hide resolved
XBMC Remote/ProgressBarView.h Outdated Show resolved Hide resolved
XBMC Remote/ProgressBarView.m Outdated Show resolved Hide resolved
XBMC Remote/ProgressBarView.m Outdated Show resolved Hide resolved
XBMC Remote/ProgressBarView.m Show resolved Hide resolved
@wutschel
Copy link
Collaborator Author

Renamed new class to BroadcastProgressView to better match its purpose.

@wutschel
Copy link
Collaborator Author

Refactored this by introducing new class ProgressBarView which is then used by BroadcastProgressView. In a second step ProgressBarView can be used when rebasing and updating #1226. Does this look good?

@kambala-decapitator
Copy link
Collaborator

Refactored this by introducing new class ProgressBarView which is then used by BroadcastProgressView. In a second step ProgressBarView can be used when rebasing and updating #1226. Does this look good?

sounds good

@wutschel
Copy link
Collaborator Author

Refactored this by introducing new class ProgressBarView which is then used by BroadcastProgressView. In a second step ProgressBarView can be used when rebasing and updating #1226. Does this look good?

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.

@wutschel
Copy link
Collaborator Author

Let me know once you're good with the implementation. I would then proceed with squashing and updating the PR description.

XBMC Remote/BroadcastProgressView.h Outdated Show resolved Hide resolved
XBMC Remote/ProgressBarView.m Show resolved Hide resolved
Comment on lines 43 to 45
CGRect frame = progressBar.frame;
frame.size.width = progress * CGRectGetWidth(self.frame);
progressBar.frame = frame;
Copy link
Collaborator

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

Copy link
Collaborator Author

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;
    }
}

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@wutschel wutschel Jan 14, 2025

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants