From 8fda035132c7734c228c420020e2b01d435820e5 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Sun, 2 Oct 2022 00:17:12 -0700 Subject: [PATCH 1/2] Fix bug where clipboard would be polluted when showing Services menu Previously, whenever the user selects some text in MacVim, and then bring up the Services menu (either by right-clicking on the selection, or go to MacVim -> Services), MacVim will copy the selected text to the system clipboard, which is quite an unexpected behavior. Fix that here. Part of the issue is that Vim has built-in ways to convert the selected range to a single copyable text, while managing complexities of dealing with blockwise/linewise modes. Previous implementation in MacVim was lazy in that it just invoked those code, but the side effect was that it would also copy the text to the system clipboard and pollute Vim's star register as well. This was quite undesirable because the user has not done anything other than opening a menu and wouldn't have expected the system clipboard or the Vim registers to have changed. In this fix, we unfortunately had to do a little bit more copy-pasting to extract the useful bits so we can copy the text to a register (but not system clipboard), invoke the code to convert the register to clipboard-happy text, restore the register, and then move on. A little annoying but better than having the unintuitive / annoying behaveior from before, and this way we don't have to do too much intrusive refactoring of upstream Vim code as well. --- src/MacVim/MMBackend.m | 115 +++++++++++++++++++++++++++----- src/MacVim/MMWindowController.m | 27 +++++--- src/MacVim/MacVim.h | 2 +- src/clipboard.c | 14 +++- 4 files changed, 133 insertions(+), 25 deletions(-) diff --git a/src/MacVim/MMBackend.m b/src/MacVim/MMBackend.m index 3c3dcaddd6..8e503caac2 100644 --- a/src/MacVim/MMBackend.m +++ b/src/MacVim/MMBackend.m @@ -1357,26 +1357,111 @@ - (NSString *)evaluateExpression:(in bycopy NSString *)expr return eval; } -- (BOOL)starRegisterToPasteboard:(byref NSPasteboard *)pboard +/// Extracts the text currently selected in visual mode, and return them in str/len. +/// +/// @return the motion type (e.g. blockwise), or -1 for failure. +static int extractSelectedText(char_u **str, long_u *len) +{ + // Note: Most of the functionality in Vim that allows for extracting useful + // text from a selection are in the register & clipboard utility functions. + // Unfortunately, most of those functions would actually send the text to + // the system clipboard, which we don't want (since we just want to extract + // the text instead of polluting the system clipboard). We don't want to + // refactor upstream Vim code too much to avoid merge pains later, so we + // duplicate a fair bit of the code from the functions below. + + if (!(VIsual_active && (State & MODE_NORMAL))) { + // This only works when we are in visual mode and have stuff to select. + return -1; + } + + // Step 1: Find a register to yank the selection to. If we don't do this we + // have to duplicate a lot of code in op_yank(). We save it off to a backup + // first so we can restore it later to avoid polluting the registers. + + // Just use the yank / '0' register as it makes sense, but it doesn't + // really matter. + yankreg_T *target_reg = get_y_register(0); + + // Move the contents to the backup without doing memory allocs. + yankreg_T backup_reg = *target_reg; + target_reg->y_array = NULL; + target_reg->y_size = 0; + + // Step 2: Preserve the local states, and then invoke yank. + // Note: These were copied from clip_get_selection() in clipboard.c + yankreg_T *old_y_previous, *old_y_current; + pos_T old_cursor; + pos_T old_visual; + int old_visual_mode; + colnr_T old_curswant; + int old_set_curswant; + pos_T old_op_start, old_op_end; + oparg_T oa; + cmdarg_T ca; + + // Avoid triggering autocmds such as TextYankPost. + block_autocmds(); + + // Yank the selected text into the target register. + old_y_previous = get_y_previous(); + old_y_current = get_y_current(); + old_cursor = curwin->w_cursor; + old_curswant = curwin->w_curswant; + old_set_curswant = curwin->w_set_curswant; + old_op_start = curbuf->b_op_start; + old_op_end = curbuf->b_op_end; + old_visual = VIsual; + old_visual_mode = VIsual_mode; + clear_oparg(&oa); + oa.regname = '0'; // Use the '0' (yank) register. We will restore it later to avoid pollution. + oa.op_type = OP_YANK; + CLEAR_FIELD(ca); + ca.oap = &oa; + ca.cmdchar = 'y'; + ca.count1 = 1; + ca.retval = CA_NO_ADJ_OP_END; + do_pending_operator(&ca, 0, TRUE); + + // Step 3: Convert the yank register to a single piece of useful text. This + // will handle all the edge cases of different modes (e.g. blockwise, etc). + const int convert_result = clip_convert_selection(str, len, NULL); + + // Step 4: Clean up the yank register, and restore it back. + set_y_current(target_reg); // should not be necessary as it's done in do_pending_operator above (since regname was set to 0), but just to be safe and verbose in intention. + free_yank_all(); + *target_reg = backup_reg; + + // Step 5: Restore all the loose states that were modified during yank. + // Note: These were copied from clip_get_selection() in clipboard.c + set_y_previous(old_y_previous); + set_y_current(old_y_current); + curwin->w_cursor = old_cursor; + changed_cline_bef_curs(); // need to update w_virtcol et al + curwin->w_curswant = old_curswant; + curwin->w_set_curswant = old_set_curswant; + curbuf->b_op_start = old_op_start; + curbuf->b_op_end = old_op_end; + VIsual = old_visual; + VIsual_mode = old_visual_mode; + + unblock_autocmds(); + + return convert_result; +} + +/// Extract the currently selected text (in visual mode) and send that to the +/// provided pasteboard. +- (BOOL)selectedTextToPasteboard:(byref NSPasteboard *)pboard { - // TODO: This method should share code with clip_mch_request_selection(). - - if (VIsual_active && (State & MODE_NORMAL) && clip_star.available) { - // If there is no pasteboard, return YES to indicate that there is text - // to copy. + if (VIsual_active && (State & MODE_NORMAL)) { + // If there is no pasteboard, just return YES to indicate that there is + // text to copy. if (!pboard) return YES; - // The code below used to be clip_copy_selection() but it is now - // static, so do it manually. - clip_update_selection(&clip_star); - clip_free_selection(&clip_star); - clip_get_selection(&clip_star); - clip_gen_set_selection(&clip_star); - - // Get the text to put on the pasteboard. long_u llen = 0; char_u *str = 0; - int type = clip_convert_selection(&str, &llen, &clip_star); + int type = extractSelectedText(&str, &llen); if (type < 0) return NO; diff --git a/src/MacVim/MMWindowController.m b/src/MacVim/MMWindowController.m index 0ba41998e8..298fe8dd9e 100644 --- a/src/MacVim/MMWindowController.m +++ b/src/MacVim/MMWindowController.m @@ -87,7 +87,7 @@ - (void)resizeWindowToFitContentSize:(NSSize)contentSize - (NSSize)constrainContentSizeToScreenSize:(NSSize)contentSize; - (NSRect)constrainFrame:(NSRect)frame; - (NSTabViewItem *)addNewTabViewItem; -- (BOOL)askBackendForStarRegister:(NSPasteboard *)pb; +- (BOOL)askBackendForSelectedText:(NSPasteboard *)pb; - (void)updateTablineSeparator; - (void)hideTablineSeparator:(BOOL)hide; - (void)doFindNext:(BOOL)next; @@ -1336,21 +1336,25 @@ - (id)validRequestorForSendType:(NSString *)sendType returnType:(NSString *)returnType { if ([sendType isEqual:NSStringPboardType] - && [self askBackendForStarRegister:nil]) + && [self askBackendForSelectedText:nil]) return self; return [super validRequestorForSendType:sendType returnType:returnType]; } +/// Called by OS when it tries to show a "Services" menu. We ask Vim for the +/// currently selected text and write that to the provided pasteboard. - (BOOL)writeSelectionToPasteboard:(NSPasteboard *)pboard types:(NSArray *)types { if (![types containsObject:NSStringPboardType]) return NO; - return [self askBackendForStarRegister:pboard]; + return [self askBackendForSelectedText:pboard]; } +/// Called by the OS when it tries to update the selection. This could happen +/// if you selected "Convert text to full width" in the Services menu, for example. - (BOOL)readSelectionFromPasteboard:(NSPasteboard *)pboard { // Replace the current selection with the text on the pasteboard. @@ -1678,18 +1682,25 @@ - (NSTabViewItem *)addNewTabViewItem return [vimView addNewTabViewItem]; } -- (BOOL)askBackendForStarRegister:(NSPasteboard *)pb -{ - // TODO: Can this be done with evaluateExpression: instead? +/// Ask Vim to fill in the pasteboard with the currently selected text in visual mode. +- (BOOL)askBackendForSelectedText:(NSPasteboard *)pb +{ + // We use a dedicated API for this instead of just using something like + // evaluateExpression because there's a fair bit of logic in Vim that + // figures out how to convert from a visual selection to an externally + // presentable text in the clipboard code. Part of the complexity has to do + // with the three modes (character/blockwise/linewise) that visual mode can + // be in. We would like to reuse that code, instead of hacking it via some + // expression evaluation results. BOOL reply = NO; id backendProxy = [vimController backendProxy]; if (backendProxy) { @try { - reply = [backendProxy starRegisterToPasteboard:pb]; + reply = [backendProxy selectedTextToPasteboard:pb]; } @catch (NSException *ex) { - ASLogDebug(@"starRegisterToPasteboard: failed: pid=%d reason=%@", + ASLogDebug(@"selectedTextToPasteboard: failed: pid=%d reason=%@", [vimController pid], ex); } } diff --git a/src/MacVim/MacVim.h b/src/MacVim/MacVim.h index 7d0e70ca77..dbc37577cf 100644 --- a/src/MacVim/MacVim.h +++ b/src/MacVim/MacVim.h @@ -118,7 +118,7 @@ - (NSString *)evaluateExpression:(in bycopy NSString *)expr; - (id)evaluateExpressionCocoa:(in bycopy NSString *)expr errorString:(out bycopy NSString **)errstr; -- (BOOL)starRegisterToPasteboard:(byref NSPasteboard *)pboard; +- (BOOL)selectedTextToPasteboard:(byref NSPasteboard *)pboard; - (oneway void)acknowledgeConnection; @end diff --git a/src/clipboard.c b/src/clipboard.c index 5e97a5fc64..78abdeddda 100644 --- a/src/clipboard.c +++ b/src/clipboard.c @@ -2114,7 +2114,19 @@ clip_convert_selection(char_u **str, long_u *len, Clipboard_T *cbd) int_u eolsize; yankreg_T *y_ptr; - if (cbd == &clip_plus) + if (!cbd) + { + // MacVim extension: This makes this function much more useful as we + // can now extract usable texts from any registers for use instead of + // being forced to go through the system clipboard. This is useful for + // features that expose selected texts (e.g. system services) without + // polluting the system clipboard. + int unname_register = get_unname_register(); + if (unname_register < 0) + return -1; + y_ptr = get_y_register(unname_register); + } + else if (cbd == &clip_plus) y_ptr = get_y_register(PLUS_REGISTER); else y_ptr = get_y_register(STAR_REGISTER); From f45f9394dd4dfbf802ee028b1efc8b82d32a3ef1 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Sun, 2 Oct 2022 01:17:31 -0700 Subject: [PATCH 2/2] Fix code to use getreg() instead I just missed the fact that getreg() also works and I don't need to hack to use clip_convert_selection() and the mod on it. This is kind of... odd, because clip_convert_selection() is essentially doing the same thing as getreg() with *very* minor differences. You would hope that would be consolidated and refactored into a single function, but of course not. Also cleaned up some comments to make it clear that evaluateExpression probably would have worked except for the autocmd blocking part. --- src/MacVim/MMBackend.m | 27 +++++++++++---------------- src/MacVim/MMWindowController.m | 12 +++++------- src/clipboard.c | 14 +------------- 3 files changed, 17 insertions(+), 36 deletions(-) diff --git a/src/MacVim/MMBackend.m b/src/MacVim/MMBackend.m index 8e503caac2..6224822dbc 100644 --- a/src/MacVim/MMBackend.m +++ b/src/MacVim/MMBackend.m @@ -1357,10 +1357,10 @@ - (NSString *)evaluateExpression:(in bycopy NSString *)expr return eval; } -/// Extracts the text currently selected in visual mode, and return them in str/len. +/// Extracts the text currently selected in visual mode, and returns it. /// -/// @return the motion type (e.g. blockwise), or -1 for failure. -static int extractSelectedText(char_u **str, long_u *len) +/// @return the string representing the selected text, or NULL if failure. +static char_u *extractSelectedText() { // Note: Most of the functionality in Vim that allows for extracting useful // text from a selection are in the register & clipboard utility functions. @@ -1372,7 +1372,7 @@ static int extractSelectedText(char_u **str, long_u *len) if (!(VIsual_active && (State & MODE_NORMAL))) { // This only works when we are in visual mode and have stuff to select. - return -1; + return NULL; } // Step 1: Find a register to yank the selection to. If we don't do this we @@ -1423,9 +1423,8 @@ static int extractSelectedText(char_u **str, long_u *len) ca.retval = CA_NO_ADJ_OP_END; do_pending_operator(&ca, 0, TRUE); - // Step 3: Convert the yank register to a single piece of useful text. This - // will handle all the edge cases of different modes (e.g. blockwise, etc). - const int convert_result = clip_convert_selection(str, len, NULL); + // Step 3: Extract the text from the yank ('0') register. + char_u *str = get_reg_contents(0, 0); // Step 4: Clean up the yank register, and restore it back. set_y_current(target_reg); // should not be necessary as it's done in do_pending_operator above (since regname was set to 0), but just to be safe and verbose in intention. @@ -1447,7 +1446,7 @@ static int extractSelectedText(char_u **str, long_u *len) unblock_autocmds(); - return convert_result; + return str; } /// Extract the currently selected text (in visual mode) and send that to the @@ -1460,23 +1459,19 @@ - (BOOL)selectedTextToPasteboard:(byref NSPasteboard *)pboard if (!pboard) return YES; - long_u llen = 0; char_u *str = 0; - int type = extractSelectedText(&str, &llen); - if (type < 0) + char_u *str = extractSelectedText(); + if (!str) return NO; - // TODO: Avoid overflow. - int len = (int)llen; if (output_conv.vc_type != CONV_NONE) { - char_u *conv_str = string_convert(&output_conv, str, &len); + char_u *conv_str = string_convert(&output_conv, str, NULL); if (conv_str) { vim_free(str); str = conv_str; } } - NSString *string = [[NSString alloc] - initWithBytes:str length:len encoding:NSUTF8StringEncoding]; + NSString *string = [[NSString alloc] initWithUTF8String:(char*)str]; NSArray *types = [NSArray arrayWithObject:NSStringPboardType]; [pboard declareTypes:types owner:nil]; diff --git a/src/MacVim/MMWindowController.m b/src/MacVim/MMWindowController.m index 298fe8dd9e..571b2770eb 100644 --- a/src/MacVim/MMWindowController.m +++ b/src/MacVim/MMWindowController.m @@ -1685,13 +1685,11 @@ - (NSTabViewItem *)addNewTabViewItem /// Ask Vim to fill in the pasteboard with the currently selected text in visual mode. - (BOOL)askBackendForSelectedText:(NSPasteboard *)pb { - // We use a dedicated API for this instead of just using something like - // evaluateExpression because there's a fair bit of logic in Vim that - // figures out how to convert from a visual selection to an externally - // presentable text in the clipboard code. Part of the complexity has to do - // with the three modes (character/blockwise/linewise) that visual mode can - // be in. We would like to reuse that code, instead of hacking it via some - // expression evaluation results. + // This could potentially be done via evaluateExpression by yanking the + // selection, then returning the results via getreg('@') and restoring the + // register. Using a dedicated API is probably a little safer (e.g. it + // prevents TextYankPost autocmd's from triggering) and efficient + // and hence this is what we use for now. BOOL reply = NO; id backendProxy = [vimController backendProxy]; diff --git a/src/clipboard.c b/src/clipboard.c index 78abdeddda..5e97a5fc64 100644 --- a/src/clipboard.c +++ b/src/clipboard.c @@ -2114,19 +2114,7 @@ clip_convert_selection(char_u **str, long_u *len, Clipboard_T *cbd) int_u eolsize; yankreg_T *y_ptr; - if (!cbd) - { - // MacVim extension: This makes this function much more useful as we - // can now extract usable texts from any registers for use instead of - // being forced to go through the system clipboard. This is useful for - // features that expose selected texts (e.g. system services) without - // polluting the system clipboard. - int unname_register = get_unname_register(); - if (unname_register < 0) - return -1; - y_ptr = get_y_register(unname_register); - } - else if (cbd == &clip_plus) + if (cbd == &clip_plus) y_ptr = get_y_register(PLUS_REGISTER); else y_ptr = get_y_register(STAR_REGISTER);