Use window's color map when setting border. #9

Closed
wants to merge 11 commits into
from

Projects

None yet

8 participants

@asjo
asjo commented Nov 22, 2015

If the window's own color map is available, it is used, otherwise
fall back to the default color map.

This solves the problem described in https://code.google.com/p/xmonad/issues/detail?id=581
by following the tip given in chjj/compton#240 (comment)

I would love to have the changes in this commit improved to be "better Haskell".

Depends on pull request xmonad/X11#35 - which gives rise to pull request xmonad/xmonad-contrib#13

@asjo asjo Use window's color map when setting border.
If the window's own color map is available, it is used, otherwise
fall back to the default color map.

This solves the problem described in https://code.google.com/p/xmonad/issues/detail?id=581
by following the tip given in chjj/compton#240 (comment)
e7d0e00
@dmwit
Contributor
dmwit commented Nov 24, 2015

I'm not sure, but looking at the documentation, it seems you may also want to install the fallback color if the XWindowAttributes' structure has map_installed set to False. This probably involves a similar round of changes to the X11 Haskell package to the ones needed to make the colormap field visible.

@asjo
asjo commented Nov 24, 2015

I have made the changes to X11 and xmonad-contrib, but I don't seem to be able to figure out how to throw an exception if wa_map_installed is False.

asjo added some commits Dec 8, 2015
@asjo asjo Merge remote-tracking branch 'upstream/master' faec492
@asjo asjo Catch getWindowAttributes exception in handle.
When getWindowAttributes in X11 throws an exception when
XGetWindowAttributes returns 0, handle needs to catch it.
8c2b604
@ptrxyz
ptrxyz commented Jan 31, 2016

Any updates on this? Seems the tryGetWindowAttributes is added to xmonad/X11. Is there still something missing to pull in asjo's changes?

@asjo
asjo commented Jan 31, 2016

