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] hide remaining time when fullscreen-info is displayed #17474

Merged
merged 1 commit into from Mar 12, 2020
Merged

[Estuary] hide remaining time when fullscreen-info is displayed #17474

merged 1 commit into from Mar 12, 2020

Conversation

howie-f
Copy link
Contributor

@howie-f howie-f commented Mar 10, 2020

Description

cosmetic surgery on the estuary-seekbar.
tags are overlapping remaining time.

Screenshots:

remaining

Types of change

  • Cosmetic change (non-breaking change that doesn't touch code)

@ronie please review the only reasonable solution that came to my mind

@ronie
Copy link
Member

ronie commented Mar 10, 2020

thanx, that's indeed an issue.

however, on the left side Estuary optionally displays chapter info, so moving it there doesn't really solve it.

chapter

perhaps move it to the top-right, and place it below the 'Ends at" line?

@howie-f
Copy link
Contributor Author

howie-f commented Mar 10, 2020

hm, ok i'll try that.... not many options but sounds good

screenshot001

so how's that?


EDIT2: but to be honest i would prefer s.th. like this:
Unbenannt

@ronie
Copy link
Member

ronie commented Mar 10, 2020

but to be honest i would prefer s.th. like this

yes, looks good to me as well.
feel free to go with that if you like.

@howie-f
Copy link
Contributor Author

howie-f commented Mar 10, 2020

ok - ready to go. thanks

@ronie ronie self-requested a review March 11, 2020 00:48
@ronie ronie added Component: Skin Type: Fix non-breaking change which fixes an issue v19 Matrix labels Mar 11, 2020
@ronie ronie added this to the Matrix 19.0-alpha 1 milestone Mar 11, 2020
Copy link
Member

@ronie ronie left a comment

Choose a reason for hiding this comment

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

looks good overall. just a bit of fine-tuning required.
thx!

addons/skin.estuary/xml/DialogSeekBar.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/DialogSeekBar.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/DialogSeekBar.xml Outdated Show resolved Hide resolved
@chewitt
Copy link
Member

chewitt commented Mar 11, 2020

I always understood (and appreciate) the media flags in list views as sometimes I have different versions of media (1080p and 4k HDR for testing) and 2.0 and DTS versions of albums. However once I selected the media I don't really need to see the flags again - alongside media controls on the OSD when it's visible. Could we remove the flagging and leave 'remaining' time in the current bottom-right location?

@howie-f howie-f changed the title [estuary] move remaining time away from video-tags (SeekBar) [Estuary] move rem.-time-label away from media flags (SeekBar) Mar 11, 2020
@ksooo
Copy link
Member

ksooo commented Mar 11, 2020

How can I reproduce the original problem? I'm using this a lot and never have seen that.

Is it specific to Matrix?

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

How can I reproduce the original problem? I'm using this a lot and never have seen that.

@ksooo i just play any video and then press "info" on my "official-kodi-remote (ios)", same with live-tv

@DaVukovic
Copy link
Member

DaVukovic commented Mar 11, 2020

Totally agree with @chewitt
I would suggest to remove those flags on that info screen. We are currently trying to move something to a location where it simply doesn't match (sorry if that sounds harsh):

image

The section above that green line is darkened and we already have specific things which are located inside this section. As one can see, the "Restzeit" simply doesn't match inside this section (beside we would make it bigger which would destroy the overall-Estuary-layout).

If I have a voice in that case, I´m against this change and I would suggest to remove those flags from that screen completely. The "time remaining" label matched perfectly before the flags have been added.

Jm2c

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

We are currently trying to move something to a location where it simply doesn't match

@DaVukovic his version is obsolete. the "remaining"-label is back in the seekbar.
generally i'm fine with anything but overlapping labels. so you decide

@howie-f howie-f requested a review from ronie March 11, 2020 07:41
@DaVukovic
Copy link
Member

I'm not deciding anything. I'm saying that we have a change which hasn't been introduced by you (the flags on that info screen). The remaining-label matched perfectly where it has been before those flags have been added. You are trying to fix that somehow and I appreciate that.

From my point of view, every other location of that remaining label looks either "meh" or doesn't fit or isn't the correct location as there are already other labels located. The flags shouldn't have been added in the first place on this screen. So my suggestion is to don't touch the location of the remaining label but to remove those flags from the info screen

I'm also against overlapping labels. Don't get me wrong. I would simply solve it in another way

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

Clarify: you decide = the team decides. don‘t geht me wrong. there‘s no problem at all.

@phunkyfish
Copy link
Contributor

Does some have a screenshot showing the media flags conflict? (Sorry nowhere near a kodi at the moment)

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

Does some have a screenshot showing the media flags conflict? (Sorry nowhere near a kodi at the moment)

@phunkyfish first screenshot in this pr

@phunkyfish
Copy link
Contributor

Does some have a screenshot showing the media flags conflict? (Sorry nowhere near a kodi at the moment)

@phunkyfish first screenshot in this pr

Sorry, too early ;)

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

