Improve monitor event handling

[farblos]

What does this PR do?

After the monitor event handling changes earlier this year I noticed that I do not get any longer monitor_changed events. I started checking the relevant code and found some issues, then some more, and then I went down the rabbit hole ...

Know I have Fvwm in a state where I get montor_changed, _disabled, _enabled events exactly mirroring the xrandr changes.

Here is a rough overview of what I did:

• I removed old code related to monitorsold_q, since I decided that it is a left-over of an earlier event implementation
• I fixed some conditions on m->emit in monitor_emit_broadcast, plus I removed a break statement that would (IMO) exit the loop even if there are still events that need reporting (also some left-over?)
• I added some check in dispatch_event to avoid double handling of identical events (even though scan_monitors should now handle these well)
• I extended the event handling in scan_monitors to more accurately detect changes in monitor configuration also in all sorts of edge cases

Known Open Issues and Questions

• The handling of events on primary monitor changes is still not quite correct. It probably should be also redone in terms of some flags on m->emit. Not sure how relevant that is, though.
• Any idea why I get duplicate RandR events with identical serial number? Anybody else seing this?

Edits

• Removed the draft header.
• Updated overview and known issues.
• Removed the link to that other issue.

[ThomasAdam]

Thanks, @farblos

There's a few changes here I don't think we need, plus many changes we can consolidate.

We don't need any of the documentation changes you've made.

I'll reply more fully in the coming days. The needs a lot more refinement yet.

[farblos]

I'll reply more fully in the coming days. The needs a lot more refinement yet.

Sure, thanks. If you think it'll help we can discuss this also via IRC. Only we need to arrange some kind of a meeting then, since I'm usually not available on IRC.

[ThomasAdam]

Thanks, @farblos -- I'm always in #fvwm on IRC, so if you're not always around, catch me when you can... :)

[ThomasAdam]

Hi @farblos

Thanks for this.

Overall it looks good to me. I've left specific comments in this review.

Key things here to note are:

1. I'd like you to remove the comments you've added before each function in libs/FScreen.c -- presumably it's there to help you, but doesn't belong in this commit.
2. We don't need MONITORS.md so that can go.
3. We need to think more carefully about FvwmEvent, and what we want to pass to it. That said, I don't want us to introduce a PassArgs component to FvwmEvent as you're suggesting. I suspect it will be enough to just send m->si->name to either RandRFunc, for the specific event in FvwmEvent.

If you could make the required changes, this should help reduce some of the extraneous noise in this change.

Any questions, please let me know.

[farblos]

Thanks for your review! That's much more detailed than I expected - and it will take some time for me to address them, here and on IRC...

[farblos]

Since we settled on RandRFunc in IRC, I'll revert all changes on FvwmEvent.c and its man page.

Regardless of that I might (or not) take another stab at the man page in a separate PR. I think it could state more clearly and at the beginning how exactly the command is built. For example, I took me reading the sources to understand that I can leave *FvwmEvent: Cmd blank ...

[farblos]

Hi @ThomasAdam,

I think I'm through with the changes from your review, removing most of the noise. Plus I reverted all changes related to FvwmEvent, keeping basically only:

• removal of obsolete code related to monitorsold_q
• consolidation of event handling and emitting code

I removed the "draft" flag from the PR, but I'm not sure how to close the open review status ...

Anyway, we can continue from here as you prefer:

• I could re-request review
• I could squash and rebase the commits
• You could add your patches on top as you have been planning
• You could even merge the PR as is, as this a) seems to be a self-contained unit of work and b) comprises already what is most important to me

Just pls let me know how to continue, and preferredly in this PR, since I won't hang around in IRC the next days.

Thanks!

[ThomasAdam]

@farblos

Thanks. Please have a look at commit 652eb34 on branch ta/review-gh-1069.

I've squashed all of your intermediary commits into just one commit for these changes (even though these changes cross fvwm <-> module boundaries, it's all related). I've left the authorship intact, but changed the commit message.

You'll note in I've left a commented block with FIXME in it -- this is in monitor_update_ewmh(). Please take a look.

In order to see what I've changed, relative to this PR, you can do:

git diff improve-monitor-events..ta/review-gh-1069

Which assumes your branch for this PR is called improve-monitor-events.

What I suggest you do is cherry-pick 652eb34 onto your branch, make any changes to that commit, and just force-push it to this PR.

Hope that helps. Any questions let me know.

[farblos]

Fair enough, thanks.

Regarding your FIXME in monitor_update_ewmh. The code in main at that point does the following:

TAILQ_FOREACH(mo, &monitorsold_q, oentry) {
  if (strcmp(m->si->name, mo->si->name) != 0)
    continue;
  if (mo->Desktops == NULL)
    continue;
  for (t = Scr.FvwmRoot.next; t; t = t->next) {
    if (t->m == mo) {
      t->m = m;
      update_fvwm_monitor(t);
    }
  }
  m->emit |= MONITOR_CHANGED;
}

Since you have let me remove the monitorsold_q-related code, I guess it really must have been obsolete, and we can ignore most of it here. The only thing that still could be relevant in the above snippet, however, should be the call to update_fvwm_monitor(t), right?

But that is done identically also at the very bottom of function monitor_update_ewmh:

for (t = Scr.FvwmRoot.next; t; t = t->next) {
  update_fvwm_monitor(t);
}

Wouldn't that suffice?

[ThomasAdam]

Hi @farblos

Yes. This should suffice.

[farblos]

Hi @ThomasAdam,

this is it. Hopefully. My changes + your cleanup + your commit message - fixme, squashed and rebased onto main. Should be identical to ta/review-gh-1069 (after rebasing that as well) except for the fixme.

So if you also think that this is it, feel free to merge.

Thanks a lot!

P.S. I have tried to reach you on IRC wrt. some follow-up issues, will try to pester you later with these...