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

Use override, fix interceptor class #13542

Merged
merged 2 commits into from Feb 14, 2018

Conversation

@notspiff
Copy link
Contributor

commented Feb 13, 2018

  • Trivial override usage
  • A fix in the interceptor class. Started out as a warning quell, but I cannot quell them all due to the construct in use. @jimfcarroll please look at the last commit.
@@ -120,7 +120,7 @@ namespace XBMCAddon
CGUIWindow* get() override { return this; }

// this is only called from XBMC core and we only want it to return true every time
virtual bool Update(const String &strPath) { return true; }
virtual bool Update(const String&, bool = true) { return true; }

This comment has been minimized.

Copy link
@Rechi

Rechi Feb 13, 2018

Member

override is now missing

This comment has been minimized.

Copy link
@akva2

akva2 Feb 13, 2018

Contributor

i tried to explain this in the pr text. this template is instanced both for a CGUIWindow and for a CGUIMediaWindow. for the former there is no method to override.

This comment has been minimized.

Copy link
@Rechi

Rechi Feb 13, 2018

Member

have overlooked the commit message

This comment has been minimized.

Copy link
@jimfcarroll

jimfcarroll Feb 13, 2018

Member

Perhaps it would be better to add a default virtual Update to CGUIWindow that just returns true and then mark it as override?

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 13, 2018

Author Contributor

i considered it but i kinda dislike inflating api due to user code concerns like this. i concluded i prefer the warning, no strong opinion though.

This comment has been minimized.

Copy link
@jimfcarroll

jimfcarroll Feb 13, 2018

Member

This is probably good to go but let me do a little more digging later today.

This comment has been minimized.

Copy link
@jimfcarroll

jimfcarroll Feb 14, 2018

Member

@notspiff

  1. Is it possible to remove this method (Update) from Interceptor all together? It will still be on WindowXMLInterceptor which is defined in the C++ file only, and bridges WindowXML to CGUIMediaWindow. Window doesn't define an Update and neither does CGUIWindow so it's not exposed to Python at that point anyway.

  2. It compiles with the method deleted.

  3. The scripting language side currently isn't getting the additional bool parameter (see WindowXML.h the WindowXML class). So the WindowXMLInterceptor is bridging CGUIMediaWindow::Update(String&, bool) <-> WindowXML::Update(String&). Should the additional parameter be available to the addon developers?

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 14, 2018

Author Contributor
  1. that's what i tried to asked above. as long as there is no usage of the base for WindowXML class, it will work just fine. the bool is a detail that should not be required for scripting windows as i doubt the filtering stuff is exposed in general.

This comment has been minimized.

Copy link
@jimfcarroll

jimfcarroll Feb 14, 2018

Member

Personally I'd say delete it but I'm only about 80% sure it wont have an unforeseen affect. Also, you should probably decide on (3) and if you think addon developers should see the additional parameter, add it to WindowXML.

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 14, 2018

Author Contributor

if you say so, that's what i will do. i don't want to expose the filtering bool, it's a quirk thingie for internal updates and as i said i doubt the filtering is exposed in general.

now we actually catch the call from the core to window update call.
@notspiff notspiff force-pushed the notspiff:warnings_and_stuff branch from 9d81ce6 to 1404473 Feb 14, 2018
@jimfcarroll jimfcarroll self-assigned this Feb 14, 2018
@jimfcarroll jimfcarroll removed their assignment Feb 14, 2018
@notspiff notspiff merged commit 9146d54 into xbmc:master Feb 14, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@Rechi Rechi added the v18 Leia label Feb 14, 2018
@Rechi Rechi added this to the L 18.0-alpha1 milestone Feb 14, 2018
@ronie

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

@notspiff this seems to be breaking some addon functionallity: https://forum.kodi.tv/showthread.php?tid=329208

mind having a look-see ?

@jimfcarroll

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

I'll try to look at it later tonight. We've been out of power since Friday so I can't promise anything. @notspiff took my advice and deleted it so I guess my comment where I was "80% sure there wouldn't be any unforeseen effects" was too optimistic. :-)

@jimfcarroll

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

I'm at a loss to understand how removing the Update call from the Interceptor had any effect on the behavior. Obviously I understood it when I originally wrote it. I hate to just put it back and not understand why. @notspiff can you explain what I'm missing?

@notspiff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

it makes no sense to me either, unless there is explicit calls to the base class instance of the method, instance->Base::foo() which i very much doubt there is

@notspiff notspiff deleted the notspiff:warnings_and_stuff branch Mar 7, 2018
@jimfcarroll

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

Explicit calls should have resulted in a compile failure once the method was removed. I remember there was a small handful (maybe 2) of methods I needed to stub out on the base Interceptor class but revisiting it now I can't figure out what problem that solved. We can just put it back in and see if that resolved it. Any other suggestions?

@notspiff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

As the interceptor did nothing due to signature changes i dont see what good that would do. Any error should come from it having been reactivated no?

@jimfcarroll

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

Maybe we're looking at the wrong thing. This change did modify the behavior:

https://github.com/xbmc/xbmc/pull/13542/files#diff-9095b1771d51a346264eb59ffb9cd5d2L86

The intention originally (I guess) was to disable the default CGUIMediaWindow::Update functionality for Python windows. When the bool parameter was added to CGUIMediaWindow::Update the Interceptor stopped disabling this for Update on Python WindowXMLs. Maybe someone is now relying on that behavior so when the blocking of the behavior was put back by your change, his code broke.

@notspiff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

That is what i tried to state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.