Make screen option of icon boxes work again by bitstreamout · Pull Request #1230 · fvwmorg/fvwm3

bitstreamout · 2025-09-12T10:31:23Z

Make IconBox screen option working again even with several monitors.
Compare with issue #1229

What does this PR do?

It fixes issue #1229
Please read comment #1229 (comment)

It also allows the `@' symbol be the prefix character for screen names as the
manual page is not clear here.

In comparision to the attached patch in issue #1229
Ihere is also a change included which should set the loc_ok to True for a specfic monitor like
my test with @DVI-1 for my second monitor.

Issue number(s)

Fixes #1229

ThomasAdam

Hi @bitstreamout

Thanks for this. A few comments in-line for your consideration.

libs/FScreen.c

fvwm/icons.c

ThomasAdam

Hi @bitstreamout

Hmm. I don't like this approach at all.

First of all, fvwm's codebase is really old, and has all manner of interesting edge-cases. You're seeing one of them here, with how we want to try and make IconBoxes work.

Over the years, there's been a few attempts at making multiple monitors (screens) work better than they did when Xinerama was in use.

I introduced that, but what I haven't done is necessarily update any of the existing APIs, which is the case here.

With your renewed proposal though, you're introducing yet another convoluted change which makes no practical sense for me.

I think the confusion you're alluding to with @p, @g, etc., comes from the fact that geometry strings in fvwm3, can have that suffix after them to indicate the string, such as: 80x100+0+0@p -- which would explicitly apply that geometry to the primary monitor.

What it isn't though, is a specifier for the monitor name in general.

We already have a mechanism of looking up monitor names in libs/FScreen.c:monitor_resolve_name() and I see no reason why we couldn't use this, such that with your changes, we do something like:

struct monitor *m = monitor_resolve_name(icon_boxes_ptr->IconScreen);

ref.x = m->si->x;
ref.y = m->si->y;
ref.width = m->si->w;
ref.height = m->si->h;

...

That way, we're not special casing anything, and this works the same for other places which are looking up monitors to use.

I hope that makes sense. Let me know if you've any other questions.

ThomasAdam · 2025-09-16T09:05:37Z

Hi @bitstreamout

Still not sure why you're changing monitor_resolve_name(), it's not required.

ThomasAdam · 2025-09-16T10:44:39Z

@bitstreamout

So it seems like a lot of this code you're adding is in support of appeasing FScreenIsRectangleOnScreen() which is pointless, really.

If you look at where it's being used:

fvwm/conditional.c:                     is_on_page = !!FScreenIsRectangleOnScreen(
fvwm/icons.c:     if (FScreenIsRectangleOnScreen(fscr, FSCREEN_XYPOS, &ref))

... in, fvwm/conditional.c we're calling it with its first argument of NULL, which already makes it a glorified monitor lookup, anyway.

In, fvwm/icons.c we're already comparing a specific monitor which we know.

What I think we should do here, is not add all of this stuff to fvwm/icons.c just to support this function call to FScreenIsRectangleOnScreen and instead just compare the two sets of coordinates.

bitstreamout · 2025-09-16T11:16:37Z

What exactly do you mean here?

          /* this may be a good location */
          if (FScreenIsRectangleOnScreen(NULL, FSCREEN_XYPOS, &ref))
          {
            loc_ok = True;
          }
          else
          {
            loc_ok_wrong_screen2 = True;
          }

leads on DVI-0 with

Style   "xterm" IconBox screen DVI-1              -55  82     -5    -5, \
                IconGrid 5 5, IconSize 35 35 50 75

that loc_ok remains False and therefore the icon gets not placed on the right side of DVI-1
but on the instrinsic default location means left side on DVI-0 .... the same for FSCREEN_CURRENT
instead of FSCREEN_XYPOS ... how fix this regression?

bitstreamout · 2025-09-16T11:39:09Z

Also some lines after monitor_resolve_name(icon_boxes_ptr->IconScreen) should be added just in case someone misstype the monitor name

      if (m == NULL)
      {   
        m = monitor_get_current();
      }

Make IconBox screen option working again even with several monitors. Signed-off-by: Werner Fink <werner@suse.de>

ThomasAdam · 2025-09-16T15:12:14Z

Also some lines after monitor_resolve_name(icon_boxes_ptr->IconScreen) should be added just in case someone misstype the monitor name
      if (m == NULL)
      {   
        m = monitor_get_current();
      }

Depends on what the behaviour should be. What do you think? I thought we defaulted to either the global screen or primary screen, so maybe it should be one of those?

ThomasAdam · 2025-09-16T15:13:21Z

What exactly do you mean here?

Could you be more specific? I've lost the context of your question.

fvwm/icons.c

ThomasAdam · 2025-09-16T15:17:32Z

fvwm/icons.c

-	  if (FScreenIsRectangleOnScreen(fscr, FSCREEN_XYPOS, &ref))
+	  if (IsRectangleOnThisPage(m, &ref, t->Desk))


Why have you changed from FScreenIsRectangleOnScreen() to IsRectangleOnThisPage()?

As otherwise the defined IconBox of the user does not match but that of the global_icon_box_ptr

while(do_all_iconboxes(t, &icon_boxes_ptr)) { struct monitor *m; fprintf(stderr, "FFFF: %s %d\n", icon_boxes_ptr->IconScreen, loc_ok); if (loc_ok == True)

leads to

loaded [0]: /suse/werner/.fvwm/config FFFF: DVI-1 0 FFFF: (null) 0

on stderr output of fvwm3 (here with a not set global_icon_box_ptr->IconScreen)

So?

My point is that this is an entirely different function call you're changing. Why?

What is your proposal to make the users IconBox win here. Using NULL for fscreen_scr_arg *arg in FScreenIsRectangleOnScreen() does not help

What is your proposal to make the users IconBox win here. Using NULL for fscreen_scr_arg *arg in FScreenIsRectangleOnScreen() does not help

What I'm asking, is why you changed from FScreenIsRectangleOnScreen() to IsRectangleOnThisPage() -- it's a completely different function.

compare with my current test in #1230 (comment)

We're going round in circles, @bitstreamout

What I'm saying about FScreenIsRectangleOnScreen() is it's only used in two places.

Rather than write all this additional crap in icons.c, we should change FScreenIsRectangleOnScreen()'s implementation. We already have both "rectangles" to compare, so we should stop it from looking up monitors, etc., and just pass the relevant information in.

As a consequence ef doing this, we will need to update conditionals.c as well.

Would you prefer if I did this?

Would you prefer if I did this?

Indeed before I do the next crap ;)

Maybe it is enough to use FSCREEN_GLOBAL with FScreenIsRectangleOnScreen() as DVI-1 has an y offset of 1920 and the global screen has twice the size aka 3840

fvwm/icons.c

ThomasAdam · 2025-09-16T15:18:25Z

fvwm/icons.c

 		global_icon_box_ptr->IconGrid[1] = 80;
 		global_icon_box_ptr->IconFlags = ICONFILLHRZ;
+		global_icon_box_ptr->IconScreen = "p";
+		global_icon_box_ptr->do_free_screen = 0;


Why add do_free_screen here?

Simply to be sure that the static string is not freed ... even it free_icon_boxes() will currently not reach this.

Is an unnecessary addition.

Is an unnecessary addition.

How to resolve the NULL for the IconScreen of the last IconBox seen from do_all_iconboxes() in the loop?

ThomasAdam requested changes Sep 13, 2025

View reviewed changes

libs/FScreen.c Outdated Show resolved Hide resolved

fvwm/icons.c Outdated Show resolved Hide resolved

ThomasAdam self-assigned this Sep 13, 2025

ThomasAdam added the type:enhancement Augmenting an existing feature label Sep 13, 2025

ThomasAdam added this to FVWM3 Sep 13, 2025

github-project-automation bot moved this to PRs in FVWM3 Sep 13, 2025

bitstreamout requested a review from ThomasAdam September 15, 2025 07:15

bitstreamout force-pushed the main branch from fd0ec70 to 7fbba48 Compare September 15, 2025 09:46

ThomasAdam requested changes Sep 15, 2025

View reviewed changes

ThomasAdam added this to the 1.1.4 milestone Sep 15, 2025

bitstreamout force-pushed the main branch from 7fbba48 to 125fcf0 Compare September 16, 2025 08:46

bitstreamout force-pushed the main branch from 125fcf0 to eb8cc43 Compare September 16, 2025 09:26

bitstreamout requested a review from ThomasAdam September 16, 2025 09:59

Make screen option of icon boxes work again

fefe9af

Make IconBox screen option working again even with several monitors. Signed-off-by: Werner Fink <werner@suse.de>

bitstreamout force-pushed the main branch from eb8cc43 to fefe9af Compare September 16, 2025 11:51

ThomasAdam requested changes Sep 16, 2025

View reviewed changes

ThomasAdam removed this from the 1.1.4 milestone Nov 8, 2025

ThomasAdam marked this pull request as draft November 8, 2025 17:35

		if (FScreenIsRectangleOnScreen(fscr, FSCREEN_XYPOS, &ref))
		if (IsRectangleOnThisPage(m, &ref, t->Desk))

Uh oh!

Conversation

bitstreamout commented Sep 12, 2025

Uh oh!

ThomasAdam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ThomasAdam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomasAdam commented Sep 16, 2025

Uh oh!

ThomasAdam commented Sep 16, 2025

Uh oh!

bitstreamout commented Sep 16, 2025

Uh oh!

bitstreamout commented Sep 16, 2025

Uh oh!

ThomasAdam commented Sep 16, 2025

Uh oh!

ThomasAdam commented Sep 16, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitstreamout Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomasAdam Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThomasAdam Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ThomasAdam left a comment •

edited

Loading

ThomasAdam left a comment •

edited

Loading

bitstreamout Sep 17, 2025 •

edited

Loading

ThomasAdam Sep 17, 2025 •

edited

Loading

ThomasAdam Sep 16, 2025 •

edited

Loading