Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial implementation of _NET_FRAME_EXTENTS #303

Merged
merged 1 commit into from Dec 21, 2020
Merged

Conversation

raboof
Copy link
Owner

@raboof raboof commented Dec 19, 2020

Should fix #59

Co-Authored-By: Florian Larysch fl@n621.de

Copy link
Contributor

@florolf florolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for proposing this. I was reluctant to do this because it might cause other regressions while simultaneously fixing #59: Other toolkits also have (possibly differently-buggy-than-GTK) fallback paths that trigger when this property is absent (as it was previously), see for example QT.

Furthermore, who knows what implicit assumptions toolkits/programs make based on the actual value of the property. EWMH doesn't really define what it is actually supposed to mean or what clients are allowed to deduce from it. It only really makes sense when making further assumptions about the window hierarchy one ends up with after reparenting and neither EWMH nor ICCCM make any strong claims about this as far as I can see.

All of this leads to the rather philosophical question what extents actually are in a window manager that is less WIMPy than assumed by those specs, such as notion. The closest thing would probably be the offset added by frame_managed_geom, but I don't know how to properly propagate this into a client window in all situations. The proper place to set the property would probably be clientwin_fitrep, but this can be called with a multitude of different parameters and even multiple times for a single attach; I'm not sure how to handle this correctly.

So I guess the easy way out is to claim that since the specs are unclear, a fixed (0, 0, 0, 0) is as valid a setting for this property as any. I've been using this setting for a week now and have not experienced any glitches in the applications I usually use, so it might not even be a problem in practice.

Given that the presence of this property mainly seems to serve to switch between different assumptions about the shape of the window hierarchy in different toolkits, maybe adding a winprop kludge to enable/disable setting this property is probably more useful than agonizing about the actual value if problems ever crop up because of this.

ioncore/netwm.c Outdated
Comment on lines 109 to 117
void netwm_set_frame_extents(WClientWin* cwin, long* extents) {
XChangeProperty(ioncore_g.dpy, cwin->win, atom_net_frame_extents,
XA_CARDINAL, 32, PropModeReplace, (uchar*)extents, 4);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a very safe API: The caller needs to know that they have to pass 4 values in extents. I'd rather make this explicit, which also allows hiding the gotcha that a protocol-level CARD32 is represented a long in the xlib ABI, no matter the actual sizeof(long), while the actual CARD32 type varies in size:

Suggested change
void netwm_set_frame_extents(WClientWin* cwin, long* extents) {
XChangeProperty(ioncore_g.dpy, cwin->win, atom_net_frame_extents,
XA_CARDINAL, 32, PropModeReplace, (uchar*)extents, 4);
}
void netwm_set_frame_extents(WClientWin *cwin, CARD32 left, CARD32 right, CARD32 top, CARD32 bottom)
{
unsigned long data[4];
data[0] = left;
data[1] = right;
data[2] = top;
data[3] = bottom;
XChangeProperty(ioncore_g.dpy, cwin->win, atom_net_frame_extents,
XA_CARDINAL, 32, PropModeReplace, (uchar*)data, 4);
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be better to pass in the 4 values individually (or in a dedicated data structure). I think I'd prefer to pass them in as regular ints and do the conversion to long/CARD32 inside the function, though. Amended, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, I guess that's more consistent with the rest of the codebase anyway.

@raboof raboof closed this Dec 20, 2020
@raboof raboof reopened this Dec 20, 2020
Should fix #59

Co-Authored-By: Florian Larysch <fl@n621.de>
@raboof raboof merged commit 867650e into master Dec 21, 2020
raboof added a commit to raboof/nixpkgs that referenced this pull request Dec 21, 2020
What's New

The highlight of this release is fixing the long-standing issue
where Firefox popups would sometimes show up in the wrong place
on multi-monitor setups (https://github.com/raboof/notion/issue/59),
thanks to great detective work by @florolf . Thanks!

* Initial implementation of _NET_FRAME_EXTENTS (raboof/notion#303) @florolf/@raboof
* Fix potential livelock in do_timer_set (raboof/notion#302) @dnr

Docs

* Add more docs to cfg_notion.lua (raboof/notion#283) @raboof

Under the hood

* Trigger release drafter from github actions (raboof/notion#304) @raboof
* Remove some colorful language (raboof/notion#296) @raboof

https://github.com/raboof/notion/releases/tag/4.0.2
@raboof raboof mentioned this pull request Dec 21, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firefox subwindows pop up on the wrong monitor
2 participants