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

Add Cut/Copy/Paste Context Menu #539

Merged
merged 4 commits into from
May 15, 2019
Merged

Conversation

jgehrig
Copy link
Collaborator

@jgehrig jgehrig commented Apr 28, 2019

I would like to see support in neovim-qt for a basic context menu. It is a feature supported by other vim front-ends (gVim, neovim-gtk, etc) and it makes the occasional mouse operation quicker/easier.

I have extended MainWindow to include fixed QActions for all context menu entries. I did change the mouse handling routine in shell.cpp, so right click events will no longer be passed to NeoVim. Click events occur on right mouse button release. Each time an event is detected by shell.cpp, an new QMenu is painted at the cursor position.

Any feedback you have as to how this could be better-implemented or modularized is welcome :)

This change modifies MainWindow to add Cut/Copy/Paste/SelectAll events. All of
these events are appended to a context menu which is triggered on right-click.

The function Shell::neovimMouseEvent(...) is modified to trigger right click
events on right mouse button release. Some of these event may no longer be
passed along to the NeoVim backend.
@justinmk
Copy link
Contributor

Somewhat related, it was discussed somewhere on this tracker before that users might want to click a selection from the Nvim built-in popupmenu (e.g. ctrl-n during insert-mode). To support that, Neovim now has nvim_select_popupmenu_item API function.

@equalsraf
Copy link
Owner

Hi @jgehrig, thanks for working on this

I like the idea but there are a couple of points that need to be handled before this can be merged:

[1.] As is, this prevents a rightclick from being forwarded to nvim, which might not be what the user wants. He may actually have mappings that use a right click.

Instead I would prefer to allow neovim to continue handling configuration and input and trigger the context menu, i.e.

  1. user right clicks and the event reaches nvim as <RightMouse>
  2. in nvim a mapping notifies the GUI using rpcnotify (here is an example for GuiWindowMaximized (vimscript + C++)
  3. The GUI calls showMenu or exec like you are doing right now.

[2.] QMenu::exec() is modal and blocks all other input, not sure if this was intentional and i dont know how it plays out with resizes

[3.] vim_feedkeys() mode is now m. Should this not be n and perhaps x. Also what happens if the user is not in normal mode?

[4.] Nitpick on my end, but I dont think you need to rebuild the QMenu every time you show it.

@jgehrig
Copy link
Collaborator Author

jgehrig commented May 1, 2019

Thanks, everything sounds reasonable to me! 👍

[1.] Thanks for the example.
I will post an iteration with noremap <RightMouse> ... in nvim_gui_shim.vim.

[2.] No, making the menu blocking was not exactly a deliberate decision. However, the menu behavior is consistent with how other applications work (gVim for example).

Is there a specific scenario which concerns you or behavior you are looking for?

[3.] Yes, it looks like vim_feedkeys is not the correct API to call.

[4.] Yes, I thought the same thing. Easy modification.

@equalsraf
Copy link
Owner

Is there a specific scenario which concerns you or behavior you are looking for?

Window resize was the first thing that came to mind. Because the UI will not notify nvim of the resize, you should see the window with a margin or a cropped window.

For an asynchronous menu see the popupmenu used for completion (m_pum).

In both cases the tricky part is how to make clear to the user how to interact with this menu, e.g. in popupmenu, keyboard input never goes through the menu widget, but that makes more sense there than for this case because neovim handles the input for that.

Now for an historical side note :), in vim this could be done using the popup command.

In fact you can test this in gvim using :popup File which is in fact modal (at least in my Linux box). Any mouse click is handled by the menu, and keyboard input is handled by the menu. That is probably a good reference for testing this.

The popup menu is not available in nvim. But the menu commands are (including the PopUp menu). I think there was another nvim GUI that fully supported these menus. But I cannot remember which. Found it

@jgehrig
Copy link
Collaborator Author

jgehrig commented May 1, 2019

Window resize was the first thing that came to mind. Because the UI will not notify nvim of the resize, you should see the window with a margin or a cropped window.

I don't think that is an issue for this change? Display of the context menu blocks other events, so the user is unable to resize until the menu is closed. Qt directs keyboard events to the context menu, so behavior should be as-expected in that area as well.

From the user's perspective, the behavior is the same as other context menus I see (vim and elsewhere). For example Chrome's right-click menu blocks window resize and keyboard events from happening as long as the menu is visible.

Now for an historical side note :)

Cool! I didn't know vim supported this... I will play around with the feature, and see if anything can be incorporated into this change.

Previously Qt was allowed to take control of right-click events in order to
display the context menu. This change reverts that behavior, and has mouse
events handled by NeoVim again.

