[mythtv-commits] Ticket #10250: Big Mythgame patch

MythTV noreply at mythtv.org
Tue Jan 10 05:32:39 UTC 2012


#10250: Big Mythgame patch
-------------------------------------------------+-------------------------
 Reporter:  Chris Lack <psycho_driver@…>         |          Owner:  robertm
     Type:  Patch - Feature                      |         Status:
 Priority:  minor                                |  infoneeded_new
Component:  Plugin - MythGame                    |      Milestone:  unknown
 Severity:  low                                  |        Version:  Trunk
 Keywords:  mythgame metadata sort giantbomb     |  Head
                                                 |     Resolution:
                                                 |  Ticket locked:  0
-------------------------------------------------+-------------------------
Changes (by robertm):

 * status:  new => infoneeded_new


Comment:

 A lot of good thoughts here, but a lot of things that need addressing,
 too.  This isn't a complete list, but it's what I see on a first read-
 through.

 1) As you mention, this patch is big.  Way, way too big.  I will need
 these broken out into functional parts to be able to apply.  Start with
 just the additions to the metadata, about which I have the following
 comments:

     - First, a general comment.  You should strive for consistency of
 operation between MythGame and MythVideo, Watch recordings, etc. in both
 terminology and functionality.  Some of the following comments go towards
 that.

     - ESRB is a North American entity, and thus doesn't apply to much of
 the world.  I would prefer a term like "Certification" which is what is
 used in MythVideo to indicate age ratings.  Should also work to not use
 ESRB specific ratings in the  as those are not used outside of the US,
 Canada, and Mexico.

     - Instead of "metarating" use "userrating" which is what this general
 concept is called everywhere else.  I would not bother to break up the
 metadata rating and the user rating as two distinct values. This isn't
 done anywhere else we get ratings from the metadata/listings provider/etc.
 Just fill an empty value with the metadata rating and let the user edit it
 as they prefer afterwards.  Drop "rating."

     - Developer and Players are fine.

 2) Next thing you might break down/issue to address:  Some of the metadata
 image handling changes here are outright wrong or really problematic-- You
 can't just stack up image selection windows, three per looked up item, on
 a metadata return.  The original behavior is correct-- in a regular
 metadata lookup, the code needs to pull in the first item in the list and
 download it.  If you want to give the user the opportunity to select from
 the list (and I agree that we should) then you need to implement specific
 buttons to do so like in MythVideo (Find online artwork, etc).  A regular
 metadata lookup shouldn't incur multiple stacked selection dialogs without
 any obvious text prompt-- it's inconsistent and unclear.  Also, you've
 added a hardcoded setting of the host for image download to the master
 backend.  This is incorrect.
 MythGame doesn't support games in storage groups and doesn't support
 imagery in storage groups either.  This is just going to download each
 image type to the storage group on the master backend, but does nothing to
 make them work when actually *loading* the images (and thus would only
 work if the SGs as defined on the MBE were actually also mounted on the
 various frontends).  This part either needs to go or needs a serious
 rethink.

 3) The Players UI widget should be a spinbox, not a textedit.  Since you
 only want to accept integer input, you can enforce it by using the right
 widget.  The rating UI widget should be a spinbox too, with a defined top
 and bottom, and sane increments in between rather than a buttonlist.
 Again here, you should follow the exampled set by MythVideo, MythMusic,
 etc.

 4) Once the above are addressed, we can add the theme changes so that the
 new metadata becomes visible/accessible

 5) At this point you can probably safely add the changes to the tree
 building/filtering.

 6) In the next patch, move on to the scanning behavior.  Not removing if
 the prompt is enabled is fine, but you need to actually prompt if you're
 going to obey the setting, not just do nothing.  You should add a prompt
 here if you want to see this behavior realized.

 7) You can add the changes to the base metadata parsing classes and
 grabber scripts as a separate patch too.  I have not looked at this one
 but I'll warn in advance that it's probably the one I'll take the most
 critical eye to, as these classes are used in a number of spots and I've
 worked to keep them clean.

 As a final, unnumbered item, I think there's probably a lot here that if I
 read more carefully I will have some concerns with, but those things
 become a lot easier to pick out when the patch isn't 90 KB.  Example: Why
 set all the artwork to NULL is file.size() == 1 ?  This is broken in at
 least a couple of cases I can think of.  Look for logical flaws and issues
 like this as you groom each piece for inclusion.

 If you would like to start breaking this apart, we can start reviewing the
 various pieces for inclusion one by one.

-- 
Ticket URL: <http://code.mythtv.org/trac/ticket/10250#comment:1>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center


More information about the mythtv-commits mailing list