Hi Stefan, thanks for reviewing my patch. Stefan Monnier writes: > [ Oh, and sorry for the copyright assignment repeat. I just noticed that > you already signed the paperwork. Not sure what I was smoking. ] :-) >> +;; The following code relies on the evaluation order of function >> +;; paramerters, which must be evaluated from left to right. According > ^^^^^^^^^^^ >> +;; to the elips manual "Evaluation of Function Forms" this is true. > ^^^^^ > Elisp > > Indeed, this property is true and will stay true, so you don't need > a comment about it, a lot of code silently relies on it. This was meant as a reminder to myself: I deleted it. > >> +;; Ther is no error checking performed wether the utf-8 character is > ^^^^ >> +;; encoded with minimal number of bytes. > > Why does it matter? The comment is a relict of the previous version of the code, where I did the UTF-8 parsing myself: I deleted the comment. >> +(defun read-utf8-char (&optional prompt seconds) > > Please use an "xterm-mouse-" prefix for all functions in xt-mouse.el. > In this case I'd call it xterm-mouse--read-utf8-char. You can add > a FIXME comment that you think this should be moved to something like > subr.el. I haven't found any use cases of this functionality outside of this file: I threrefore added a "xterm-mouse--" prefix and deleted the comment. >> +(defun xterm-mouse--read-number-from-terminal-1 (extension) >> + (let (c) >> + (if extension >> + (let ((n 0)) >> + (while (progn >> + (setq c (read-utf8-char)) > ^^^^^^^^^^^^^^^^ > IIUC this is always going to be ASCII, so we can just use read-event, > here, right? Yes: I changed it to read-char >> + (code (xterm-mouse--read-number-from-terminal c extension)) >> + (x (1- (xterm-mouse--read-number-from-terminal c extension))) >> + (y (1- (xterm-mouse--read-number-from-terminal c extension))) > > I think I'd prefer to use > > (pcase-let* > ((`(,code . ,_) (xterm-mouse--read-number-from-terminal-1 extension)) > (`(,x . ,_) (xterm-mouse--read-number-from-terminal-1 extension))) > (`(,y . ,c) (xterm-mouse--read-number-from-terminal-1 extension))) > <...and subtract 1 from x and y...> I did't knew about pcase, but I like it. >> + ;; The default mouse protocol does not report the button >> + ;; number in release events: use the button from the last >> + ;; button-down event. >> + (or (terminal-parameter nil 'xterm-mouse-last-button) >> + ;; Spurious release event without previous button-down >> + ;; event: assume, that the last button was button 1. >> + 1))) > > There's related code in xterm-mouse-translate-1 using the > xterm-mouse-last-down property. Is it because your code was written for > an older xt-mouse.el and you did not notice this change, or is there > some deeper reason why we need this apparent duplication? Hmm, I certaily have seen the xterm-mouse-last-down property, but somehow I came to the false conclusion, that the property does not record the button number. I fixed it and now use the `xterm-mouse-last-down´ property: (btn (cond ((or extension down wheel) (+ (logand code 3) (if wheel 4 1))) ;; The default mouse protocol does not report the button ;; number in release events: extract the button number ;; from last button-down event. ((terminal-parameter nil 'xterm-mouse-last-down) (string-to-number (substring (symbol-name (car (terminal-parameter nil 'xterm-mouse-last-down))) -1))) ;; Spurious release event without previous button-down ;; event: assume, that the last button was button 1. (t 1))) >> + (sym (if move 'mouse-movement >> + (intern (concat (if ctrl "C-" "") >> + (if meta "M-" "") >> + (if shift "S-" "") >> + (if down "down-" "") >> + "mouse-" >> + (number-to-string btn)))))) > > You could use `event-convert-list' here. Not sure if it'd really be > better, tho. With `event-convert-list´, the code would look similar to this: (sym (if move 'mouse-movement (event-convert-list (list (if ctrl 'ctrl) (if meta 'meta) (if shift 'shift) (if down 'down) (intern (concat "mouse-" (number-to-string btn))))))) I see only one advantage: the order, in which the event modifierers are prepended to the base event symbol, is ensured by `event-convert-list´. I left the code as it was. >> + (let* ((click (cond ((or (null extension) (= extension 1006)) > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Aka (memq extension '(1006 nil)) Agreed, this looks better. >> (defconst xterm-mouse-tracking-enable-sequence >> - "\e[?1000h\e[?1006h" >> + "\e[?1000h\e[?1002h\e[?1005h\e[?1006h" > > We should add a comment here explaining the name and intended effect of > each one of those escape sequences. I added an explanation in the documention string of the variable. This gives people, who use a terminal emulator other than xterm, a chance to delete unsupported escape sequences. (defconst xterm-mouse-tracking-enable-sequence "\e[?1000h\e[?1002h\e[?1005h\e[?1006h" "Control sequence to enable xterm mouse tracking. Enables basic mouse tracking, mouse motion events and finally extended tracking on terminals that support it. The following escape sequences are understood by modern xterms: \"\\e[?1000h\" `Basic mouse mode´: Enables reports for mouse clicks. There is a limit to the maximum row/column position (<= 223), which can be reported in this basic mode. \"\\e[?1002h\" `Mouse motion mode´: Enables reports for mouse motion events during dragging operations. \"\\e[?1005h\" `UTF-8 coordinate extension`: Enables an extension to the basic mouse mode, which uses UTF-8 characters to overcome the 223 row/column limit. This extension may conflict with non UTF-8 applications or non UTF-8 locales. \"\\e[?1006h\" `SGR coordinate extension´: Enables a newer alternative extension to the basic mouse mode, which overcomes the 223 row/column limit without the drawbacks of the UTF-8 coordinate extension. The two extension modes are mutually exclusive, where the last given escape sequence takes precedence over the former.") > > is undefined > [...] > > It seems, that changing the line while draging, generates a > > "vertical-line" event. > > This looks like the usual "location pseudo events" (not an official > name), such as the or events added before > mouse events that occur on those elements. > > > This can be fixed by adding a "vertical-line" mapping to > > the transient map, see my second patch. But I'm not sure, if generation > > of the "vertical-line" event should be prevented, instead. > > Good question. Why is there such a `vertical-line' event in this case > and not in the "usual" (e.g. GUI) case? Which code ends up generating > this event in this case but not in the GUI case? I finally found out where and why those events are generated in terminal frames (I didn't bother finding out what happens in GUI frames): Where: These `vertical-line´ events are generated by `xterm-mouse-event´. In `xterm-mouse-event´ the function `posn-at-x-y´ is used to find out at which window & position the xterm mouse event happend. Why: When dragging horizontally, `posn-at-x-y´ returns the window to the left or right of the `vertical-line´, since this was the location of the mouse pointer, when the event was triggerd. Similar, when dragging vertically, `posn-at-x-y´ *correctly* returns `vertical-bar´, because the mouse has not been dragged to an other window, but is still on the `vertical-line´. Therefore I think, that the `vertical-line´ events are legitimate and shouldn't be prevented, but instead ignored by the transient map in `mouse-drag-line´. I attached an update of my patches for xt-mouse.el and mouse.el Olaf