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.