Is it specific to Matrix?

yes.
Leia hides the "Remaining"-Label as soon as the video-info shows up.
The visible-conditions for both Leia and Matrix are the same:
<visible>![Player.ShowInfo | Window.IsVisible(playerprocessinfo) | VideoPlayer.HasEpg]</visible>
Then i must admit that changing the skin would be the wrong approach.
i'll try to track that down

@jjd-uk
Copy link
Member

jjd-uk commented Mar 11, 2020

Personally I think the media flags need to stay, what I think is out of place is the chapter info down there at the bottom, as we only have room for the Chapter Numbers. If we move chapter info to the top (like it is for Confluence) then we have the space to also display Chapter Name.

So this is what I'd have in mind

image

From a cursory glance moving media flags to the left with chapter moved to the top only produces a conflict for displaying rating on music viz info but that can solved by moving rating to the right.

If this is an acceptable solution then I'll submit to avoid messing howie-f about (I'd noticed the overlap as well so had the code partially done).

For comparison:

Current Estuary with no Chapter Name

image

Confluence

image

@ronie
Copy link
Member

ronie commented Mar 11, 2020

ask 10 people and get 20 different opinions :-)

@jjd-uk
Copy link
Member

jjd-uk commented Mar 11, 2020

Note I'm ok for this to merge as is. I can do my changes as a follow up once I've fully proven them.

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

believe it or not:
turns out that removing (yet nearly empty) DialogFullScreenInfo.xml leads to previous, imo correct, Leia-behavior. see the actual commit history. must be work in progess on that? then this pr can either close or the first solution applies.. i'm fine with both.

@jjd-uk
Copy link
Member

jjd-uk commented Mar 11, 2020

I can confirm.

Overlap starts with Windows build KodiSetup-20191109-8b6bf13d after #16878

Previously Time Remaining was only shown when seeking and full screen info was NOT being shown. That PR somehow introduced Time Remaining also being shown in full screen info thus causing the overlap.

@howie-f howie-f changed the title [Estuary] move rem.-time-label away from media flags (SeekBar) [Estuary] hide remaining time when fullscreen-info is displayed Mar 11, 2020
@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

@ronie finally the correct solution is simple and everyone should be happy now.
to me this is ready to merge. thanks to everyone for input

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

after #16878
that was the crucial hint, thanks @jjd-uk

@jjd-uk
Copy link
Member

jjd-uk commented Mar 11, 2020

Personally I think having the Time Remaining when in full screen info is a good idea even if it was unintentional, but this restores the previous behaviour so I think it should be merged if ronie is ok with it, or he has a better fix.

@howie-f
Copy link
Contributor Author

howie-f commented Mar 11, 2020

Time Remaining when in full screen info is a good idea

sure, i absolutely agree, if the space is there like in your screenshots (which i like)
now we know how to get it back

Copy link
Member

@ronie ronie left a comment

Choose a reason for hiding this comment

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

thanx for the effort to get to the bottom of it!

@howie-f
Copy link
Contributor Author

howie-f commented Mar 12, 2020

all's well that ends well :-)

@fuzzard fuzzard merged commit 40b26aa into xbmc:master Mar 12, 2020
@howie-f howie-f deleted the fix-seekbar-alignment branch March 12, 2020 19:59
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Mar 13, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request May 5, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
[Estuary] hide remaining time when fullscreen-info is displayed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Skin Type: Fix non-breaking change which fixes an issue v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants