[gui][info] Remove Player.DisplayAfterSeek #21425
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
enen92
added
Type: Cleanup
16 tasks
garbear
approved these changes
May 22, 2022
There was a problem hiding this comment.
This was an easy one to review. 9 lines added, 101 lines deleted. All deleted lines are GUI stuff intermingled with VideoPlayer, and removing GUI dependencies from VP is definitely needed. Only lines added are comments documenting the removal. This looks like a clean, advantageous removal, so +1 from me.
garbear
reviewed
May 22, 2022
Forcing visibility conditions on window controls is wrong as they must be added be the skin implementation. Progressbar, time and buffering controls were removed and must be implemented on the xml definition of the window if the skinner so desires.
enen92
force-pushed
the
removedisplayafterseeknexuss
branch
from
89e34c6
to
6673147
Compare
jojobrogess
added a commit
to jojobrogess/skin.auramod
that referenced
this pull request
Feb 14, 2023
https://forum.kodi.tv/showthread.php?tid=363553&pid=3098753#pid3098753 2022-05-22 - Removed Player.DisplayAfterSeek boolean condition The old Player.DisplayAfterSeek boolean condition was a design flaw of Kodi since it was coupling the videoplayer with GUI components. It was removed and replaced by the previously added Player.HasPerformedSeek(interval) boolean condition. By default Player.DisplayAfterSeek was valid for 2.5 seconds after a seek. To retain similar behaviour please use Player.HasPerformedSeek(3). Estuary and estouchy were adapted in xbmc/xbmc#21380 PR: xbmc/xbmc#21425
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The aforementioned infolabel is a design flaw of VP according to FernetMenta (see: #20753 (comment)) as it ties the player with the info interface.
Player.HasPerformedSeekwas introduced in #21366 as an alternative. Usages ofPlayer.DisplayAfterSeekwere removed from builtin skins (estuary and estouchy) in favor of the new infobool in #21380.This further removes all usages and the main logic of
DisplayerAfterSeekfrom kodi core.While the cleanup is smooth and should not cause regressions there is a little issue in the last commit (89e34c6):
In VideoFullScreen window we were forcing visibility conditions manually on static controls. While I could have just replaced the visibility conditions with the new one I don't think it is correct. Skinners should have full control of their skin elements, i.e., shouldn't be the core to dictate when to show/hide a control. So...decided to remove them all together. Note that the big majority of skins on our repo (nexus and matrix) do not use those controls. The only two exceptions are:
skin.pelucid by @TheDeadman
https://github.com/xbmc/repo-skins/blob/matrix/skin.pellucid/1080i/VideoFullScreen.xml#L82-L92
skinner must add
<visible>player.caching</visible>to retain same behaviorskin.amber by @bartolomesoriano
https://github.com/xbmc/repo-skins/blob/matrix/skin.amber/1080i/VideoFullScreen.xml#L74
Unsure about this one. It uses the static ids we're using but with a different type (image vs label). Likely nothing needed. Please confirm @bartolomesoriano
Motivation and context
Cleanup. Decouple stuff.
How has this been tested?
Runtime tested in estuary.
What is the effect on users?
Skinners should adapt the usage of
Player.DisplayAfterSeektoPlayer.HasPerformedSeek(interval)