1.23.0 MatroskaTagInfo uses the wrong casing for tags (1 Viewer)

doskabouter

Development Group
  • Team MediaPortal
  • September 27, 2009
    4,566
    2,938
    Nuenen
    Home Country
    Netherlands Netherlands
    That's true for the most part.
    My concerns are not about code size, but more that it takes effort to keep that stuff working.
    Especially in a year or so when the devs do not have the old (wrong) format xml's on their system, we still have to make sure reading old style xml's still works.

    But then again, the code to do that is fairly simple, devs just have to keep this in mind when fixing/refactoring that area.

    To summarize the issue: when downloading metadata from imdb for videos, the metadata is stored and read in a wrong format (xml next to the video file). Onlinevideos also stores metadata but in the correct format. when mixing those 2 (f.e. when you have the onlinevideos download folder also in the videos section), the videos section can't find the metadata and does not display the title and description.

    I will add a backwards compatibility mode to my fix.
     

    doskabouter

    Development Group
  • Team MediaPortal
  • September 27, 2009
    4,566
    2,938
    Nuenen
    Home Country
    Netherlands Netherlands
    Added support for reading old-style xmls.

    The only thing left is that if users install newest MP, and add fetch metadata from imdb for their videos, and after that downgrading MP the title and description will not show for those videos.
    Assuming they will be upgrading sooner or later, that issue will resolve itself.

    Only thing is, I can't test this properly as I have no old xml files present on my system. But I'm fairly confident that this will work...
     

    doskabouter

    Development Group
  • Team MediaPortal
  • September 27, 2009
    4,566
    2,938
    Nuenen
    Home Country
    Netherlands Netherlands
    Ah crap.
    Just noticed there are three places where that xml is read...
    MediaPortal/MediaPortal-1
    internal in MediaPortal.GUI.Video namespace
    added a really long (>10 years) ago
    and
    MediaPortal/MediaPortal-1
    public in MediaPortal.Video.Database namespace
    added about 7 years ago
    and
    MediaPortal/MediaPortal-1
    public in TvDatabase namespace
    added a really long (>10 years) ago

    Codewise its better to remove the one in the IMDBMovie file, because I think its not at its proper place, but that one is public, so possibly used by other plugins...
    The one in MatroskaTagLib is nowhere used in mediaportal, and because it's internal, there's no worries about other dependencies failing.

    Pff. and another thing I noticed: MatroskaTagHandler.Persist is never called from mediaportal at least (being a public method, theoretically it could be called from somewhere else). So at least in recent mediaportals there is never going to be an xml file in the old format.

    However, the only place where I found that such a xml file is written is in the tvservice, for recorded tv.
    That's one area I don't like to change because of lots more dependencies (MP2 for example), but it gives me a good place to test!

    So my solution is (as is committed now to my pullrequest Bug mp1 4977 matroska tag info uses the wrong casing for tags by doskabouter · Pull Request #182 · MediaPortal/MediaPortal-1):
    removed the MatroskaTagLib.cs from mediaportal solution
    removed persist code from mediaportal
    removed unused properties from IMDBMovie's MatroskaTagInfo
    added support for correct and incorrect xml-files.

    So from my point of view this is mergeable without any regressions.
     

    doskabouter

    Development Group
  • Team MediaPortal
  • September 27, 2009
    4,566
    2,938
    Nuenen
    Home Country
    Netherlands Netherlands
    this couldn't be used in other tool as all the methods were marked as internal.
    and if needed again it's easy enough to add it. Perhaps then we could do a bit of refactoring because now reading and writing matroskatags is in 3 places all slightly different
     

    azzuro

    Test Group
  • Team MediaPortal
  • May 10, 2007
    9,948
    5,617
    France - IDF
    Home Country
    France France
    and if needed again it's easy enough to add it. Perhaps then we could do a bit of refactoring because now reading and writing matroskatags is in 3 places all slightly different
    can be great for me, if you done change can you base your work on test4+PR please
     

    Users who are viewing this thread

    Top Bottom