The minimal diff (plus honoring bwidth changes) was applied; thanks for your patience!
On Fri, May 24, 2013 at 8:01 AM, Okan Demirmen <[email protected]> wrote: > Hi Mike, > > I could have replied earlier - your 4 line diff exploded into a huge > diff to fix event handling (recall I said I was sure some other stuff > was not right :)). > > If I don't have a working re-write soon, I'll address just the bits > you've pointed out separately and move on from there. Thank you for > your detailed reports so far! > > Thanks, > Okan > > On Thu, May 23, 2013 at 9:36 PM, Mike Small <[email protected]> wrote: >> >> Maybe a problem with my previous report was that the steps only gave a >> reproducible case on macppc (and even that was surprising since the >> uninitialized variable, wc.stack_mode, had to happen to pick up the >> value 1 -- the value for Below -- though it consistently happened for me >> and another user on daemonforums on our macppc machines). Here are >> steps I can use to show the bug on an amd64 machine. This variation >> should be more likely to be reproducible in general since any value >> other than 1 for wc.stack_mode will yield incorrect behaviour. >> >> I) with emacs (this time 21.3 from the 5.3 cd, but that shouldn't matter): >> 1. Place the emacs window above another window >> 2. In the *Scratch* buffer evaluate the following expression: >> (lower-frame) ; eval an expression by pressing ctrl-c, ctrl-e >> ; after it or ctrl-meta-x in it >> >> The frame incorrectly stays on top. >> >> II) without emacs: >> 1. compile the following program >> ======================================================================== >> #include <X11/Xlib.h> >> #include <X11/Xutil.h> >> >> #include <err.h> >> #include <stdlib.h> >> >> const char msg_lower[] = "Click to lower."; >> const char msg_quit[] = "Q to quit."; >> >> int >> main() >> { >> Display *dpy; >> Screen *s; >> Window win; >> XEvent ev; >> GC gc; >> XGCValues values; >> KeySym k; >> unsigned long black; >> >> dpy = XOpenDisplay(NULL); >> if (!dpy) >> errx(1, "XOpenDpy: Couldn't open X dpy."); >> s = DefaultScreenOfDisplay(dpy); >> black = BlackPixelOfScreen(s); >> >> win = XCreateSimpleWindow(dpy, RootWindowOfScreen(s), >> WidthOfScreen(s)/2 - 100, >> HeightOfScreen(s)/2 - 100, >> 180, 150, 2, >> black, WhitePixelOfScreen(s)); >> XSelectInput(dpy, win, ButtonPressMask|KeyReleaseMask|ExposureMask); >> XMapWindow(dpy, win); >> >> values.font = XLoadFont(dpy, "fixed"); >> gc = XCreateGC(dpy, win, GCFont, &values); >> >> while (1) { >> XNextEvent(dpy, &ev); >> switch (ev.type) { >> case ButtonPress: >> XLowerWindow(dpy, win); >> break; >> case KeyRelease: >> k = XkbKeycodeToKeysym(dpy, ev.xkey.keycode, 0, 0); >> if (k == XK_q || k == XK_Q) { >> XCloseDisplay(dpy); >> return 0; >> } >> break; >> case Expose: >> XDrawString(dpy, win, gc, 20, 20, msg_lower, >> sizeof(msg_lower) - 1); >> XDrawString(dpy, win, gc, 20, 75, msg_quit, >> sizeof(msg_quit) - 1); >> break; >> } >> } >> return 1; >> } >> >> ======================================================================== >> 2. place its window above another window >> 3. click on its window >> >> The window again incorrectly stays on top. >> >> After applying the following patch, I see the right behaviour on my >> amd64 machine. The windows drop to the bottom of the stacking order in >> each case. >> >> Index: xevents.c >> =================================================================== >> RCS file: /cvs/xenocara/app/cwm/xevents.c,v >> retrieving revision 1.71 >> diff -u -u -p -r1.71 xevents.c >> --- xevents.c 5 Apr 2013 17:07:25 -0000 1.71 >> +++ xevents.c 24 Apr 2013 02:49:00 -0000 >> @@ -151,6 +151,10 @@ xev_handle_configurerequest(XEvent *ee) >> cc->geom.y = e->y; >> if (e->value_mask & CWBorderWidth) >> wc.border_width = e->border_width; >> + if (e->value_mask & CWSibling) >> + wc.sibling = e->above; >> + if (e->value_mask & CWStackMode) >> + wc.stack_mode = e->detail; >> >> if (cc->geom.x == 0 && cc->geom.w >= sc->view.w) >> cc->geom.x -= cc->bwidth; >> >> >> dmesg for this other machine (acpi disabled to get it booting -- I'll >> provide a proper report for that issue sometime): >> >> OpenBSD 5.3 (GENERIC) #53: Tue Mar 12 18:15:44 MDT 2013 >> [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC >> real mem = 1600815104 (1526MB) >> avail mem = 1535799296 (1464MB) >> mainbus0 at root >> bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xfcd90 (44 entries) >> bios0: vendor American Megatrends Inc. version "306" date 09/14/2007 >> bios0: ASUSTeK Computer Inc. Z35FM >> acpi at bios0 not configured >> mpbios0 at bios0: Intel MP Specification 1.4 >> cpu0 at mainbus0: apid 0 (boot processor) >> cpu0: Intel(R) Core(TM)2 CPU T7200 @ 2.00GHz, 1995.32 MHz >> cpu0: >> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,NXE,LONG,LAHF,PERF >> cpu0: 4MB 64b/line 16-way L2 cache >> cpu0: apic clock running at 166MHz >> cpu at mainbus0: not configured >> mpbios0: bus 0 is type PCI >> mpbios0: bus 1 is type PCI >> mpbios0: bus 2 is type PCI >> mpbios0: bus 3 is type PCI >> mpbios0: bus 4 is type PCI >> mpbios0: bus 5 is type PCI >> mpbios0: bus 6 is type ISA >> ioapic0 at mainbus0: apid 2 pa 0xfec00000, version 20, 24 pins >> cpu0: unknown Enhanced SpeedStep CPU, msr 0x06130c2806000c28 >> cpu0: using only highest and lowest power states >> cpu0: Enhanced SpeedStep 1995 MHz: speeds: 2000, 1000 MHz >> pci0 at mainbus0 bus 0 >> pchb0 at pci0 dev 0 function 0 "Intel 82945GM Host" rev 0x03 >> vga1 at pci0 dev 2 function 0 "Intel 82945GM Video" rev 0x03 >> wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation) >> wsdisplay0: screen 1-5 added (80x25, vt100 emulation) >> intagp0 at vga1 >> agp0 at intagp0: aperture at 0xd0000000, size 0x10000000 >> inteldrm0 at vga1: apic 2 int 16 >> drm0 at inteldrm0 >> "Intel 82945GM Video" rev 0x03 at pci0 dev 2 function 1 not configured >> azalia0 at pci0 dev 27 function 0 "Intel 82801GB HD Audio" rev 0x02: msi >> azalia0: codecs: Realtek ALC660, Motorola/0x3055, using Realtek ALC660 >> audio0 at azalia0 >> ppb0 at pci0 dev 28 function 0 "Intel 82801GB PCIE" rev 0x02: msi >> pci1 at ppb0 bus 1 >> ppb1 at pci0 dev 28 function 1 "Intel 82801GB PCIE" rev 0x02 >> pci2 at ppb1 bus 4 >> wpi0 at pci2 dev 0 function 0 "Intel PRO/Wireless 3945ABG" rev 0x02: msi, >> MoW1, address 00:13:02:1a:b1:b4 >> uhci0 at pci0 dev 29 function 0 "Intel 82801GB USB" rev 0x02: apic 2 int 23 >> uhci1 at pci0 dev 29 function 1 "Intel 82801GB USB" rev 0x02: apic 2 int 19 >> uhci2 at pci0 dev 29 function 2 "Intel 82801GB USB" rev 0x02: apic 2 int 18 >> uhci3 at pci0 dev 29 function 3 "Intel 82801GB USB" rev 0x02: apic 2 int 16 >> ehci0 at pci0 dev 29 function 7 "Intel 82801GB USB" rev 0x02: apic 2 int 23 >> usb0 at ehci0: USB revision 2.0 >> uhub0 at usb0 "Intel EHCI root hub" rev 2.00/1.00 addr 1 >> ppb2 at pci0 dev 30 function 0 "Intel 82801BAM Hub-to-PCI" rev 0xe2 >> pci3 at ppb2 bus 5 >> "Ricoh 5C832 Firewire" rev 0x00 at pci3 dev 3 function 0 not configured >> sdhc0 at pci3 dev 3 function 1 "Ricoh 5C822 SD/MMC" rev 0x19: apic 2 int 20 >> sdmmc0 at sdhc0 >> "Ricoh 5C843 MMC" rev 0x01 at pci3 dev 3 function 2 not configured >> "Ricoh 5C592 Memory Stick" rev 0x0a at pci3 dev 3 function 3 not configured >> "Ricoh 5C852 xD" rev 0x05 at pci3 dev 3 function 4 not configured >> rl0 at pci3 dev 4 function 0 "Realtek 8139" rev 0x10: apic 2 int 20, address >> 00:18:f3:cf:55:06 >> rlphy0 at rl0 phy 0: RTL internal PHY >> pcib0 at pci0 dev 31 function 0 "Intel 82801GBM LPC" rev 0x02 >> pciide0 at pci0 dev 31 function 1 "Intel 82801GB IDE" rev 0x02: DMA, channel >> 0 configured to compatibility, channel 1 configured to compatibility >> wd0 at pciide0 channel 0 drive 0: <ST980825A> >> wd0: 16-sector PIO, LBA48, 76319MB, 156301488 sectors >> atapiscsi0 at pciide0 channel 0 drive 1 >> scsibus0 at atapiscsi0: 2 targets >> cd0 at scsibus0 targ 0 lun 0: <TSSTcorp, CD/DVDW TS-L632D, AS05> ATAPI >> 5/cdrom removable >> wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 5 >> cd0(pciide0:0:1): using PIO mode 4, Ultra-DMA mode 2 >> pciide0: channel 1 ignored (disabled) >> usb1 at uhci0: USB revision 1.0 >> uhub1 at usb1 "Intel UHCI root hub" rev 1.00/1.00 addr 1 >> usb2 at uhci1: USB revision 1.0 >> uhub2 at usb2 "Intel UHCI root hub" rev 1.00/1.00 addr 1 >> usb3 at uhci2: USB revision 1.0 >> uhub3 at usb3 "Intel UHCI root hub" rev 1.00/1.00 addr 1 >> usb4 at uhci3: USB revision 1.0 >> uhub4 at usb4 "Intel UHCI root hub" rev 1.00/1.00 addr 1 >> isa0 at pcib0 >> isadma0 at isa0 >> pckbc0 at isa0 port 0x60/5 >> pckbd0 at pckbc0 (kbd slot) >> pckbc0: using irq 1 for kbd slot >> wskbd0 at pckbd0: console keyboard, using wsdisplay0 >> pms0 at pckbc0 (aux slot) >> pckbc0: using irq 12 for aux slot >> wsmouse0 at pms0 mux 0 >> pms0: Synaptics touchpad, firmware 6.2 >> pcppi0 at isa0 port 0x61 >> spkr0 at pcppi0 >> mtrr: Pentium Pro MTRR support >> uhub5 at uhub2 port 1 "Chicony Generic USB Hub" rev 1.10/1.00 addr 2 >> uhidev0 at uhub5 port 1 configuration 1 interface 0 "Chicony PFU-65 USB >> Keyboard" rev 1.10/1.00 addr 3 >> uhidev0: iclass 3/1 >> ukbd0 at uhidev0: 8 variable keys, 6 key codes >> wskbd1 at ukbd0 mux 1 >> wskbd1: connecting to wsdisplay0 >> uhidev1 at uhub5 port 3 configuration 1 interface 0 "Microsoft Microsoft >> 3-Button Mouse with IntelliEye(TM)" rev 1.10/3.00 addr 4 >> uhidev1: iclass 3/1 >> ums0 at uhidev1: 3 buttons, Z dir >> wsmouse1 at ums0 mux 0 >> vscsi0 at root >> scsibus1 at vscsi0: 256 targets >> softraid0 at root >> scsibus2 at softraid0: 256 targets >> root on wd0a (72eb03b5e79429f1.a) swap on wd0b dump on wd0b >> >> >> >> >> Mike Small <[email protected]> writes: >> >>> Okan Demirmen <[email protected]> writes: >>> >>>> On Thu 2013.05.02 at 00:22 -0400, [email protected] wrote: >>>> Hi, >>>> >>>> First off, thanks for the detailed report! >>>> >>>>> While using emacs org mode and entering a timestamp using [C-c !] the >>>>> emacs window would drop to the bottom of the X stacking order. This >>>>> command can be traced to the raise-frame function which sends a raise >>>>> window request to cwm. >>>>> >>>>> It happened consistently for me, but when I asked another user on >>>>> daemonforums to try it he found it only happened on his macppc and not >>>>> his i386 machine. Also, I had it go away briefly when I fumbled in >>>>> getting debugging information (unstripped but no -g or maybe a mix of >>>>> object files with -g and without, not sure now). I mention this in >>>>> case you don't see the problem yourself. There's an uninitialized auto >>>>> involved, which I believe to be the cause of the pseudo-intermittency. >>>>> >>>>> With debugging info I could step through >>>>> xevents.c::xev_handle_configurerequest >>>>> and see the following: >>>>> >>>>> 1. The call to XConfigureWindow reuses the value mask from the >>>>> incoming XConfigureReqestEvent. >>>>> 2. The value of that mask was 64. i.e. CWStackMode (defined as 1 << 6 in >>>>> X.h) >>>>> 3. CWStackMode is not one of the bits looked for in the preceding >>>>> check masks and set corresponding wc field statements. >>>>> 4. Not involved in my scenario I don't think, but CWSibling is also >>>>> missing. >>>>> 5. The XWindowChanges variable wc used for the XConfigureWindow call >>>>> happens to have the value 1 for its stack_mode field for me. 1 is >>>>> defined as Below in X.h. Above is 0. >>>>> >>>>> When I add checks for CWStackMode and CWSibling with appropriate sets >>>>> of the corresponding wc fields (excuse the pun) the problem seems >>>>> fixed. If it's on top it stays on top. If it's lower it raises. >>>> >>>> Unfortunately, I'm not able to replicate, nor I suppose understand, the >>>> current behavior wrt to the behavior with the patch applied - basically, >>>> there's no change in my tests. I could be missing your point... >>> >>> Sorry I was unclear. Annotations follow (excuse me if this is gets too >>> verbose or if I state obvious things)... >>> >>> static void >>> xev_handle_configurerequest(XEvent *ee) >>> { >>> XConfigureRequestEvent *e = &ee->xconfigurerequest; >>> struct client_ctx *cc; >>> struct screen_ctx *sc; >>> >>> -> wc is not initialized here (of course, this isn't C++), particularly the >>> -> .sibling and .stack_mode fields. As matters later, these fields >>> -> correspond, I believe (I have an Xlib book on order -- the X man pages >>> -> leave me guessing slightly on this point), to the .above and .detail >>> -> fields in XConfigureRequestEvent. See XConfigureRequestEvent(3) and >>> -> XConfigureWindow(3). This latter page has problems you can see below, >>> -> btw. I intend to report this (to OpenBSD? to Xorg?) later along with >>> -> another man page problem but don't have my notes handy for that. >>> >>> XWindowChanges wc; >>> >>> if ((cc = client_find(e->window)) != NULL) { >>> sc = cc->sc; >>> >>> if (e->value_mask & CWWidth) >>> cc->geom.w = e->width; >>> if (e->value_mask & CWHeight) >>> cc->geom.h = e->height; >>> if (e->value_mask & CWX) >>> cc->geom.x = e->x; >>> if (e->value_mask & CWY) >>> cc->geom.y = e->y; >>> >>> -> This if block is useless since wc.border_width is re-assigned below. >>> >>> if (e->value_mask & CWBorderWidth) >>> wc.border_width = e->border_width; >>> >>> if (cc->geom.x == 0 && cc->geom.w >= sc->view.w) >>> cc->geom.x -= cc->bwidth; >>> >>> if (cc->geom.y == 0 && cc->geom.h >= sc->view.h) >>> cc->geom.y -= cc->bwidth; >>> >>> -> Next is where wc's members get their values, but two of them >>> -> (namely .sibling and .stack_mode) are not set so will have fairly >>> unpredictable >>> -> values. No doubt when you run you happen to get a zero for >>> -> wc.stacking_mode, which corresponds to Above. I get 1, corresponding to >>> -> Below. If you try my steps again, but instead put my test program window >>> -> on the bottom and click on a corner hanging out from under, my guess is >>> -> that you'll see nothing happen. But if you look at my code you can see >>> -> that clicking on the window should make it request a raise in the >>> -> stacking order. >>> -> >>> -> From XConfigureWindow(3): >>> -> >>> -> /* Configure window value mask bits */ >>> -> >>> -> #define .el CWX (1<<0) [[what's this .el doing >>> here?]] >>> -> #define .el CWY (1<<1) >>> -> #define .el CWWidth (1<<2) >>> -> #define .el CWHeight (1<<3) >>> -> #define .el CWBorderWidth (1<<4) >>> -> #define .el CWSibling (1<<5) >>> -> #define .el CWStackMode (1<<6) >>> -> /* Values */ >>> -> >>> -> typedef struct { >>> -> int x, y; >>> -> int width, height; >>> -> int border_width; >>> -> Window sibling; >>> -> int stack_mode; >>> -> } XWindowChanges; >>> -> >>> -> From X.h: >>> -> #define Above 0 >>> -> #define Below 1 >>> -> #define TopIf 2 >>> -> #define BottomIf 3 >>> -> #define Opposite 4 >>> -> >>> wc.x = cc->geom.x; >>> wc.y = cc->geom.y; >>> wc.width = cc->geom.w; >>> wc.height = cc->geom.h; >>> wc.border_width = cc->bwidth; >>> >>> -> Next we're re-using the e->value_mask that came in with the client's >>> -> configure request for our outgoing configure request. But e->value_mask >>> -> was set to tell us what we can look at on ee->xconfigurerequest. >>> -> XConfigureWindow needs a mask to say which fields we set on wc. The set >>> -> of fields we assigned on wc is not the same as the set of fields that >>> -> were assigned on e by the client's request (wc lacks sibling and >>> stack_mode, >>> -> or rather has not very predictable values for those since we didn't >>> -> assign to them). When I looked in the debugger I could see that >>> -> e->value_mask was 64, a.k.a. 1<<6, a.k.a. CWStackMode, and that >>> wc.stack_mode >>> -> happened to be 1 somehow. The result was to send a stacking order >>> -> request with this uninitialized value, 1 or Below, for wc.stack_mode >>> -> and sending emacs or my test program to the bottom. You may have 0 >>> -> (Above) sending your window to the top. If you start with it at the top, >>> -> you see no action. >>> -> >>> XConfigureWindow(X_Dpy, cc->win, e->value_mask, &wc); >>> xu_configure(cc); >>> } else { >>> /* let it do what it wants, it'll be ours when we map it. */ >>> wc.x = e->x; >>> wc.y = e->y; >>> wc.width = e->width; >>> wc.height = e->height; >>> wc.border_width = e->border_width; >>> wc.stack_mode = Above; >>> e->value_mask &= ~CWStackMode; >>> >>> XConfigureWindow(X_Dpy, e->window, e->value_mask, &wc); >>> ... >>> >>> >>> I hope the problem is clearer now. If not, let me know and I'll try >>> again. I often struggle to make myself understood it seems. >>> >>> So if it is clear, then about my patch. I thought it might be too bold >>> to say so initially, but I don't think re-using e->value_mask in >>> XConfigureWindow is wise here. If the changes structure ever gets new >>> members then we're right back to the same problem. I'd think it better >>> to dedicate a separate mask variable for XConfigureWindow's use that >>> gets its bits set as the corresponding changes struct fields are >>> set. e.g. something like this (untested)... >>> >>> ... >>> unsigned long wc_values_mask; >>> ... >>> wc_values_mask = 0 >>> ... >>> if (e->value_mask & CWSibling) { >>> wc.sibling = e->above; >>> wc_values_mask |= CWSibling; >>> } >>> if (e->value_mask & CWStackMode) { >>> wc.stack_mode = e->detail; >>> wc_values_mask |= CWStackMode; >>> } >>> >>> wc.x = cc->geom.x; >>> wc.y = cc->geom.y; >>> wc.width = cc->geom.w; >>> wc.height = cc->geom.h; >>> wc.border_width = cc->bwidth; >>> wc_values_mask |= CWX | CWY | CWWidth | CWHeight | CWBorderWidth; >>> >>> XConfigureWindow(X_Dpy, e->window, wc_values_mask, &wc); >>> >>> But I'm not an Xlib programmer, so I have no idea if it's a realistic >>> expectation for XConfigureRequestEvent and XWindowChanges to ever >>> change.
