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

[estuary] - show buffering progress in video full screen #10325

Merged
merged 1 commit into from Aug 28, 2016

Conversation

@stealthflyer
Copy link
Contributor

commented Aug 22, 2016

The current estuary skin just has a rotating icon, with no indication how bad the streaming source is when it comes to filling the buffer. Suggesting to restore the progress indicator (using the volume progress images for now) and add a label so it is clear that the source is either buffering slow or fast. Grey font color may need tweaking, it could blend with the video but at least the progress bar will be enough.

This is a replacement to PR #10322 incorporating the changes recommended by @BigNoid using estuary branch instead of my master.

Screenshot of worst case:
image

@BigNoid

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Thanks. I think it will be better readable if both text and progress circle have the theme color. Thats the button_focus color. Could you try that?

@stealthflyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

Attached is a screenshot with the change recommended (also switched to lato-bold)

image

@BigNoid

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Yeah much better, thx. One final comment, if you look at the screenshot and don't know the percentage, it looks like its 25% caching to me. If you switch the colors on the progress circle it might be clearer.

stealthflyer added a commit to stealthflyer/xbmc that referenced this pull request Aug 23, 2016
Per suggestion in xbmc#10325 (comment) trying a new way to show the progress bar
@stealthflyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

It uses the volume bar progress png's and rotates clockwise (so the behavior is the same as volume).

I experimented with the alternative and it looked worse. I found an easy solution that looks good and doesnt require making a new set of images and running it through the texturepacker. The first image below is what I just delivered, the second is your recommendation. Let me know if you prefer the second, I can deliver that instead.

image

image

@BigNoid

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

if you remove the fade animation you'd have what I meant. The blue progress from the first screenshot with a white circle underneath like in the second screenshot.

@stealthflyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

I get it, I thought you wanted to swap colors. Removing the fade effect is easy, I'll back out my last change and remove the fade.

Result will look like below:
image

stealthflyer added a commit to stealthflyer/xbmc that referenced this pull request Aug 23, 2016
Based on suggestion from xbmc#10325 (comment) revert the change and just remove the fade effect. Makes the white progress 100% opaque.
@BigNoid

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

I meant also to swap colors, so just the other way around and its good to go.

stealthflyer added a commit to stealthflyer/xbmc that referenced this pull request Aug 23, 2016
Per suggestion xbmc#10325 (comment) swapping the colors and bringing back the fade effect (otherwise too bright)
@stealthflyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

Change made. Added the fade back (white was too bright).

Here's a preview:
image

@stealthflyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

When I tried it on my TV instead of the monitor, using real sources, the TV contrast ratio made it more apparent that the fade effect was interfering with certain video stills. I tried it removed (except for the initial white circle that appears at buffer=0) and the result seemed more in line with the estuary theme.

@BigNoid

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

Looks good to me

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

@stealthflyer could you please squash all the commits into one and force push the changes to your branch?

@stealthflyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2016

Could you grant me write access to the repository? I could leverage github to do the squash and merge using the tutorial here: https://github.com/blog/2141-squash-your-commits

This would keep the history on my branch while maintaining the cleanliness on the main branch. Otherwise I presume you are suggesting a rebase and a force commit.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

@stealthflyer sorry we don't give access. You need squash the commits in this current branch and force push to the same branch in your repo. We don't need history as all this is a single change.

Edit:
You'll need to do some git commandline for this

@fritsch

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

git branch my_old_history
git rebase -i HEAD~5
squash all of them
git push -f yourorigin your branch

After that your whole history will still be in my_old_history branch

2016-08-27 15:58 GMT+02:00 Martijn Kaijser notifications@github.com:

@stealthflyer https://github.com/stealthflyer sorry we don't give
access. You need squash the commits in this current branch and force push
to the same branch in your repo. We don't need history as all this is a
single change.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#10325 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABCfHcnPZy6FDHDZleq9fT3FChtISo0Fks5qkEKGgaJpZM4JqSbg
.

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

Bring back the cache level but also add a label to help identify how long some streaming sources may take to reach full cache capacity. Using estuary theme coloring with a percent indicator in the center.
@stealthflyer stealthflyer force-pushed the stealthflyer:stealthflyer-estuary branch from 52260ec to 859983d Aug 27, 2016
@stealthflyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2016

Squashed.

@fritsch thanks for the suggestion, I didn't branch since the history is logged in this PR and its a very simple change anyways.

@fritsch

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

Now only the force push is missing :-)

@fritsch

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

Err looks okay - @martijn: final review? All fine in his squash or
something missing?

2016-08-27 16:15 GMT+02:00 Aleks Rozman notifications@github.com:

Squashed.

@fritsch https://github.com/fritsch thanks for the suggestion, I didn't
branch since the history is logged in this PR and its a very simple change
anyways.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10325 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABCfHfLYcm9c8V1R7T3_PJaARb1auzWcks5qkEZwgaJpZM4JqSbg
.

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@BigNoid

This comment has been minimized.

Copy link
Member

commented Aug 28, 2016

Thx!

@BigNoid BigNoid merged commit 7da0099 into xbmc:master Aug 28, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta2 milestone Aug 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.