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

Jump to bottom

[gui] no need to store/restore modeless dialogs while unload/load skin #12105

Merged
merged 3 commits into from May 16, 2017
Merged

[gui] no need to store/restore modeless dialogs while unload/load skin #12105

merged 3 commits into from May 16, 2017

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented May 16, 2017

There is no need to store/restore modeless dialogs while unload/load skin. A modeless dialog will automatically be opened/closed depending on its visible condition.

This also leads to problems if different skins use custom modeless dialogs with the same id (skin A and B use window id 1000) and you switch between this skins (see http://forum.kodi.tv/showthread.php?tid=314185)

@BigNoid mind taking a look.

@xhaggi
[gui] no need to store/restore modeless dialogs while unload/load skin
8ba5f23 
A modeless dialog will automatically be opened/closed depending on its
visible condition. This also leads to problems if different skins use
custom modeless dialogs with the same id (skin A and B use window id
1000) and you switch between this skins (see
http://forum.kodi.tv/showthread.php?tid=314185)
@xhaggi
[cosmetics] rename vars, use foreach, add comments in CApplication::L…
aa9e849 
…oadSkin
@BigNoid
Copy link
Member

BigNoid commented May 16, 2017

I see no regressions when testing, e.g. custom windows/dialogs still load when its visible after switching skins.

Not sure if it should be part of this PR or not, but I do think we need an extra function like CGUIWindowManager::RemoveFromWindowHistory() that can remove custom window id's from history when CGUIWindowManager::DeInitialize() is called. Right now if you switch to a skin from Estuary's addoninfo (navigated to through the main menu addon menu item) and you go back, you can go back to the new skin's custom window with id="1100" which can lead to unexpected results like a black screen if that window/dialog should not be visible.

@xhaggi xhaggi force-pushed the cleanup-restore-modeless-dialogs branch from 87c4564 to dacce08 Compare May 16, 2017 16:19
@xhaggi xhaggi force-pushed the cleanup-restore-modeless-dialogs branch from dacce08 to c48862b Compare May 16, 2017 16:23
@xhaggi
Copy link
Member Author

xhaggi commented May 16, 2017 •
edited

Added another commit to address your comment. It makes totally sense to remove custom windows from our window history while destroy them.

@BigNoid
Copy link
Member

BigNoid commented May 16, 2017

Works great, thx for adding that 👍

@xhaggi
Copy link
Member Author

xhaggi commented May 16, 2017 •
edited

Thanks for testing it.

@xhaggi xhaggi added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Fix non-breaking change which fixes an issue labels May 16, 2017
@MartijnKaijser
Copy link
Member

jenkins build this please

1 similar comment
@xhaggi
Copy link
Member Author

xhaggi commented May 16, 2017

jenkins build this please

@Rechi Rechi added the v18 Leia label May 16, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone May 16, 2017
@MartijnKaijser MartijnKaijser merged commit db1c544 into xbmc:master May 16, 2017
@xhaggi xhaggi deleted the cleanup-restore-modeless-dialogs branch May 17, 2017 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Component: GUI engine Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Milestone
Leia 18.0-alpha1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xhaggi @BigNoid @MartijnKaijser @Rechi