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

[gui] fix activation of window if top most modal is dialog busy #7380

Merged
merged 3 commits into from Jul 3, 2015

Conversation

Projects
None yet
4 participants
@xhaggi
Member

xhaggi commented Jul 2, 2015

In some cases it happens that the busy dialog is already open before the window activation kicks in.
This will results in a refuse of the activation and some pain for different addon devs because they need to hack around that issue.

This is an attempt to overcome the root cause and adds a new condition to the check for open modals.

Related Trac ticket http://trac.kodi.tv/ticket/15960

@xhaggi xhaggi added the Fix label Jul 2, 2015

@xhaggi xhaggi added this to the Isengard 15.0-rc1 milestone Jul 2, 2015

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 2, 2015

@mkortstiege or @Montellese mind taking a look please

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 2, 2015

i've added a RFC commit which drops the static comparison against WINDOW_DIALOG_BUSY in CGUIWindowManager and introduces a new modal dialog type to differ between modals. Currently I've added a default type NORMAL and a second type SYSTEM where dialog busy is a system modal. I've also adjust the method CGUIWindowManager::HasModalDialog to accept a list of types to check for and call it in CGUIWindowManager::ActivateWindow_Internal only for NORMAL modals. This implementation will gives us more flexibility in case we need more types or handling another dialog like the busy dialog.

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 2, 2015

jenkins build this please ... i want to check if everything works fine on all platforms

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 2, 2015

@Paxxi mind taking a look too

@Paxxi

This comment has been minimized.

Member

Paxxi commented Jul 2, 2015

Not familiar with the issue but it looks like a nice, clean and extensible way to solve it.

@MartijnKaijser MartijnKaijser modified the milestone: Isengard 15.0-rc1 Jul 2, 2015

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 3, 2015

jenkins build this please

@mkortstiege

This comment has been minimized.

Member

mkortstiege commented Jul 3, 2015

+1

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 3, 2015

jenkins build this please

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 3, 2015

I've done a small optimization to the current modality handling. I've replaced the flag m_bModal which is currently used to indicate if the dialog is modal or modeless with a new enum based member m_modalityType. This enum holds the values MODELESS, MODAL and SYSTEM_MODAL. Now it's a bit easier to configure the modality of a dialog (without setting the flag and type like i did in my first approach).

xhaggi added some commits Jul 2, 2015

[gui] introduce a dialog modality type to differ between modalities
In some cases it happens that the busy dialog is already open before the window activation kicks in.
This will results in a refuse of the activation and some pain for different addon devs because they need to hack around that issue.

This is an attempt to overcome the root cause. It adds a new dialog modality type as a replacement for the current modal flag and
specifed a new type SYSTEM_MODAL which is left-out at the check in CGUIWindowManager::ActivateWindow_Internal
@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 3, 2015

jenkins build this please

@Paxxi

This comment has been minimized.

Member

Paxxi commented Jul 3, 2015

you should be able to skip the assignments in Show_Internal as the fields are already initialized in the constructor. Still looks like a good change.

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 3, 2015

depending on the method you call CGUIDialog::DoModal() or CGUIDialog::Show() the dialog's type switch between modal and modeless except some special dialogs like BusyDialog which overload this.
I don't know why it was done like this, but IMO a dialog is either a modal or modeless, not sometimes modal and sometimes modeless.

@xhaggi

This comment has been minimized.

Member

xhaggi commented Jul 3, 2015

As this needs to be back-ported for 15.x i'll take a look at unifying the Show() and DoModal() methods later. If no objections i'll push this in and do a back-port PR.

xhaggi added a commit that referenced this pull request Jul 3, 2015

Merge pull request #7380 from xhaggi/fix-activate-window-with-busydialog
[gui] fix activation of window if top most modal is dialog busy

@xhaggi xhaggi merged commit f29e373 into xbmc:master Jul 3, 2015

1 check passed

default Merged build finished.
Details

@MartijnKaijser MartijnKaijser added this to the Isengard 16.0-alpha1 milestone Jul 4, 2015

@xhaggi xhaggi deleted the xhaggi:fix-activate-window-with-busydialog branch Jul 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment