[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