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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gui] fix missing path param in CGUIDialog::Open() after 447ec5b #8194

Merged
merged 1 commit into from Oct 16, 2015

Conversation

Projects
None yet
8 participants
@xhaggi
Copy link
Member

xhaggi commented Oct 10, 2015

As pointed out by @jmarshallnz (thanks for that 馃槃) at 447ec5b#commitcomment-13699848 this will fix the missing path parameter in CGUIDialog if called by ActivateWindow(dialog_id, /some/path/here) e.g. for CGUIDialogSmartPlaylistEditor.

@mkortstiege mind taking a look.

@xhaggi xhaggi force-pushed the xhaggi:fix-dialog-path-param branch 2 times, most recently from c3d808a to bed56a2 Oct 10, 2015

@xhaggi xhaggi added the Type: Fix label Oct 10, 2015

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Oct 10, 2015

+1. jenkins build this please.

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 10, 2015

Was testing this PR to see if it improved http://trac.kodi.tv/ticket/16314

  • However this just makes the working dialog permanently visible and active during active playback
  • Another interesting side effect is that during said payback all manners of OSD are also impossible to bring up.

screenshot008

So here hoping this isnt merged as is.

@jmarshallnz

This comment has been minimized.

Copy link
Member

jmarshallnz commented Oct 10, 2015

Code changes seem fine (and unrelated to whatever @un1versal is commenting on).

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 11, 2015

Code changes seem fine (and unrelated to whatever @un1versal is commenting on).

Thats fine, but with this PR ontop of master you will break those things, Im not imagining them.

@jmarshallnz

This comment has been minimized.

Copy link
Member

jmarshallnz commented Oct 11, 2015

Ah, looks like it's missing some overrides (busy/progress at least) which would explain what @un1versal is seeing.

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 11, 2015

will check it asap thanks @un1versal

@xhaggi xhaggi force-pushed the xhaggi:fix-dialog-path-param branch from bed56a2 to 894ee6e Oct 11, 2015

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 11, 2015

I didn't changed the overrides in busy and progress instead I've added two more overloads instead of using a default for the path parameter. This make it easier to extend from CGUIDialog because you don't need to take care of the new path parameter.

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 11, 2015

@un1versal could you please do another try?

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 11, 2015

Sure give me some time to compile.

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 11, 2015

However this just makes the working dialog permanently visible and active during active playback
Another interesting side effect is that during said payback all manners of OSD are also impossible to bring up.

@xhaggi - Still both apply, sadly codecinfo also not working while the working dialog is up btw.

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 11, 2015

are you sure you did it right? I've force pushed the changes and now it should work like without this PR

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 11, 2015

Well soon as you messaged me I added the patch again and re-compiled. once it was compiled I restarted Kodi and retested.

I guess I can do it again jst to make sure.

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 11, 2015

ok will do some testing tomorrow too, thanks in advance

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 11, 2015

Well, here is a retest, more in depth.

local files this is OK, but via NFS/SMB triggers the working dialog and thats all she wrote.

What happens exactly is this.

Press play on any file target NFS/SMB source in your library.
If the source is span down disks even better, you will see working dialog come up, then briefly after pressing play, there's a moment where the working dialog disappears (as it transitions from kodi gui to player), then as soon as the playback really starts (1 second later), bingo working dialog shows up again and stays there for remainder, and you cant bring up codec infor or full OSD if you press info you may be lucky enough to see that minimal info screen..

Yesterday, I didnt test with local files so have no means of comparison.

@jmarshallnz

This comment has been minimized.

Copy link
Member

jmarshallnz commented Oct 11, 2015

Given you're only calling Open() with the parameter in ActivateWindow(), this can't possibly work, no?

You must override the function you're actually calling from outside the class. In this case, Open is always called with the parameter from outside the class, so you either need to call the parameter-less one from inside the one with the parameter (doesn't make sense) or override the parameter version.

Note that only Open_Internal() need be virtual (just like only DoModal_Internal was virtual before).

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 11, 2015

meh you are right, don't know why i thought this will work 馃槃

@xhaggi xhaggi force-pushed the xhaggi:fix-dialog-path-param branch 2 times, most recently from 83a2a65 to cd3c7c7 Oct 11, 2015

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 11, 2015

okay thanks @jmarshallnz

@un1versal I've force pushed the necessary changes. a new test round would be much appreciated.

@xhaggi xhaggi force-pushed the xhaggi:fix-dialog-path-param branch from cd3c7c7 to 375a918 Oct 11, 2015

@@ -83,5 +83,5 @@ class CGUIDialogKaraokeSongSelectorLarge : public CGUIDialogKaraokeSongSelector
{
public:
CGUIDialogKaraokeSongSelectorLarge();
void Open();
void Open(const std::string &param = "");

This comment has been minimized.

@jmarshallnz

jmarshallnz Oct 11, 2015

Member

This one isn't an override by the looks (Open in base isn't virtual) so the only way this one does anything is if it's getting called on the class directly (which, given the karaoke code, is almost certain).

It doesn't hurt I guess, but also could cause confusion with folk thinking it's an override when it's not (and shouldn't be in my view). I'd track down why this is even there. By the looks something to do with the fact that the Small version takes an integer into it's (also non-overriding) version of Open().

This comment has been minimized.

@Montellese

Montellese Oct 12, 2015

Member

To avoid this kind of confusion or human error please start using the override keyword to mark methods that are meant to override a virtual method of a base class. The compiler will then throw an error if it doesn't actually override an existing virtual method (e.g. because the base method is not virtual, or the parameter list doesn't match).

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 11, 2015

@xhaggi jm's comments not withstanding, from my part this is looking -- and feeling, right as rain. 馃憤 馃憤 It even feels faster.

@xhaggi xhaggi force-pushed the xhaggi:fix-dialog-path-param branch from 375a918 to 561219f Oct 12, 2015

@xhaggi xhaggi force-pushed the xhaggi:fix-dialog-path-param branch from 561219f to 5f481cc Oct 12, 2015

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 12, 2015

@jmarshallnz reverted the changes to GUIDialogKaraokeSongSelector.cpp

@un1versal again, please do a new test round 馃槃

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 12, 2015

@Montellese thanks for your hint to use the keyword override. I'll keep it in mind for new PR's.

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 12, 2015

In addition to the other tests, added karaoke to list 馃憤 馃憤

@xhaggi

This comment has been minimized.

Copy link
Member Author

xhaggi commented Oct 12, 2015

jenkins build this please

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Oct 13, 2015

Build issue on jenkins is unrelated. Guess this one is good to go?

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Oct 16, 2015

jenkins build and merge.

@jenkins4kodi jenkins4kodi merged commit 08c824b into xbmc:master Oct 16, 2015

1 check was pending

default Merged build started.
Details

@Razzeee Razzeee added the v16 Jarvis label Oct 22, 2015

@xhaggi xhaggi deleted the xhaggi:fix-dialog-path-param branch Feb 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.