[mythtv] CI Progress

[Kenneth Aafløy]

Ben Bucksch wrote:
>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?
Can't understand why you 'mix and match' class and C functions (except for
functions that needs the speed or are in a C library imported for
functionality and the need to not rewrite everything!)?

>     * 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;
Originally wanted to move it into DVBChannel, as it belongs there, basic
classwriting.

I was trying to move pid setting out of DVBChannel, it has no reference to
tuning at all. Besides, whats the point of using a vector to store it when
it actually will become a C array?

>     * Properties are lower case, without m_ (it makes no sense to only
>       have half the properties start with m_), methods are upppercase.
Agree!

>     * What is that "version"? Is that a replacement for "bool
>       pid_changed;". Why is the latter insufficient?
So where do I unset it, what if even one more function needs it?
Ease of maintance!

>     * Why did you change the way the PID processing works?
Out of DVBChannel, planning on writing a new demux function, because I'll
need it if I'm going to write a Table parser (EPG/PMT...).

> 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.
Then comment before I get close to releasing, gave a chance last time and
gave a chance this time, and not with one-liners explaining notting!

> >    - cleaned up dvb*
> >
> That "clean up" is what I disagree with.
>
> >    - builds without USING_DVB
> >
> No, you just moved it to tv_rec.cpp.
Makes more sence to #ifdef small parts than all over DVB*, besides did you
miss the libmythtv.pro changes?

Note: The comment blocks ******** are there to separate things that will
eventually be moved!

Kenneth