[mythtv] [PATCH] Display Resolution switching

[Isaac Richards]

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.

> >- 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.

> >- 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.

> >- 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.

> >  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.

> >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.

> >- 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.

Isaac