[pbureau]

Hi,

I am running a kodi Helix 14.02 build, and I am trying to call ActivateWindow() from the <onunload> tag in my VideoFullScreen.xml window definition, trying to invoke a specific window on video exit.

With all the windows I tried, kodi enters an infinite loop and ends up crashing. The only exception seems to be dialogs (it works with "movieinformation" for instance).

Is this a bug or am I missing something?

[Hitcher]

Have you tried using a conditional onload for all the other windows where the condition is Window.Previous(fullscreenvideo)?

[ronie]

Hi,

I am running a kodi Helix 14.02 build, and I am trying to call ActivateWindow() from the <onunload> tag in my VideoFullScreen.xml window definition

when you exit fullscreen, kodi will activate the previous window.
if you add add another ActivateWindow() through onunload on top of it, odd things can happen i guess.

[pbureau]

ok, after a look at the code, this behavior is normal.

• unload actions are executed in the: OnMessage(), GUI_MSG_WINDOW_DEINIT, callback for the current window.
  
• That calls CGUIAction::ExecuteActions which loops through all the <onunload> statements
  
• When ActivateWindow() is called, it runs CGUIWindowManager::ActivateWindow_Internal()
  
• This calls CloseWindowSync() for the current window, which runs CGUIWindow::Close_Internal()
  
• Finally this sends a GUI_MSG_WINDOW_DEINIT message... closing the loop
  

I am not sure if this was intended from the developers, but it seems like a pretty open way to crash the application with a bad xml file. At the very least it should be documented.

I am going to propose a patch for this, a submit a pull request...

[phil65]

ok, after a look at the code, this behavior is normal.

• unload actions are executed in the: OnMessage(), GUI_MSG_WINDOW_DEINIT, callback for the current window.
  
• That calls CGUIAction::ExecuteActions which loops through all the <onunload> statements
  
• When ActivateWindow() is called, it runs CGUIWindowManager::ActivateWindow_Internal()
  
• This calls CloseWindowSync() for the current window, which runs CGUIWindow::Close_Internal()
  
• Finally this sends a GUI_MSG_WINDOW_DEINIT message... closing the loop
  

I am not sure if this was intended from the developers, but it seems like a pretty open way to crash the application with a bad xml file. At the very least it should be documented.

I am going to propose a patch for this, a submit a pull request...

You are correct. Thanks for helping out!

[pbureau]

no problem.

By the way, is the <previouswindow> tag actually intended to provide this functionality? Which means ActivateWindow() is actually not meant to be supported inside the <onunload> tag??

This would be fine, except it seems that <previouswindow> will not work for dialogs (such as "movieinformation"), is this normal?

[phil65]

previouswindow can be used to override the window stack afaik. I´m not 100% sure what you intend to do but there are probably nicer ways than putting ActivateWindow() into an <onunload> element. Anyways, no matter if your approach makes sense or not, if you are able to crash Kodi with some xml code then it should get fixed.

[pbureau]

agreed, but do you think the fix should be to enable usage of ActivateWindow() inside <onunload>, or should <onunload> ignores ActivateWindow() actions (probably throwing a LOGINFO), thus only preventing the infinite loop?

[phil65]

either enabling the usage of ActivateWindow() or ignoring the call in case it would lead to a crash (for dialogs this works fine for example). Ignoring it in general is not a good idea since that could break skins.

[pbureau]

ok I have prepared (and going to submit) a patch for this, however I don't think that trying to support ActivateWindow() from the <onunload> tag actually makes sense. When the onunload actions are invoked, we are already in the ActivateWindow() execution of the stack previous windows, that would need to be aborted and replaced by the action defined window. The fact that dialogs don't break the application is more a side effect than because it has been designed that way.
The <previouswindow> tag is really meant to provide this functionality in skins...

The proposed patch simply ignores ActivateWindow() statements.