The context menu is now bound to VimScript function :GuiShowContextMenu. A
rpcnotify event has been added to the nvim_gui_shim.vim on '<RightMouse>'
events. In Normal/Insert modes, the command can be called. However, in Visual
mode text may be selected and a call to :GuiShowContextMenu will wipe out any
user selections. To work the selection wipe-out after calls to
:GuiShowContextMenu, gv is called to re-select any user selections.
@jgehrig
Copy link
Collaborator Author

jgehrig commented May 3, 2019

Commit 1ec3b4b added to address your feedback.

[1.] NeoVim now handles <RightMouse> events. I have done some quick testing... I will use it day-to-day to see if I notice any issues or odd behavior. I tested adding a custom <RightMouse> map, this should not override any user maps.

It should be noted that :GuiShowContextMenu wipes any selections made in visual mode. To work around this I did mode-based mapping. There is "vg" command at the end of vnoreamp. Technically this wipes the user selection, then re-selects it. I am unaware of a better way to write this with VimScript... Let me know if you can think of a cleaner way to write this.

[2.] When the input is blocked by Qt to handle QMenu (commit 1), ::show() works fine. With NeoVim handling the events (commit 2) we need ::popup(). Changed to async.

[3.] I changed the NeoVim API calls to send the command normal! .... I tested with feedkeys(..., 'nx', true) too, that would also work.

[4.] Tested, seems to work fine.

endfunction

command! -range GuiShowContextMenu call s:GuiShowContextMenu()
nnoremap <silent><RightMouse> :GuiShowContextMenu<CR>
Copy link
Owner

Choose a reason for hiding this comment

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

Can we keep these disabled by default, for now it is enough to move these examples to the docs/README.

... which reminds me, the docs :D https://github.com/equalsraf/neovim-qt/blob/master/src/gui/runtime/doc/nvim_gui_shim.txt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! That makes sense to me :)

I can get rid of the command! GuiShowContextMenu, and add a brief summary to the documentation.

I don't see a docs/README, so I assume the example should be added to nvim_gui_shim.txt?

							*GuiShowContextMenu()*
Displays a Cut-Copy-Paste context menu at the current cursor position. This
event can be bound to right click events in ginit.vim, e.g.
>
	nnoremap <silent><RightMouse> :call GuiShowContextMenu()<CR>
	inoremap <silent><RightMouse> <Esc>:call GuiShowContextMenu()<CR>
	vnoremap <silent><RightMouse> :call GuiShowContextMenu()<CR>gv

@equalsraf
Copy link
Owner

Looks good, the failing test is because of a call to QAction, maybe this was added in a newer version of Qt.

/home/runner/neovim-qt/src/gui/mainwindow.cpp:54:69: error: no matching function for call
to ‘QAction::QAction(QIcon, QString)’
  m_actCut = new QAction(QIcon::fromTheme("edit-cut"), QString("Cut"));

@jgehrig
Copy link
Collaborator Author

jgehrig commented May 14, 2019

Ahhh I see what is going on... I missed the -DCMAKE_PREFIX_PATH=/opt/qt54/ in the failing test.

It looks like for >=Qt 5.7, parent became an optional argument. The following should resolve the issue:
m_actCut = new QAction(QIcon::fromTheme("edit-cut"), QString("Cut"), nullptr /*parent*/);

jgehrig added 2 commits May 14, 2019 18:50
Resolve semaphoreci CI build failure.

Starting with Qt-5.7, the parent parameter of QAction's constructor is
optional. However, to continue support for nvim-qt compile against older
versions of Qt, this parameter must be provided anyways.
Users can opt-in to a right-click menu in their ginit.vim file.

Normal Mode:
nnoremap <silent><RightMouse> :call GuiShowContextMenu()<CR>

Insert Mode:
inoremap <silent><RightMouse> <Esc>:call GuiShowContextMenu()<CR>
" <Esc> or <C-O> is required, so the command runs in Normal Mode.
" <Esc> will exit, and <C-O> will remain in Insert Mode.

Visual Modea;
vnoremap <silent><RightMouse> :call GuiShowContextMenu()<CR>gv
" gv causes the selection to persist after the event. The call to
" GuiShowContextMenu wipes the selection, and gv restores it.
@equalsraf equalsraf merged commit 663f2b7 into equalsraf:master May 15, 2019
@equalsraf
Copy link
Owner

Very cool thanks @jgehrig

@jgehrig jgehrig deleted the jg-contextmenu branch August 8, 2019 20:45
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.

3 participants