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

[VideoOSD] Add autoclose setting #20267

Merged
merged 1 commit into from Dec 19, 2021
Merged

[VideoOSD] Add autoclose setting #20267

merged 1 commit into from Dec 19, 2021

Conversation

enen92
Copy link
Member

@enen92 enen92 commented Oct 6, 2021

Description

Any modern video player (or video application) autocloses the video OSD after a small period of inactivity by the user. In KODI this seems to be skin dependent. Some skins implement the feature, others do not. Personally, I think this has a broader scope than skins (it defines behaviour not visual aspect) and should be provided as an option in the application itself.

  • I am even unsure if it shouldn't be enabled by default (currently it's off). What do you think?
  • Also unsure about the location of the setting. Does a new category "Video OSD" make sense? It's odd to be under "Skin"

Motivation and context

There are too many clicks involved in the OSD, usually the user just wants to watch its own media. For instance, when downloading a subtitle or selecting a subtitle, the OSD buttons will be left on the screen and the user has to explicitly hit back to watch the content.

How has this been tested?

Runtime tested

What is the effect on users?

Slicker user experience if the new setting is enabled

Screenshots (if appropriate):

image

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@enen92 enen92 added Type: Feature non-breaking change which adds functionality Component: GUI engine v20 Nexus labels Oct 6, 2021
@enen92 enen92 added this to the Nexus 20.0 Alpha 1 milestone Oct 6, 2021
@enen92 enen92 added the hacktoberfest-accepted Hacktoberfest tagging label Oct 6, 2021
@jjd-uk
Copy link
Member

jjd-uk commented Oct 7, 2021

In my view Off is the correct Default as that is what emulates current behaviour.

xbmc/settings/Settings.h Outdated Show resolved Hide resolved
xbmc/video/dialogs/GUIDialogVideoOSD.cpp Outdated Show resolved Hide resolved
@ksooo
Copy link
Member

ksooo commented Oct 8, 2021

Is this only of interest for video OSD? What about music OSD?

And for PVR we already have such a setting. Maybe it's time to consolidate stuff?
screenshot00004

@jjd-uk
Copy link
Member

jjd-uk commented Oct 8, 2021

Music OSD behaves differently from Video, so if that was touched care would be to be taken to not break current behaviour. OSD/Info can be switched to permently on/off with a toggle of the appropiate button similar to video, however in the case of the Info not being displayed then Kodi will always display the Info on a track change for 10 sec before hiding again.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 16, 2021
@Ch1llb0
Copy link

Ch1llb0 commented Oct 19, 2021

I am even unsure if it shouldn't be enabled by default (currently it's off). What do you think?

In my view Off is the correct Default as that is what emulates current behaviour.

My skin is one of those which offer this feature currently and I'd tend to say it shouldn't be 'off' by default. It's a change, so why not go all the way and also implement a new behaviour? The OSD auto hiding is something many people have requested on our forums and it seems sensible to me to make that a default - which time period to set as default would be a separate question.

@Ch1llb0
Copy link

Ch1llb0 commented Oct 19, 2021

Music OSD behaves differently from Video, so if that was touched care would be to be taken to not break current behaviour. OSD/Info can be switched to permently on/off with a toggle of the appropiate button similar to video, however in the case of the Info not being displayed then Kodi will always display the Info on a track change for 10 sec before hiding again.

Tbh, from a personal standpoint, the current music OSD behaviour is not really something that makes too much sense, especially from a UX standpoint. There's absolutely no feedback whatsoever whether you've set the OSD to stay open permanently or only for the (fixed!) 10 seconds atm. And the music OSD for radio behaves differently again... Couldn't this be a chance to streamline behaviour of all OSDs?

@enen92
Copy link
Member Author

enen92 commented Dec 11, 2021

