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

Mouse tracking tests #2321

Merged
merged 26 commits into from Jul 31, 2019
Merged

Mouse tracking tests #2321

merged 26 commits into from Jul 31, 2019

Conversation

jerch
Copy link
Member

@jerch jerch commented Jul 17, 2019

PR to have a solid mouse tracking test base.

The tests are a bit degraded atm to test and pass our current implementation. A new implementation should pass at least these tests to not introduce regressions. All tests have extensive comments about bugs and the desired behavior. A new implementation can work with these towards more standard conform behavior.

@jerch jerch added the work-in-progress Do not merge label Jul 17, 2019
@jerch jerch self-assigned this Jul 17, 2019
@jerch
Copy link
Member Author

jerch commented Jul 17, 2019

Added test stubs for modifier keys. None of these are working so all is commented out.

@jerch jerch removed the work-in-progress Do not merge label Jul 19, 2019
test/api/MouseTracking.api.ts Outdated Show resolved Hide resolved
@Tyriar Tyriar added this to the 4.0.0 milestone Jul 19, 2019
test/api/MouseTracking.api.ts Show resolved Hide resolved
// 2 bits: 0 - left, 1 - middle, 2 - right, 3 - release
// higher bits: 4 - shift, 8 - meta, 16 - control
const modifier = {shift: !!(code & 4), meta: !!(code & 8), control: !!(code & 16)};
const wheel = code & 64;

Choose a reason for hiding this comment

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

This is not actually a bit for wheel events. Xterm reports wheel events basically using the usual X11 button codes. But X11 only uses 4 buttons as wheel events and further traditional buttons use the same code space.
See https://terminalguide.netlify.com/mouse/ and the implementation for how xterm encodes the buttons.
X11 uses the buttons with the following mapping {1: left, 2: middle, 3: right, 4/5: scroll wheel, 6/7 scroll wheel left/right, 8-x: further buttons}
The xterm button encoding starts at 0, but reserves 3 as special "some button released" marker. So the buttons get shuffled to {0: left, 1: middle, 2: right, 4/5: scroll wheel, 6/7 scroll wheel left/right, 8-x: further buttons}
As the encoding of the button started with only 2 bits needed those 2 bits are used normally. The later addition of button numbers now up to 15 while the modifiers already got bits in between resulted in the bits with values 4 and 8 to be transposed up to the bits with values 64 and 128.

So while (code & 64) == 1 && (code & 128) == 0 end up encoding wheel events, (code & 64) == 1 && (code & 128) == 1 encodes button events again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh thats kinda different from whats stated in the xterm docs itself. Seems I have to work through your links first. Also we are currently limited to 3 button mouse + wheel (cannot test higher buttons myself, not even sure if browsers agree for those on the buttons reported).

Copy link
Member Author

@jerch jerch Jul 22, 2019

Choose a reason for hiding this comment

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

For my understanding - what would (code & 64) == 1 && (code & 128) == 1 encode here? Maybe I have a wrong assumption, for me doing +64 is equivalent to |64, thus the opposite &64 can test for it. Ofc this only works if the lower range does not overflow (create that bit by other means), which seems to be the case?
The example you gave just looks wrong to me as it cannot be reached by normal means. To me the 128+ rule read like the one for wheel - buttons from 8 to 11 would just be added to 128. Again I cannot test the higher buttons myself.

Copy link
Member Author

@jerch jerch Jul 22, 2019

Choose a reason for hiding this comment

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

Hmm ok seems past button 11 overflow actually happens, thus my |64 == +64 does not hold here. Geez this buttonCode stuff is really awkward designed.
Edit: Yeah need to rework this and apply the "spreading bit" behavior in the test here and in the new implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@textshell: Changed the buttonCode eval to the reverse of the xterm code. Plz have a look at it again, hope I did not miss anything.

Choose a reason for hiding this comment

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

I've added a few comments to the new commit. For the actually defined range your commit should work, but see my comments for ideas on simplifying it and making it a bit more meaningful with further bits set.

The encoding is a bit historical, but i don't think it could be a lot easier given the history and the strong preference for terminal sequence to use ultra compact encoding (which is still strong even today).

If you are using linux (or a linux VM) you can test with other buttons using the trick in
https://gitlab.gnome.org/GNOME/vte/issues/93#note_433817

test/api/MouseTracking.api.ts Outdated Show resolved Hide resolved
test/api/MouseTracking.api.ts Outdated Show resolved Hide resolved
test/api/MouseTracking.api.ts Outdated Show resolved Hide resolved
test/api/MouseTracking.api.ts Show resolved Hide resolved
@textshell textshell mentioned this pull request Jul 21, 2019
code -= 64;
}
button += code & 3;
}

Choose a reason for hiding this comment

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

The special case for code === 3 is not needed.
I think you could also use something like this:

let button = code & 3;
if (code & 128) {
    button |= 8;
}
if (code & 64) {
   button |= 4
}

This would ignore bits allocated in the future, but otherwise be very safe. But in a unit test it might be better to reject the sequence if code > 255 (like using actionS = 'invalid')

I think your code would get wrong results when confronted with code that has further bits set.

Copy link
Member Author

Choose a reason for hiding this comment

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

@textshell Yeah much cleaner.

Is there a valid way to get here code > 255? To me it seems this cannot be reached by normal "button addressing" (no +256 rule for 4th bit)? However put the code > 255 in just to be safe against malformed stuff.

Choose a reason for hiding this comment

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

These things are always evolving. So it's always good to think about what should happen if someone happens to allocate a new bit (which is not that hard if you have a good use case). Or in a unit test, if the code under test does something unexpected.

The button code decoding looks good now.

test/api/MouseTracking.api.ts Outdated Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Jul 23, 2019

@Tyriar
Phew, MacOS tests work now. Problem was the Shift modifier, for some reason that gets not caught by the selection manager. I think we should unify the escape strategies for default actions across the platforms and maybe make it customizable so vendors can do as they wish. Already scratched that topic here #2323 (comment).

@@ -332,6 +335,8 @@ describe('Mouse Tracking Tests', function(): void {

// SHIFT
// note: caught by selection manager
// bug? Why not caught by selection manger on macos?
Copy link
Member

Choose a reason for hiding this comment

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

TODO: ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, in the selection manager there is an explicit test against the platform. Seems it was done that way by purpose?

@@ -20,6 +20,9 @@ const fontSize = 6;
const cols = 260;
const rows = 50;

// for some reason shift gets not caught by selection manager on macos
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what's shift meant to do? macOptionClickForcesSelection is the main mac different thing around mouse that I can think of.

Copy link
Member Author

Choose a reason for hiding this comment

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

Culprit is this code:

public shouldForceSelection(event: MouseEvent): boolean {
if (Browser.isMac) {
return event.altKey && this._optionsService.options.macOptionClickForcesSelection;
}
return event.shiftKey;
}

For some reason Mac is treated differently.

Copy link
Member

Choose a reason for hiding this comment

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

Oh OK, I think I used another terminal as a basis for that (user expectations for platform).

@jerch jerch mentioned this pull request Jul 24, 2019
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Good to merge after CI

@jerch jerch merged commit a89771a into xtermjs:master Jul 31, 2019
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.

None yet

3 participants