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

feat: apply margin to osd-margin-y #499

Merged
merged 5 commits into from
Apr 9, 2023

Conversation

christoph-heinrich
Copy link
Contributor

@christoph-heinrich christoph-heinrich commented Apr 5, 2023

The margins were already set for osc-margins and user-data/osc/margins, but osd messages have their own properties osd-margin-y and osd-margin-x.
Offsetting osd-margin-y by our margin prevents overlap of osd messages with the GUI.

ref. #498

@hooke007
Copy link
Contributor

hooke007 commented Apr 5, 2023

I still think it's user's duty to use a reasonable OSD setting. Not every script would care about their personal settings. But I am not against this PR.

And I think it could be improved a little.

  1. To make it optional. For those hovering messages, it really looks terrible because of 'jumping'.(i.e. toggling stats)
  2. Left or right elements would also cover OSD if someone uses osd-align-y=center

@tomasklaen
Copy link
Owner

I don't actually see this as an issue. It's not like top bar just shows itself unannounced, you have to hover it, so you control when it overlays stuff. And offsetting osd by top bar size is not bullet proof as top bar might have a 2nd and 3rd line that would still overlay osd.

Also I think this would case less space for stats, which I personally don't like, and yeah, the jumping would be annoying :)

Overall I think this brings more issues than it solves. I've personally never had an issue of not being able to read osd messages because of uosc.

Margins make sense for console as you want to make sure it's readable as much as possible even when UI is on, but I don't share the same sentiment with osd messages. If you are using uosc UI, you don't care about them, and if you care about them, you can just hide uosc.

The issue from the discussion that spawned this is that top bar is popping up when it shouldn't, which is a different problem.

@christoph-heinrich
Copy link
Contributor Author

The reason I jumped at this is that I was already bothered by the top bar covering osd messages (to the point of being unreadable) months ago, and I couldn't find a good config that would work well in both windowed and fullscreen, so I ended up with something that was just "good enough". Back then I didn't think of dynamically adjusting it with the margin.

While it's true that the jumping around of the stats isn't great, I don't think it would be much of a problem in practice, after all how often do you switch between showing/hiding the top bar?

  1. To make it optional. For those hovering messages, it really looks terrible because of 'jumping'.(i.e. toggling stats)
  2. Left or right elements would also cover OSD if someone uses osd-align-y=center

I forgot that the volume bar can also be on the left, I'll adjust that later if this doesn't get shut down completely.

@hooke007
Copy link
Contributor

hooke007 commented Apr 6, 2023

how often do you switch between showing/hiding the top bar?

It's up to the usecase. For instance, watching a images dir.

@tomasklaen
Copy link
Owner

I forgot about persistency. If people use something like audio or paused persistency for top bar than this makes sense.

To comment on the actual PR, we need to account for:

  • volume bar
  • top bar displaying multiple lines of titles (subtitle, chapter title)

@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Apr 6, 2023

Left and right osc-margin as well as osd-margin-x now get set based on the volume slider.
The top bar and volume slider only cause a margin when they are persistent.
The top margin respects all title lines, not just the main one.

Also there was some stuff in the top bar that must have gotten copied over from the timeline but were unused, so I removed them.

Do you still want it to be configurable?
Should we only have a bottom margin for a persistent timeline/controls as well?

The margins were already set for `osc-margins` and
`user-data/osc/margins`, but osd messages have their own properties
`osd-margin-x` and `osd-margin-y`.
Offsetting `osd-margin-x` and `osd-margin-y` by our margin prevents
overlap of osd messages with the GUI.
Avoids a jumping margin when the top bar is only temporarily visible.
The margin now also respects multiple title lines (alt-title, chapter)
@christoph-heinrich
Copy link
Contributor Author

I've implemented the "bottom margin for persistent elements only" thing because I wanted to try it out.

@tomasklaen
Copy link
Owner

Hm... I'm not 100% sure we should margin this only for persistent elements. Does anyone else have an input on this? I'm indifferent.

@dyphire
Copy link
Contributor

dyphire commented Apr 7, 2023

My opinion is in line with @hooke007. It needs to be configurable. The jumping stats are really annoying.

@christoph-heinrich
Copy link
Contributor Author

When does it even jump anymore? The only annoying jumping I can see is when dragging the timeline because the player gets paused while doing so, triggering "paused" persistency. Maybe we want to avoid triggering persistency in that case? After all we avoid the paused indicator flashing as well.

But ok I'll add an option later, then everyone's happy.

@tomasklaen
Copy link
Owner

I don't think this is a candidate for a new option. I assume everyone want's to see the UI that is being displayed, so there shouldn't be an option to makes stuff impossible to read.

Instead we need to figure out the ideal compromise. It seems that it should probably be just a check for element.enabled.

@hooke007
Copy link
Contributor

hooke007 commented Apr 7, 2023

Haven't checked the latested commits yet.

Ideally, I already use the correct setting to make sure that my osd is always visible, then i don't want other scripts would force its position to be changed.
Which is the reason i support for configuration. Some people prefer fixed position rendering while others like dynamic one.

@tomasklaen
Copy link
Owner

All right. adjust_osd_margins that is on by default would make sense then.

@christoph-heinrich
Copy link
Contributor Author

Paused persistency now isn't triggered by dragging in the timeline.
New option adjust_osd_margins has been added.

@tomasklaen tomasklaen merged commit 6d58ef4 into tomasklaen:main Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants