[mythtv] [mythtv-commits] mythtv/master commit: 45a3bfcd4 by Robert McNamara (rmcnamara)

[David Blain]

> >> Log:
> >>
> >> Convert to string output for rec/searchtype, dupmethod/in in Services.
> >>
> >> Removes use of internal enums and uses translated strings instead.
> >>
> >> Apologies to anyone using the recrule services for the changes, but
> >> it was probably just me given the bug I just fixed preventing them
> >> from working right.
> > Is there a reason you couldn't expose the enum from the service.  I
> > kind of wanted to keep all datacontracts how we'd want to use them
> internally.
> >
> > If the enum is used instead of string or int as the return type, with
> > a little more use of Qt macros, it should serialize the enum correctly
> > (see recording.h).
> 
> FWIW, I think external, 3rd-party clients should not be interpreting data
> based on raw, internally-defined values.  We have far too many 3rd party
> clients that get broken spectacularly when we change/add/remove values
> for such things (even clients using bindings have been affected by past
> changes).  And, since our 3rd-party clients have a tendency to not
actually
> care about version changes (and fake protocol/version compatibility), this
> can result in not only those clients failing to work, but actually doing
things
> to break MythTV (at minimum sending wrong values so that, for example,
> recording rules are set up incorrectly or, worse, actually corrupting
data,
> which may cause more extreme failures of mythbackend/mythfrontend).
> 
> My preference for external API use is to present the data such that
clients
> don't need to know that (currently) 0 means HDTV, 1 means WIDESCREEN,
> and 2 means AVC for program's videoprop, and for recordedprogram's
> videoprop, there are additional values, 3 = 720 and 4 = 1080 and (as of
only
> very recently) 5 = DAMAGED.  It would also be nice if clients didn't need
to
> know that, for dupin values, 1=kDupsInRecorded, 2=kDupsInOldRecorded,
> 15=kDupsInAll, and 16=kDupsNewEpi (and other values are invalid--though
> in 0.24, we also had meanings for 32, 64, and 128).  Since we're
constantly
> changing our internal values, I don't want clients trying to interpret
those
> values.
> 
> I understand the desire to use internal values so the services API (or at
least
> datacontracts) can be used for internal code, too, but we have to do
> something so that clients don't have to know these constantly-changing
> MythTV internals.  I think even if we use "external"
> values that are specific to the API--and map them to internal
values--we'll
> have the same issue with changes over time.  This approach could minimize
> the damage changes do over time, but would result in cluttering the
> external API values.
> 
> My current thinking is that the best approach would be making it so that
> clients aren't interpreting data at all--but rather displaying it and
allowing
> user interaction with it.  At minimum, we need to provide a translation
> between "raw values" and their meanings.  I've created a LabelValue
> datacontract that I'm using with the logging code (which I will be pushing
> soon) to provide label/value pairs for the UI as well as "selected" and
> "active" flags (i.e. for use in drop down lists or similar).  We could use
this
> approach to allow providing internal values and a user-friendly label for
> those values.  In the context of setting up recording rules, it even makes
> sense to add a description that allows providing an explanation that
clients
> could display for users.  And this approach may actually make translation
> easier--as it would allow clients/users to see the user-friendly label,
while
> providing back the currently-mapped-to-that-label internal value.
> 
> If others agree with this approach--and if no one else has a better
idea--I
> can convert the scheduling code to use that approach for you, Robert.  I'm
> pretty sure this change came out of comments I made in IRC about not using
> internal values in the external API (and mentioned using the "external"
> values approach since I hadn't come up with a better approach at the
time--
> which is what you've done here), so since you've already changed it once
for
> me, it makes sense for me to change it this second time.
> 
> Thoughts/suggestions, David (or anyone else)?
> 
> Mike

My original comment was due to my belief that the serialization libraries
would take the enums and render its name using introspection.  I agree that
having a 0 is meaningless.

As for your other thoughts, I'll have to digest it a little before giving
feedback.

David.