[mythtv] CI Progress

[Ben Bucksch]

Kenneth Aafløy wrote:

>Ben Bucksch wrote:
>
>  
>
>>Kenneth Aafløy wrote:
>>
>>    
>>
>>>The code is ugly, but I've included a diff against dvb* files in
>>>libmythtv, so you have the opportunity to comment.
>>>      
>>>
>>Is this patch obsolete now? I hope so...
>>    
>>
>Why, was it that bad?
>
Yes. You touched almost all lines in the dvbchannel.cpp, I couldn't see 
why, I didn't like the new style, and I think you broke some features, 
but I am not sure, because the patch was unreadable (too many changes).

>Anyway, here's what I was hoping would get into CVS.
>
Some things:

    * If you make heavy whitespace changes, please *at least* include a
      diff -wu or better yet submit them with a separate patch. Same
      with code style changes.
      Example:
      -  : ChannelBase(parent),
      -    use_ts(a_use_ts),
      -    cardnum(aCardnum)
      +    : ChannelBase(parent), cardnum(aCardnum), use_ts(a_use_ts)
      I think the new code is harder to read than the old code. Don't
      make such changes.
    * You made FetchDVBTuningOptions() a method of DVBChannel. This
      requires to move the definition of DVBSettings into the header
      file. I intentionally left DVBSettings in the cpp file, because
      it's local to the implementation, may change often and I didn't
      want it to be public and cause rebuilds. You don't need the
      function outside of dvbchannel.cpp, and it doesn't need to access
      protected DVBChannel vars, so why that change?
    * Please try to avoid pointers and raw C arrays, use local vars,
      references or vectors instead. For example:
          o header: +    DVBSettings*    m_settings;
            consturctor: +    m_settings =
            (DVBSettings*)malloc(sizeof(DVBSettings));
            then tons of
            -            else if (option == "16") s.guard_interval =
            GUARD_INTERVAL_1_16;
            +            else if (option == "16")   s->guard_interval =
            GUARD_INTERVAL_1_16;
            and the like. Why don't you just use a DVBSettings
            m_settings; property? Or better yet, leave things as-is,
            keeping DVBSettings as local variable and separating out the
            PIDs, which you need elsewhere? DVBSettings was only
            intended as a convient handle to pass the tuning parameters
            between the DB-reading/parsing function and those which do
            the tuning, it wasn't intended to keep every setting related
            to tuning.
          o +    uint16_t pids[MAX_PIDS];
            -    vector_int pid;
    * Properties are lower case, without m_ (it makes no sense to only
      have half the properties start with m_), methods are upppercase.
    * What is that "version"? Is that a replacement for "bool
      pid_changed;". Why is the latter insufficient?
    * Why did you change the way the PID processing works?

In general, don't rewrite the file unless you have a really good reason 
to. Please justify every change. I can't make sense of your changes, and 
some of what you changed goes against explicit decisions I made, without 
any appearant reason.

>    - cleaned up dvb*
>
That "clean up" is what I disagree with.

>    - builds without USING_DVB
>
No, you just moved it to tv_rec.cpp.