There is still the issue of throwing an exception when wa_map_installed is False (as I mentioned above I couldn't figure out how to do), and then there are the other places in XMonad where getWindowAttributes is called, which need to handle exceptions.

@bb010g
bb010g commented Apr 4, 2016

bump

@f1u77y
f1u77y commented Nov 8, 2016 edited

Will this be merged before 0.13? I'm using this for some time and it seems to be stable.

About c00b806: isn't it better to run code from mouseBindings in userCode(like we do in keys actions)? That will also fix problems with custom mouse actions(if anyone uses it) or actions from contib(like X.A.FlexibleManipulate)

@asjo
asjo commented Nov 8, 2016

@f1u77y My comment from Jan 31 is still valid. There are a number of places I couldn't figure out how to handle exceptions from getWindowAttributes in, which can make xmonad crash.

@pjones
Member
pjones commented Nov 22, 2016

I'm willing to take this on and get it ready for the 0.13 release.

Just to make sure I'm clear on this, this patch relies on xmonad/X11#35 which was merged but is not yet part of a release. Is that correct?

@asjo
asjo commented Nov 22, 2016

@pjones Correct. Thanks for looking into this - I hope to learn from how you solve the remaining calls!

@pjones pjones added a commit that referenced this pull request Nov 23, 2016
@pjones pjones Refactor xmonad/xmonad#9 and remove explicit exception handling 9bb2a54
@pjones pjones added a commit to xmonad/xmonad-contrib that referenced this pull request Nov 23, 2016
@pjones pjones Update X11 version for xmonad/xmonad#9 1833003
@pjones pjones added a commit to xmonad/xmonad-testing that referenced this pull request Nov 23, 2016
@pjones pjones Testing setup for xmonad/xmonad#9 3edf308
@pjones
Member
pjones commented Nov 23, 2016

@asjo I refactored your PR and temporarily removed exception handling. I've been testing my local xmonad with this change in place and I've not run into any exceptions.

I also added another commit (da95d4f) that only installs the colormap if wa_map_installed is True. Strangely enough, the program I was testing with (termite) has wa_map_installed set to False so the border ends up being transparent. From that I believe that we should probably ignore the wa_map_installed field.

All of my testing is going on over here if you want to clone the repo and test for yourself.

Can you recall what situations led to exceptions being throw in xmonad?

@geekosaur
Contributor

On Tue, Nov 22, 2016 at 9:13 PM, Peter J. Jones notifications@github.com
wrote:

I also added another commit (da95d4f
da95d4f)
that only installs the colormap if wa_map_installed is True. Strangely
enough, the program I was testing with (termite) has wa_map_installed set
to False so the border ends up being transparent. From that I believe
that we should probably ignore the wa_map_installed field

That field is only meaningful with PseudoColor visuals. I'm not sure xorg
even supports those any more. (That was the 256 color mode that was used in
the 90s, and wa_map_installed indicated the window had its own private
colormap... leading to the infamous "technicolor" effect when the window
didn't have the colormap focus (which usually followed window focus).

We probably should support it in PseudoColor visuals, but on most modern
Linuxes wa_map_installed will always be False because it's using
TrueColor or DirectColor visuals. And we need a bit more than just that
to properly support PseudoColor visuals IIRC.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@geekosaur
Contributor

On Tue, Nov 22, 2016 at 9:45 PM, Brandon Allbery allbery.b@gmail.com
wrote:

That field is only meaningful with PseudoColor visuals

Well, technically that statement is false. Practically, I don't think you
will find any program bothering with a private colormap in a TrueColor or
DirectColor visual, except possibly some very old engineering CAD
programs. (And AutoCAD hasn't targeted Unix in years....)

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@pjones
Member
pjones commented Nov 23, 2016

Awesome info @geekosaur, thanks a lot!

@asjo That means you can ignore my commit that uses wa_map_installed.

The last open question is then: Under what circumstances would getWindowAttributes throw an exception?

Thanks!

@geekosaur
Contributor

On Tue, Nov 22, 2016 at 10:30 PM, Peter J. Jones notifications@github.com
wrote:

The last open question is then: Under what circumstances would
getWindowAttributes throw an exception?

Didn't I already answer that? Window gets destroyed between the time xmonad
receives an event and calls the hook and when the hook calls
getWindowAttributes. This is rare, but can happen.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@pjones
Member
pjones commented Nov 23, 2016

@geekosaur Yep, you did say that ;) Thanks!

Any suggestions on how to handle something like that?

@geekosaur
Contributor

On Tue, Nov 22, 2016 at 10:36 PM, Peter J. Jones notifications@github.com
wrote:

Any suggestions on how to handle something like that?

Most of the time the only reasonable thing to do is abort the hook.
Exceptions do have the advantage of doing that automatically... provided
every use of getWindowAttributes is under the control of a userCode.
There may be some uses in core that are not (likely triggered by
X.O.manage).

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@asjo
asjo commented Nov 23, 2016

@pjones said:

Can you recall what situations led to exceptions being throw in xmonad?

One example that I have had crashing a lot is opening/saving various files in Gimp. I think what happens is that Gimp opens floating windows and closes them quickly again.

My prime example of the borders being transparent (i.e. the original problem were trying to fix) is the terminal program sakura.

I'll see if I can test your branch in the near future and report back to you.

@asjo
asjo commented Nov 23, 2016

Isn't the wa_colormap thing also set for RGBA-windows (e.g. sakura)? (My memory of why this thing got involved is vague, sorry.)

@geekosaur
Contributor

On Wed, Nov 23, 2016 at 12:23 AM, Adam Sjøgren notifications@github.com
wrote:

Isn't the wa_colormap thing also set for RGBA-windows (e.g. sakura)? (My
memory of why this thing got involved is vague, sorry.)

I wouldn't expect so. It's specifically for a window-local colormap. Alpha
channels should not be relevant to colormaps at all (they're treated
literally, not mapped to colors), and you don't want a private colormap in
a visual with 24+-bit color.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@asjo
asjo commented Nov 23, 2016

@geekosaur wrote:

This is rare, but can happen.

After adding the exception throwing and nothing else, I can say that it happens enough [for me] to be very noticable.

The places I have figured out to handle the exception include by far the ones where this happens most often. Among the remaining ones are the ones I can trigger with floating windows in Gimp, and not by much else.

@geekosaur
Contributor

On Wed, Nov 23, 2016 at 12:27 AM, Adam Sjøgren notifications@github.com
wrote:

After adding the exception throwing and nothing else, I can say that it
happens enough [for me] to be very noticable.

From what you described the Gimp as doing, it does sound like a
pathological case for this, yes.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@asjo
asjo commented Nov 23, 2016

Just FYI, the places I haven't been able to figure out how to handle the exceptions in xmonad proper are:

src/XMonad/Main.hs
295:    io $ putStrLn "asjo handle Main.hs L295"
428:  where ok w = do putStrLn "asjo scan Main.hs L428"

src/XMonad/Operations.hs
452:    io $ putStrLn "asjo floatLocation Operations.hs L452"
553:    putStrLn "asjo mkAdjust Operations.hs L551"

And with Gimp I've registered crashes from the latter:

floatLocation Operations.hs L457
xmonad-x86_64-linux: user error (Error in function getWindowAttributes)

(line numbers out of sync due to updates over the year).

@pjones Given that there are only two, maybe keeping the exception throwing and handling those two places might be an easier/better solution?

@asjo
asjo commented Nov 23, 2016

I wouldn't expect so. It's specifically for a window-local colormap. Alpha channels should not be relevant to colormaps at all (they're treated literally, not mapped to colors), and you don't want a private colormap in a visual with 24+-bit color.

It has been a while since I looked at this, but I was pretty convinced that the colormap thing is what makes the borders of programs like sakura transparent when using xmonad.

You're saying that it isn't?

The easiest way to reproduce is to have a colourful image as your desktop background, and then opening a bunch of sakura windows. The background peeks through the borders without the patches.

@asjo
asjo commented Nov 23, 2016

From what you described the Gimp as doing, it does sound like a pathological case for this, yes.

I think Gimp opens floating status windows that get closed quickly on some file sizes/some operatoins/some combination of computer speeds, yes.

@geekosaur
Contributor

On Wed, Nov 23, 2016 at 12:40 AM, Adam Sjøgren notifications@github.com
wrote:

It has been a while since I looked at this, but I was pretty convinced
that the colormap thing is what makes the borders of programs like sakura
transparent when using xmonad.

The cause of this is that xmonad sets the border colors ignoring the alpha
channel, and it ends up being 0 as a result in RGBA visuals. The fix is to
make that color setting RGBA-aware and force the alpha to 255.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@asjo
asjo commented Nov 23, 2016

The cause of this is that xmonad sets the border colors ignoring the alpha channel, and it ends up being 0 as a result in RGBA visuals. The fix is to make that color setting RGBA-aware and force the alpha to 255.

I wish someone told me this in 2015 when I started trying to fix this :-)

And this is better than using the windows own colormap if it has one? (As RGBA windows such as sakura's do.)

@geekosaur
Contributor

On Wed, Nov 23, 2016 at 12:41 AM, Brandon Allbery allbery.b@gmail.com
wrote:

The fix is to make that color setting RGBA-aware and force the alpha to
255.

There was a slightly horrid patch to xmonad's core on the old bug tracker
that always forced alpha channel to 255, relying on the X server not
validating the color for bits set that are out of range for the visual. It
was technically wrong but it worked reliably because xorg is lax in
validating colors. But someday they will fix that and the old hack will
stop working.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@geekosaur
Contributor

On Wed, Nov 23, 2016 at 12:45 AM, Adam Sjøgren notifications@github.com
wrote:

And this is better than using the windows own colormap if it has one? (As
RGBA windows such as sakura's do.)

OK, I just dug around to see why this is happening. The colormap for RGBA
windows is different indeed and matched to RGBA visuals, but the same for
all of them (it's actually a public colormap, just different from the
default RGB colormap) and probably it does handle this if the predefined
colormap isn't buggy.

BUT.

The digging I did also suggests that actually installing that colormap can
cause applications to crash. You can experiment with it, especially since
we really should obey a window-specified private colormap, but be aware
that it may do more harm than good. An improved/safer version of the old
hack may end up being a necessary fallback.

(Recent xf86/xorg developers have generally been, shall we say, not fully
competent. This is the same kind of crap as their being given a true
compositing extension and rejecting it in favor of the current
CPU-intensive compositor boondoggle because their incompetence could only
produce core dumps, and save-unders and double buffering being broken in
xfree86/xorg for close to a decade from what appears to have been much the
same bug as the one that caused them to scrap true compositing.)

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@geekosaur
Contributor

Oh, and I just ran into the biggie that caused so much pain in the past: Acrobat Reader requests an RGBA visual with matching colormap, and crashes if you give it that colormap. I don't know if this is still true; acroread has been persona non grata on my systems for many years and I have no intention of installing it to see if it works now. (That said, it might; Adobe spun Unix acroread off as some not-quite-open source IIRC, and the current maintainers might be slightly less naïve about X11 APIs.)

@asjo
asjo commented Nov 23, 2016

geekosaur writes:

The digging I did also suggests that actually installing that colormap can
cause applications to crash.

I haven't seen any due to this during the last year that I have been
running with these changes (but then again, I don't use Adobe software
on my computer), but I can make getWindowAttributes crash without fail.
This is anecdotal, of course.

You can experiment with it, especially since we really should obey a
window-specified private colormap, but be aware that it may do more
harm than good. An improved/safer version of the old hack may end up
being a necessary fallback.

So the conclusion is that what we are trying to achieve is best done
through using the window's colormap, as is attempted in the code in
issue?

@pjones
Member
pjones commented Nov 23, 2016

@geekosaur Since you know the most about this would you please take a look at commit 9bb2a54 and give us some feedback. From what we can tell, the patch from @asjo that fetches the border color from the window's colormap seems to work correctly.

@asjo I'll work on a patch for dealing with exceptions coming out of getWindowAttributes in core xmonad. I'd love it if you could test my branch after I'm done.

@geekosaur
Contributor

On Wed, Nov 23, 2016 at 2:03 AM, Adam Sjøgren notifications@github.com
wrote:

So the conclusion is that what we are trying to achieve is best done
through using the window's colormap, as is attempted in the code in
issue?

No, I'm just pointing out that we may end up needing a config option to do
it the hacky way instead if users find themselves getting app crashes.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@asjo
asjo commented Nov 23, 2016

geekosaur writes:

On Wed, Nov 23, 2016 at 2:03 AM, Adam Sjøgren notifications@github.com
wrote:

So the conclusion is that what we are trying to achieve is best done
through using the window's colormap, as is attempted in the code in
issue?

No, I'm just pointing out that we may end up needing a config option to do
it the hacky way instead if users find themselves getting app crashes.

I can't parse "No" in combination with the rest of your sentence.

If you had said "Yes, ...", I would have understood and agreed.

Did you really mean to say that should NOT try and use the window's
colormap?

If so, what should we do instead (as well as maybe providing a config
option for the hacky way if users see crashes)?

[Non-native English speaker here :-)]

@asjo
asjo commented Nov 23, 2016
@geekosaur
Contributor

On Wed, Nov 23, 2016 at 12:44 PM, Adam Sjøgren notifications@github.com
wrote:

Did you really mean to say that should NOT try and use the window's
colormap?

I think you managed to parse wrong a message back and are still
misinterpreting based on that (the "no" was aimed at that, since your
statement then was pretty much backwards from what I actually said). I said
quite clearly

especially since we really should obey a window-specified private
colormap

My point is that, due to bad application programming (e.g. Adobe) or
potential server bugs, it would be prudent to provide a fallback path that
is technically incorrect but less likely to cause crashes with such code,
since the user otherwise has no recourse.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@asjo
asjo commented Nov 23, 2016

Did you really mean to say that should NOT try and use the window's colormap?

I think you managed to parse wrong a message back and are still misinterpreting based on that (the "no" was aimed at that, since your statement then was pretty much backwards from what I actually said). I said quite clearly

especially since we really should obey a window-specified private colormap

I found that quite clear (I understood from the discussion that you changed your mind from "let's set RGBA and stick 255 in the A-channel" to "let's use the window's colormap"), so to be absolutely sure that everyone is on the same page, I tried to sum it up by writing this:

So the conclusion is that what we are trying to achieve is best done through using the window's colormap, as is attempted in the code in issue?

Which you said "No" to.

It is still unclear to me what you think the correct/best solution to the problem is, notwithstanding any extra configuration options for bad software.

I guess I shouldn't have tried to sum up :-)

What I thought you meant was:

  • The correct thing to do is to use the window's colormap, if it has one.
  • xmonad shouldn't use the hack of putting 255 in the A-channel.
  • A configuration option should be provided if bad applications crash due to the first point, falling back to ignoring the window's colormap (what xmonad 0.12 does).

The first two points are what I tried to sum up, and this is what I understood you to reply "No" to.

My point is that, due to bad application programming (e.g. Adobe) or potential server bugs, it would be prudent to provide a fallback path that is technically incorrect but less likely to cause crashes with such code, since the user otherwise has no recourse.

I understand this part. But you said "No" when I suggested that using the window's colormap is the preferred/default solution, or whatever you want to call it. So what is your conclusion on the "correct"/default thing to do?

@geekosaur
Contributor

On Wed, Nov 23, 2016 at 2:27 PM, Adam Sjøgren notifications@github.com
wrote:

Which you said "No" to.

I said "no" to your summary, which asserted (and you continue to assert)
that I was saying to only do it the hacky way and ignore the colormap,

I have said twice already that this is a fallback if obeying the colormap
causes crashes.

I do not know what to say to you that will make this clear. You asked a
specific question ("are you saying to ignore the colormap and use the
hack?") and I answered that question ("no, we should obey the colormap").
Somehow you continue to understand this as "no, we should ignore the
colormap" --- and to be honest, now that I have written that out I am
worried that you will manage to read that as my answer yet again.

USE THE COLORMAP. Preferably provide the hack as an emergency fallback for
when badly written applications crash.

brandon s allbery kf8nh sine nomine associates
allbery.b@gmail.com ballbery@sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@asjo
asjo commented Nov 23, 2016
@pjones pjones added a commit that referenced this pull request Nov 23, 2016
@pjones pjones Guard most calls to getWindowAttributes since it may throw an exception
This is a continuation of the work done by Adam Sjøgren (@asjo) to
resolve an issue where RGBA windows have transparent borders.  It also
fixes bugs related to windows suddenly disappearing right before
xmonad calls getWindowAttributes.

For more information see xmonad/xmonad#9
33b20e0
@pjones
Member
pjones commented Nov 23, 2016

@asjo My branch is ready for you to test. I'm looking forward to your feedback.

https://github.com/xmonad/xmonad/tree/feature/fix-PR9

@asjo
asjo commented Nov 24, 2016

@pjones Step 1: building, OK. Step 2: borders looking correct, OK. Step 3: see if I can make it crash. So far I have tried various things with Gimp, no crashes yet. I will try harder in the next couple of days.

Thanks for taking the time to look at this, I will be sure to read through your changes so I can learn from them, when I get some time.

@asjo
asjo commented Nov 25, 2016

@pjones I haven't been able to make xmonad from your branch crash. Very nice!

I wonder if there are any X stress-testing programs that could be used to test more thoroughly.

@pjones
Member
pjones commented Dec 8, 2016

I'm closing this PR since it has been superseded by #59

@pjones pjones closed this Dec 8, 2016
@pjones pjones locked and limited conversation to collaborators Dec 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.