Fix xbmcvfs for write binary data to File. #2138

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@ulion

the swig support typemap for such situation, but seems xbmc not use swig generate code, instead output xml file and then use groovy code with some code copied from swig do the real work which seems not support multiple params typemap.
so, for support binary data write for the xbmcvfs module, I had to write a patch like the read method did, here it is.

@MartijnKaijser
Team Kodi member

@jimfcarroll
thoughts?

@jimfcarroll jimfcarroll was assigned Jan 30, 2013
@jimfcarroll
Team Kodi member

I addressed this prior to Frodo but the change was (rightly) deemed to large to go in.

I haven't had a reason to support multi-param typemaps but I guess it makes sense in this case (since this kind of thing is what they're for). Instead I have a memory buffer class which encapsulates the void* and length parameters and a typemap in terms of that. Here is the commit: jimfcarroll@53f15d2

@ulion, what do you think?

@jimfcarroll
Team Kodi member

Oh. In case it's not obvious, in python the memory buffer can be either a string or a python bytearray and it should work the way you'd expect.

@ulion

as long as it resolve the binary write problem, it should be ok.
I'm just wondering, why xbmc did not use swig generate cpp code directly?

@ulion

@jimfcarroll when will your one be merged in? how about use this one before yours is in?

@jimfcarroll
Team Kodi member

#2359 ... I need @bobo1on1 to make sure RenderCapture works with this.

@ulion

I saw https://github.com/xbmc/xbmc/issues/2137 and then I'd like to fix this in 12.1, that means either #2359 or this pr should be pushed in @jimfcarroll

@jimfcarroll
Team Kodi member

Yes. I have the fix for this but it will need some testing.

@jimfcarroll
Team Kodi member

How does this look: jimfcarroll@58a53c9

@ulion

how about this one go in first and backport to 12.1, and your one after this but not backport?

@jimfcarroll
Team Kodi member

I'd rather not have the API change after 12.1. It's not right in 12 so it shouldn't be used anyway but it should be right in 12.1. I'm less concerned about the particular implementation than a consistent API.

Plus, I think the other one works, there's just some suggestions you made. All I need is someone to verify the render capture functionality.

@jimfcarroll
Team Kodi member

Well, I think my PR is in acceptable shape to go in. I will have the last few things fixed today (within the next 24 hours). I will list it on the forum thread.

@ulion

well #2359 replaced this one and has been merged.

@ulion ulion closed this Mar 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment