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

Fix infinite loop when listitem action is "Play" #14653

Merged
merged 1 commit into from Oct 23, 2018

Conversation

@garbear
Copy link
Member

commented Oct 19, 2018

This fixes an infinite loop, noticed in the Game OSD, when clicking the "Play" item.

The Game OSD is a static list container. The top item has "Play" as a click action:

<control type="list" id="1103">
	<content>
		<item id="2101">
			<description>Pause / Resume button</description>
			<label>$LOCALIZE[35224]</label>
			<label2>Select + X</label2>
			<icon>osd/fullscreen/buttons/play.png</icon>
			<onclick>Play</onclick>
		</item>

The problem is the list container considers Play to be equivalent to Select. The GUI receives messages before the player, so it intercepts the Play action as a Select action. This executes the OnClick action, which is Play, and the process continues ad infinitum.

This bug was introduced in #14513.

Motivation and Context

Discovered while testing Beta 4.

How Has This Been Tested?

Tested on OSX with several cores.

Screenshots (if appropriate):

Infinite loop:

screen shot 2018-10-19 at 12 51 10 pm

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)
This fixes an infinite loop, noticed in the Game OSD, when clicking the
"Play" item. Actions are sent to the GUI before the player, and the GUI
(CGUIBaseContainer) considers the "Play" action to be a "Select" action.
It then resolves the "Select" action into "Play", which causes an infinite
loop.
@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

I would like a game only fix instead of reverting this nice feature for all components. Do you think this is feasible?

@garbear

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Well, let's think about this. "Play" is meant for the player, so whenever something is playing the player should receive the action. However, CApplication dispatches actions to Window Manager before the player. So, it's possible that CGUIBaseContainer checks if there's a player, and if not then handles the Play action. However, that adds an ugly dependency on player to the GUI code.

Alternatively, CApplication could send player actions to the player before the GUI. However, this probably has unintended side effects, and I wouldn't make this change for beta 4.

If we wanted to special-case the Game OSD, then we could have it intercept the Play command and dispatch it over the GUI bridge to the player that I built. However, this would still cause an infinite loop if a skin used the "play" action in a list container in a video or audio OSD.

@garbear

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

I think the problem is deeper, it doesn't matter if a player is active, when we get to this line, the container tries to send all actions to OnClick(), and relies on OnClick() to only handle legitimate "click" actions, like Select or mouse clicks. This is a problem anytime the container control has a select-like action for its <onclick> actions in the skin. So, my guess is that there is currently an infinite loop if the item has <onclick>Select</onclick> - but Select doesn't make sense as a click action, so we probably won't ever see this infinite loop.

It actually makes sense for Kodi to infinite-loop in this case, because Select executes <onclick>, and <onclick> resolves to Select, to infinity. We could blacklist "Select" for on-click actions, but I don't think we should blacklist "Play", because Play has behavior apart from Select. #14513 makes Play behave like Select, hence the infinite loop.

@garbear garbear added this to the Leia 18.0-beta4 milestone Oct 21, 2018
@MartijnKaijser MartijnKaijser removed this from the Leia 18.0-beta4 milestone Oct 23, 2018
@garbear

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

@ksooo are you ok with removing play from the set of "Select" actions? the only solution I see is to pass actions to the player before the GUI, and I'm not sure of the implications for this.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Yes, fine by me. Let's look for a better implementation of that feature for v19.

@garbear garbear merged commit 59307cf into xbmc:master Oct 23, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@garbear garbear deleted the garbear:fix-infinite-loop branch Oct 23, 2018
@Rechi Rechi added this to the Leia 18.0-beta5 milestone Oct 23, 2018
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.