@ksooo thinking a bit more about this I don't see a way of making it consistent and include PVR at the same time.
I am tempted to create a new section in Settings/Interface dedicated to Player OSD (and create a separator called "Video" inside). However, I think the PVR behaviour when switching channels likely should stay in the PVR section of settings - not everyone has PVR enabled and the behaviour that setting defines is specific to an action that affects "objects" that only have meaning in PVR (channels). Repeating the setting in both places is also an option though (I'd try to avoid it as much as possible).

Regarding music I won't be touching/changing behaviour with this PR for now but it can fit well in a "Music" separator within this new "Player OSD" main menu section.
It might look a bit empty for now with just this setting but I can see a few more that can be implemented. For example, currently with commbreaks a kaytoast is shown when a commbreak is skipped with no option to avoid it. Some requests have been made in the forum to have an option to disable it.

@Ch1llb0
Copy link

Ch1llb0 commented Dec 12, 2021

It would also be nice to be able to adjust the time until the music OSD is hidden... This is currently fixed and there's not setting to change it. Such a new section would make sense IMHO, @enen92 👍🏻 Put all OSD related settings for PVR, video and music there.
Wouldn't this page make more sense under Settings/Player though? As it's player related, users might look there first and wonder why it's not there...

@enen92
Copy link
Member Author

enen92 commented Dec 12, 2021

Because it's not related nor defines the player behaviour. It's only used to define the GUI operation (so the interface)...it just happens to be on the windows that have the player control (in this case the video osd).
Maybe better to call it only OSD to avoid confusion with the Player section.

@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Dec 14, 2021
@enen92
Copy link
Member Author

enen92 commented Dec 14, 2021

Rebased

  • Created new category "OSD" under "Settings/Interface" and a setting group dedicated to "Video" (as I'm only applying this behaviour on the video osd window). Music can be added later to a new group if someone is interested.
  • Changed the setting to close the video OSD as true by default; Setting for elapsed time for close has a 5 second default value
  • Setting to close the video OSD belongs to basic settings level
  • Setting to define the elapsed close time belongs to advanced settings level
  • Applied ksooo comments

image

image

Feedback appreciated

@enen92
Copy link
Member Author

enen92 commented Dec 16, 2021

Changed default Video OSD close time to 5 seconds.
Auto close video OSD setting is a Basic level setting. Elapsed time for autoclosing the video OSD has been moved to Advanced level.
See comment above.

I plan to merge this on Sunday night if no opposition/changes are requested.

@enen92
Copy link
Member Author

enen92 commented Dec 19, 2021

Jenkins build this please

@jjd-uk
Copy link
Member

jjd-uk commented Dec 19, 2021

Looks good from the settings xml side of things. Can't say anything about the C++.

@enen92 enen92 closed this Dec 19, 2021
@enen92 enen92 reopened this Dec 19, 2021
@enen92 enen92 merged commit 3b36f60 into xbmc:master Dec 19, 2021
@Hitcher
Copy link
Contributor

Hitcher commented Apr 23, 2022

@enen92 A user has raised a good point on the forum that different skins can have other dialogs open at the same time to OSD is visible and they remain open when the OSD gets closed automatically. Maybe this should close all open dialogs as well?

https://forum.kodi.tv/showthread.php?tid=367983

@enen92
Copy link
Member Author

enen92 commented Apr 23, 2022

As far as I can tell there is no clean way to close all other dialogs when this happens from the core, specially because you don't know when they were open. Which dialogs are we talking about? The only way is to make those dialogs somehow depend on the visibility of the video osd.
I already thought a bit about this PR and somehow I think the intention was good but it doesn't belong to the core... should in fact be handled by the skins. However there is also no clean way to implement this in the skin at the moment without relying on python scripts. That is not acceptable to a default skin like estuary due to the performance penalty. My idea is to have something like skin timers that would trigger builtins on elapsed time depending on Boolean conditions.
Maybe for now we should just revert this one since the first alpha is not yet out and work on a proper way to support timed based stuff on the skinning engine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: GUI engine hacktoberfest-accepted Hacktoberfest tagging Type: Feature non-breaking change which adds functionality v20 Nexus Wiki: Done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants