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

[Michael T. Dean]

On 02/07/2012 08:12 AM, David Blain wrote:
>>        Author:  Robert McNamara<rmcnamara at mythtv.org>   Change Date:
>> 2012-02-06T21:35:43-08:00
>>     Push Date:  2012/02/06 21:39:00 -0800
>>    Repository:  mythtv
>>        Branch:  master
>> New Revision:  45a3bfcd4cde5332222056f99b740c163cfe92e2
>>     Changeset:  https://github.com/MythTV/mythtv/commit/45a3bfcd4
>>
>> 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