Re: RFC: ctwm enhancement for multiple monitors

On Tue, Apr 11, 2023 at 11:14:48PM +0200, Maxime Soulé wrote:
[...]
> If Xrandr is not available, or cannot be used because its version is too
> low, or any other reason XrandrNewLayout
> (https://github.com/fullermd/ctwm-mirror/blob/master/xrandr.c#L22) returns
> NULL, the layout is initialized with root coordinates:
> https://github.com/fullermd/ctwm-mirror/blob/master/ctwm_main.c#L455-L457
 
Ah, this gave me a great way to check my code for no xrandr.  Aside
from the --cfgchk trick you mention below, I just toss a "return NULL;"
as the first line of XrandrNewLayout() for "live" testing.

> > As far as I can tell, it looks like when there's no xrandr compiled
> > in, appropriate monitor values are just substituted, so things
> > should just work as-is, right?
> 
> I added a comment to your commit to handle this specific case, for which
> there is no monitor names at all.
 
Thank you.

> > On redundant checks in my patch, after reading more code, it looks
> > like I could remove the "if (RLayoutNumMonitors(Scr->Layout) > 0) {
> > ... }" check.
> > https://github.com/qbarnes/ctwm-mirror/blob/e462a9e8ad1677fce6f01176e006f07e9b37278b/parse_m4.c#L271
> > 
> > Prior to main() calling LoadTwmrc(), which in turn runs my patch,
> > the previous code in main() has already checked to ensure that
> > RLayoutNumMonitors(Scr->Layout) will return a value greater than 0,
> > correct?
> 
> Yes.
> 
> Just as a note, ctwm can be launched with --cfgchk and no X available, to
> check its configuration and leave:
> 
> DISPLAY= ctwm --cfgchk .ctwmrc
 
Good trick.  I'll use that.

> for example. In this case, we fall into the case where Xrandr is not used,
> so the layout defaults to root coordinates, so always one monitor.
 
This brought up a question to myself of how to handle the *MONITOR*
variables when no xrandr.  The problem with just not generating
them and leaving them undefined would force people to do a lot of
ifdef'ing in their .ctwmrc.  Not very user friendly.

Since you're already setting Scr->Layout to some sane values, I
should just use those for generating default values for the macros.
So in my triple monitor use case with no xrandr, the macros look
like this:
===========
define(`XRR_MONITOR_COUNT', `1')
define(`XRR_MONITOR_0', `')
define(`XRR_MONITOR_0_WIDTH', `7360')
define(`XRR_MONITOR_0_HEIGHT', `2160')
define(`XRR_MONITOR_0_X', `0')
define(`XRR_MONITOR_0_Y', `0')
===========

> > I'm still not sure how "Scr->Layout" all works with multiple screens
> > and when it can change, so I had left it in there the patch when I
> > first coded it.
> 
> Once set at startup using Xrandr info, Scr->Layout can only change using the
> MonitorLayout variable during the parsing of the configuration, never after.
> Your code should be called before any MonitoLayout use, so it is OK. As ctwm
> boots very quickly, it has been decided to restart it when monitors layout
> changes instead of handling the change without restarting it. It is a way
> easier to handle.
 
Good to know.

> > I think "XRR_" is a good prefix.  With not abbreviating "MONITOR",
> > the macro names are already long enough!
> 
> I agree with XRR_ prefix, but I have no opinion about removing or not the
> following MONITOR_ word.
 
It was not about removing, but abbreviating.  Instead of 
"XRR_MONITOR_..." I was considering "XRR_MON_...".  I like
"XRR_MONITOR_..." better.

Ok, with all the additional review comments and testing, I have an
updated commit.  Note that I did some rebasing, so for a local repo
copy, make sure you do a "git pull -r" when updating.  Branch is
"monitor_vars".  Latest changes in:
https://github.com/qbarnes/ctwm-mirror/commit/89e3a6c2c6a7a1823626ab3efeb30527a83f3219

I also fixed a crash in RLayoutGetNameIndex() when no xrandr with
names being NULL.  The fixed version is here:
https://github.com/qbarnes/ctwm-mirror/commit/64493d8fccf8b4b62acec2500b5691ba19f36fb4

Let me know if you have any additional comments and what's next.

> ++
> 
> Max.