[vim/vim] Add (enable) support for MouseMove events (PR #10044)
errael
Goal
First phase: update Splice (merge conflict resolution), a vim-python plugin that's been idle about 10 years, to vim9 and move some python code over to vim and fix a variety of issues (include settings validation) and increase configurability (shortcuts, highlights...) and provide additional ways to interact with app.
The tool has a 3 line window at top with status and a list of commands. I've made the commands clickable and highlight them when mouse rollover (using text properties). The rollover should be particularly useful for existing users who don't read the documentation to alert them of something new.
To get the rollover effect, a patch is needed that deliver MOUSE_MOVE events, which are then access by mapping .
Overhead
When the mouse is not moving, there is no additional overhead. When the mouse is moving, the additional overhead per mouse interrupt is tiny: entry to gui_send_mouse_event() and switch (button) and gui_xy2colrow() and a few comparisons. An event is queued, add_to_input_buf(), only when the cursor moves to a different character. Little overhead.
The changes to gui.c could be restricted to gui_mouse_moved. In this case, gui_mouse_moved would maintain it's own prev_row/prev_col. As well as isolating the change, it would be a further small reduction in overhead for this change.
Overhead could be eliminated when <MouseMove> events are not mapped, for example introducing a global mapping_uses_mouse_move. Is this worth exploring?
Testing
I looked around a little in testdir. In general, motion is difficult to test; and test_gui_event('mouse', ...) doesn't have any motion related stuff. Any testing by injecting into input buf covers existing code paths that are not being modified. Is this patch simple enough as is?
Todo
I propose adding, after MOUSE_MOVE in vim.h,
#define MOUSE_MOVE_POPUP 0x800 // report mouse moved for popup consumption
and use that when popup_uses_mouse_move is true. Use the existing MOUSE_MOVE for the regular per character move events. Note: with some of the possibilities discussed above, no new define is required.
Are there other parts of vim that might be sensitive to this change?
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/10044
Commit Summary
- f15f012 Mouse move events per char move
File Changes
(1 file)
- M src/gui.c (22)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> **Goal**
> First phase: update Splice (merge conflict resolution), a vim-python
> plugin that's been idle about 10 years, to vim9 and move some python
> code over to vim and fix a variety of issues (include settings
> validation) and increase configurability (shortcuts, highlights...)
> and provide additional ways to interact with app.
>
> The tool has a 3 line window at top with status and a list of
> commands. I've made the commands clickable and highlight them when
> mouse rollover (using text properties). The rollover should be
> particularly useful for existing users who don't read the
> documentation to alert them of something new.
>
> To get the rollover effect, a patch is needed that deliver
>
> **Overhead**
> When the mouse is not moving, there is no additional overhead. When
> the mouse is moving, the additional overhead per mouse interrupt is
> tiny: entry to `gui_send_mouse_event()` and `switch (button)` and
> `gui_xy2colrow()` and a few comparisons. An event is queued,
> character. Little overhead.
>
> The changes to gui.c could be restricted to gui_mouse_moved. In this
> case, gui_mouse_moved would maintain it's own prev_row/prev_col. As
> well as isolating the change, it would be a further small reduction in
> overhead for this change.
>
> Overhead could be eliminated when `<MouseMove>` events are not mapped,
> worth exploring?
How does the plugin get the mouse move events? Depending on that we
could decide to have a way to enable the events, like the
"popup_uses_mouse_move" flag.
> **Testing**
>
> I looked around a little in testdir. In general, motion is difficult
> to test; and `test_gui_event('mouse', ...)` doesn't have any motion
> related stuff. Any testing by injecting into input buf covers existing
> code paths that are not being modified. Is this patch simple enough as
> is?
for this. I guess the equivalent of KE_MOUSEMOVE_XY. Perhaps the value
0x700 works?
--
Q: What do you call a fish without an eye?
A: fsh!
Q: What do you call a deer with no eyes?
A: no eye deer.
Q: What do you call a deer with no eyes and no legs?
A: still no eye deer.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
How does the plugin get the mouse move events? Depending on that we could decide to have a way to enable the events, like the "popup_uses_mouse_move" flag.
With only the change to vim in this PR, #10044, the plugin is doing
nnoremap <special> <MouseMove> <ScriptCmd>Move()<CR>
and getting Move events; and the plugin highlighting command when rollover is implemented and works.
Note that misc2.c already has
{K_MOUSEMOVE, (char_u *)"MouseMove"},
which is why I say that delivery path of the event is already tested.
New things should be tested. test_gui_event() can perhaps be extended
for this.
Something like mapping_uses_mouse_move could be set/reset in map.c::do_map. With that, then there's be something to test, I could modify test_gui_event to query the value of mapping_uses_mouse_move. But I still don't know to test the use of this new variable, without physically moving the mouse.
Oh, wait. Could test_gui_event call gui_mouse_moved(), which is where the new global is used? With that, should be able to get coverage.
For the changes to map.c::do_map, consider, starting with the simplest.
- After every map command, if MouseMove, turn on flag. Flag is never turned off.
- After every map/unmap command. spin through all the mappings and look for MouseMove (or it' various forms). If there's a flag that says "there's a special in here", it may not be too bad performance wise.
- Given flag, if adding mapping with MouseMove, "flag = true", if removing mapping with MouseMove, check all mappings as above. This should be extrememely reliable.
- Instead of flag, could have a counter, when non-zero there's a MouseMove mapping. This approach feels more susceptible to bugs.
I'd be inclined to go with approach 3, especially since I'm unfamiliar with the map.c::do_map function. But after doing 3, probably be comfortable with doing 4. And 2, 3 are probably implemented and used for internal validation for a little while.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> > How does the plugin get the mouse move events? Depending on that we
> > could decide to have a way to enable the events, like the
> > "popup_uses_mouse_move" flag.
>
> With only the change to vim in this PR, #10044, the plugin is doing
> ```
> nnoremap <special> <MouseMove> <ScriptCmd>Move()<CR>
> ```
> rollover_ is implemented and works.
I see. You mention detecting that a mapping uses MouseMove, but that is
not so easy.
Although the event is fairly lightweight, it might still cause trouble.
We recently had an issue that moving the mouse would abort half a
mapping. And since you do want to map the key, that is going to
interfere more.
I think adding an option is the only acceptable way. Like there already
are the 'ballooneval' and 'balloonevalterm' options. A 'mousemoveevent'
option I suppose.
I do not see a good reason to generate the normal mouse move event when
still on the same character. So how about combining the code, so we
don't need the 0xdeadbeef button value.
> > New things should be tested. test_gui_event() can perhaps be extended
> > for this.
>
> Something like `mapping_uses_mouse_move` could be set/reset in
> map.c::do_map. With that, then there's be something to test, I could
> modify `test_gui_event` to query the value of
> this new variable, without physically moving the mouse.
>
> where the new global is used? With that, should be able to get
> coverage.
moved, which is 0x700, I would think it just works.
--
ARTHUR: Ni!
BEDEVERE: Nu!
ARTHUR: No. Ni! More like this. "Ni"!
BEDEVERE: Ni, ni, ni!
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
Although the event is fairly lightweight, it might still cause trouble. We recently had an issue that moving the mouse would abort half a mapping. And since you do want to map the key,
Unless there's a reason to invent a new mechanism. One option seems plenty.
that is going to interfere more.
Only sending one event per character area should greatly reduce potential interference in any case for this and for the popup usage.
I'll take a look at those changes for the popup problem, if I can find them, to satisfy my curiosity and see if there's anything I can glean. I guess changes around popup_uses_mouse_move and whatever that previous name for the variable was.
I think adding an option is the only acceptable way. Like there already are the 'ballooneval' and 'balloonevalterm' options. A 'mousemoveevent' option I suppose.
OK, boolean option. Where to doc? Of course in options.txt. Somewhere else?
Ah, found it: gui-mouse-mapping in gui.txt. A one line reference to see 'mousemoveevent'.
I do not see a good reason to generate the normal mouse move event when still on the same character. So how about combining the code, so we don't need the 0xdeadbeef button value.
And I thought there was a reason to blast mouse move events for popup dismissal.
The "button" value isn't checked, thus if you pass the value for mouse moved, which is 0x700, I would think it just works.
Plan: in test_gui_event() add move: as an {args} item for mouse. With move: true, row/col are interpreted as pixels and the rest of the arguments are ignored and call gui_mouse_move() instead of gui_send_mouse_event(). Then the impact of set/clear 'mousemoveevent' can be tested.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
@errael pushed 1 commit.
- 487186b Mouse move events per char move
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
Updated the PR; could be final code changes. If looks OK, I'll push a commit with doc updates.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
Modify testing.g::test_gui_mouse_event so that it only fails if required arguments are missing. MouseMov only needs row/col.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #10044 (0f4a53d) into master (ccbfd48) will decrease coverage by
79.91%.
The diff coverage is0.00%.
@@ Coverage Diff @@ ## master #10044 +/- ## =========================================== - Coverage 81.92% 2.01% -79.92% =========================================== Files 167 152 -15 Lines 187527 170543 -16984 Branches 42290 39458 -2832 =========================================== - Hits 153634 3428 -150206 - Misses 21519 166422 +144903 + Partials 12374 693 -11681
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | ? |
|
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 2.01% <0.00%> (-0.01%) |
⬇️ |
| linux | 2.01% <0.00%> (-81.92%) |
⬇️ |
| mingw-x64-HUGE | ? |
|
| mingw-x64-HUGE-gui | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/gui.c | 0.19% <0.00%> (-73.71%) |
⬇️ |
| src/testing.c | 0.00% <0.00%> (-89.24%) |
⬇️ |
| src/libvterm/src/rect.h | 0.00% <0.00%> (-96.78%) |
⬇️ |
| src/sha256.c | 0.00% <0.00%> (-96.45%) |
⬇️ |
| src/libvterm/src/parser.c | 0.00% <0.00%> (-95.91%) |
⬇️ |
| src/gui_gtk_f.c | 0.00% <0.00%> (-94.72%) |
⬇️ |
| src/crypt_zip.c | 0.00% <0.00%> (-94.12%) |
⬇️ |
| src/vim9compile.c | 0.00% <0.00%> (-92.85%) |
⬇️ |
| src/debugger.c | 0.00% <0.00%> (-92.23%) |
⬇️ |
| src/eval.c | 0.10% <0.00%> (-92.22%) |
⬇️ |
| ... and 155 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update ccbfd48...0f4a53d. Read the comment docs.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
Ready for review.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.