[mythtv] [PATCH] Display Resolution switching

[John Patrick Poet]

Isaac Richards wrote:

>On Saturday 31 July 2004 04:28 am, John Patrick Poet wrote:
>  
>
>>>- It won't compile at all without xrandr
>>>      
>>>
>>If xrandr is not available on the machine, it will not compile?
>>    
>>
>
>You're using all the xrandr functions regardless if USING_XRANDR is defined.  
>No point in having the define if you ignore it like that.
>  
>

Yes, I see that.

>>>- It won't compile at all if using_x11 isn't defined
>>>      
>>>
>>Damn.  I did test that, but I must have modified libmyth.pro sometime
>>afterwards.
>>    
>>
>
>It's not even that -- MythContext unconditionally included it, but libmyth.pro 
>conditionally compiled it.
>  
>

Yes, I see that too.  That is what I get for trying to "clean up the 
code", *after* testing it.

>>>- Why is everything in DisplayRes static?  I don't see a reason for that.
>>>      
>>>
>>The way I wrote it initially, it had to be static.  Another way to
>>handle it, would have been to make it a singleton class, but I don't
>>like making the "main" program do extra work to instantiate/use a
>>class.  With it as a member of MythContext it actually does not have to
>>be done either way...
>>    
>>
>
>Extra work?  You'd be replacing the 'gContext->displayRes' with 
>'GetDisplayRes()'.  No extra work.
>  
>

I guess it is just a matter of preference.

>>>- Why is the DisplayRes class a public member of the context?
>>>      
>>>
>> I made it a member of MythContext because that is the primary place it
>>is used, and other "classes" would have easy access to it.
>>    
>>
>
>It's not a question of being a member, it's a question of it being a public 
>member.  It _should_ be private.
>  
>

Why?

>>> I think it'd
>>>live best as a singleton class (see some recent mediamonitor changes),
>>>      
>>>
>>Okay, I will make it a singleton class.
>>
>>    
>>
>>>but at
>>>the least it should be a private member with an accessor function.
>>>      
>>>
>>That data that should be private, is private within the DisplayRes
>>class, so I did not see a reason to use an accessor function.
>>    
>>
>
>I'm not talking about the data of the DisplayRes class, I'm talking about 
>having a member variable be visible in the public MythContext for absolutely 
>no reason.
>  
>

Why?  How does a layer of abstraction help?

>>>Other suggestions:
>>>- I'd make the display res class grab its own settings, and not feed em in
>>>through Activate() & addRes().
>>>      
>>>
>>I did that so it would not have to know anything about gContext -- I was
>>trying to cut down on the dependencies.  I will change it.
>>    
>>
>
>The context is global.  Not knowing anything about it is rather pointless.
>
Okay.


>>>- I would _really_ prefer it if your patch followed the rest of the style
>>>conventions ('if' is not a function, 4 space indents, etc).  Otherwise, I
>>>have to spend time fixing things as I apply it.
>>>      
>>>
>>I actually did try.  While working on this, I even went into my .emacs
>>file and changed the default to 4 space indents.  I will re-audit the
>>code and try to find the places I missed.
>>    
>>
>
>'if' without a space after it is horribly annoying.  'if' is not a function.  
>The displayres class is full of tabs as well.
>  
>

Another personal preference.  It does not bug me the way it bugs you.  
However, this is your baby, and I will do my best to do it your way.

Believe it or not, 4 space indents actually bugs me.  I like to have 
option of nesting deeply when necessary.  I personally use 2 space 
indents so the code does not become a mess of line breaks.

When I went in to do a search/replace of "if(" to "if (" I found a lot 
of instances in code that I had not written, so it has slipped by 
before.  Sorry I forgot to "untabify" the DisplayRes code.

John