sidorares / node-x11

X11 node.js network protocol client
MIT License
518 stars 72 forks source link

FocusIn/FocusOut events aren't parsed #201

Closed danbulant closed 3 years ago

danbulant commented 3 years ago

I'm trying to write a simple WM, and spent ~4 hours just looking what I'm doing wrong that the focus isn't getting changed correctly (I want to raise focused windows). node-x11 doesn't parse FocusIn/FocusOut events at all (code 9 and 10).

sidorares commented 3 years ago

can you put some very basic repro example for me to save some time debugging?

danbulant commented 3 years ago

I'm not exactly sure how unpack works (I'm new to all this, just spent last couple of days writing WM and playing with X), so I'm not going to implement it myself.

I found out about this when looking through source code and saw that event codes 9 (FocusIn), 10 (FocusOut) and 11 (KeymapNotify) aren't parsed here. Format can be found here under FocusIn.

As for the general repro, creating two windows, setting the FocusChange mask on them and listening for events with the name FocusIn or FocusOut will do (they won't as unparsed events don't have name, so this can be used as test if the implementation is correct).

var x11 = require('x11');
x11.createClient(function(err, display) {
  if(err) throw err;

  var X = display.client;
  var root = display.screen[0].root;
  var wid = X.AllocID();
  var wid2 = X.AllocID();

  X.CreateWindow(wid, root, 0, 0, 150, 150, 0, 0, 0, 0, {
    backgroundPixel: this.screen.white_pixel,
    eventMask: x11.eventMask.FocusChange
  });
  X.CreateWindow(wid2, root, 250, 250, 100, 100, 0, 0, 0, 0, {
    backgroundPixel: this.screen.white_pixel,
    eventMask: x11.eventMask.FocusChange
  });

  X.on("event", (ev) => {
    if([9, 10].includes(ev.type)) console.log("Focus changed event", ev); // will log both FocusIn and FocusOut
    if(["FocusIn", "FocusOut"].includes(ev.name)) console.log("Focus change event name parsed correctly");
  });

  X.SetInputFocus(wid);
  setTimeout(() => X.SetInputFocus(wid2), 500);
});

Then change focus between the windows (either programmatically using SetInputFocus or (when running with WM) clicking the windows).

(edit: had a typo in code)

danbulant commented 3 years ago

    if (ev.type === 9) {
        ev.name = "FocusIn";
        ev.detail = ev.rawData.readUInt8(1);
        ev.seq = ev.rawData.readUInt16LE(2);
        ev.wid = ev.rawData.readUInt32LE(4);
        ev.mode = ev.rawData.readUInt8(8);
    } else if (ev.type === 10) {
        ev.name = "FocusOut";
        ev.detail = ev.rawData.readUInt8(1);
        ev.seq = ev.rawData.readUInt16LE(2);
        ev.wid = ev.rawData.readUInt32LE(4);
        ev.mode = ev.rawData.readUInt8(8);
    }

This code works well for me when parsed in client on event handler.

sidorares commented 3 years ago

you see how unpack works here https://github.com/sidorares/node-x11/blob/97e3600467279bf16f41e8fccf7b3250804f1eec/lib/unpackbuffer.js#L25-L50

I think I modelled it to be similar to python's https://docs.python.org/3/library/struct.html#format-characters

All events are always 32 bytes long, first 8 bytes are common to all events and read here as CCSL - type, code, sequence and "extra". rest 24 bytes are passed as "raw" buffer ( also first 8 bytes as headerBuf for events that don't follow CCSL format )

so to add FocusIn / FocusOut we can do here something like

event.name = 'FocusIn'
event.mode = raw.unpack('C')[0];
event.wid = extra;
danbulant commented 3 years ago

I think I modelled it to be similar to python's https://docs.python.org/3/library/struct.html#format-characters

Thanks, didn't know that, never really used python.

so to add FocusIn / FocusOut we can do here something like

Should I make a PR then?

sidorares commented 3 years ago

Should I make a PR then?

Please do!