[GUI] Fix ActivateWindow if 'return' is defined

[enen92]

Description

This pull request fixes the usage of ActivateWindow(window, dir, return) if the action is executed from context menu items (which according to the bug report is there since krypton).

This is mainly the case in python addons where listitems are created (for example) with the ActivateWindow(xxxx, plugin://plugin.video.foo/bar, return) as context menu actions. Other examples are any other dir paths to MusicLibrary and VideoLibrary.

According to the documentation, it is expected that Kodi moves back to the plugin directory from which the action was executed. However this doesn't work because:

• Kodi always computes the history for the provided path even if we specify the return parameter (e.g. we provide "plugin://plugin.video.foo/bar" as dir, Kodi will create the respective history: plugin://plugin.video.foo/bar ../ plugin://plugin.video.foo)

• Kodi always defines the provided dir as the root folder. This works well if we are activating a different window than the one that is already active. If they are the same, the root folder of dir should be set to the path that was on the media window before activating this new one otherwise we won't be able to move back to where we were.

@mediaminister @dagwieers since you were the ones reporting the issue, it'd be nice if you guys could test it.

I am unsure who I should ping for review, this seems no-man's code by now.

Motivation and Context

Fixes #16492

How Has This Been Tested?

Context menu item created in a plugin that pointed to another plugin:

    myitem = xbmcgui.ListItem("TEST")
    myitem.addContextMenuItems([("move to programs", "ActivateWindow(Videos,plugin://plugin.video.vrt.nu/programs/emma/allseasons, return)")])
    xbmcplugin.addDirectoryItem(handle=plugin.handle, listitem=myitem, isFolder=True, url=plugin.url_for(live))

Confirmed the onBack action moved back to where I was before. Also tested library navigation (movies and tvshows) in estuary and confirmed the behaviour is still the same.

Screenshots (if appropriate):

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

[DaveTBlake]

Take care you don't break other behaviour with this @enen92 , or ways in which skinners have become used to using this and Kodi behaving. I am unable to test immediately, and @the-black-eagle may be interested

[enen92]

@DaveTBlake I was about to ping you. Unfortunately we never know if regressions are introduced. This needs extensive testing and won't be merged unless it's proven okay. I'll ask in #skinning and maybe #testing

[DaveTBlake]

Sure @enen92 I wasn't saying don't, just aware from looking at other parts of the window/history/navigation behavour that it can bite back.

[mediaminister]

Because of this bug the VRT NU add-on moved to using Container.Update(plugin_url) which also works.
To test this I changed it back to ActivateWindow(Videos,plugin_url,return) and I can confirm this works now as it should be.

Thanks a lot for fixing this bug.

[enen92]

One "regression" found by @sualfred (or to be more precise something that didn't correspond to his expected behaviour - although still better than we have now):

Test case:
HOME -> TvShows -> Select a TVShow -> Select a random season -> JSON RPC: { "jsonrpc": "2.0", "method":"GUI.Activatewindow","params":{"window":"videos","parameters":["videodb://movies","return"]},"id":"1"} -> (moves to movies as expected)

Navigating back:

PR (old implementation): Back -> episode view -> back -> home
Master: Back -> HOME
Expected Behaviour: Back -> episode view -> (back) seasons -> back (shows) -> back (HOME)

I adjusted the code to comply to the expected behaviour.

Would still be nice to have some feedback on the music section.

[DaveTBlake]

Would still be nice to have some feedback on the music section

Understood @enen92 but been a bit busy with release and sunny summer weather. :)
Will take a look soonest.

[the-black-eagle]

Tested in the music section by opening various nodes with both musicdb:// and library://music/ type paths. Didn't see any regressions and with return defined always returned to the nav window that the node was activated from. Without return defined, some nodes return to the root node list when navigating before returning to the window they were called from.
LGTM but @DaveTBlake may spot something I missed :)

[DaveTBlake]

Did you check the recently adjusted version @the-black-eagle ?

[enen92]

Thanks @the-black-eagle (hopefully you cherry-picked today since I changed the implementation last night). @DaveTBlake no rush at all, was just a reminder I didn't test the MusicLibrary :).

This needs yet to be validated with ReplaceWindow yet as it shares the same implementation as ActivateWindow and I'm pretty sure the behaviour will not be 100% identical after my changes.

[Rechi]

Please follow the current code guidelines.

[enen92]

Tested with ReplaceWindow() and it behaves correctly too:

Test case1:
Home -> RunAddon -> "ReplaceWindow(videos,videodb://movies/titles, return)" on the addon entrypoint -> moves to movie titles

Test case2:
Home -> RunAddon -> "ReplaceWindow(videos,videodb://movies/titles)" on the addon entrypoint -> moves to movie titles

OnBack navigation for test case 1:
Back -> home

OnBack navigation for test case 2:
Back -> videodb://movies -> Back -> videos -> Back -> Home

Weather in estuary (where replace window is used) also behaves identical.

[enen92]

jenkins build this please

[DaveTBlake]

Sorry for the weeks of delay @enen92, tested out the music library navigation and all looks fine to me

[enen92]

Thanks a lot @DaveTBlake for testing and no worries about the time taken!

[Nanomani]

No Backport ?

[dagwieers]

@enen92 Can we backport this to Leia?

[enen92]

Although I didn't spot any regressions I'd prefer not to. It's better be prepared to find any regression in v19 development cycle than to introduce regressions on a version that is supposed to (TM) be the last one (leia). But you are always welcome to bring it as a pull request and see what others have to say about it. Personally I think it's time to move on and focus 100% on matrix

[dagwieers]

I understand, but we add-on developers will have to work with Leia for a few years, and it helps if we can reduce the number of workarounds in our code.

(Also when we proposed a fix for v18.3 someone used this argument because it was supposed to be the final update. Things do not always go as planned 😉)