Improper pair priorization in multiple choice scenario

[jemag]

Checklist

• [X] Have you updated the plugin to the latest version on main branch?
• [X] Have you checked the Breaking Changes issue?
• [X] Have you read through :h nvim-surround to see if there might be any relevant information there?

To reproduce

minimal init.lua:

```lua local on_windows = vim.loop.os_uname().version:match("Windows") -- vim.api.nvim_command("!rm -rf /tmp/nvim") local function join_paths(...) local path_sep = on_windows and "\\" or "/" local result = table.concat({ ... }, path_sep) return result end vim.cmd([[set runtimepath=$VIMRUNTIME]]) local temp_dir = vim.loop.os_getenv("TEMP") or "/tmp" vim.cmd("set packpath=" .. join_paths(temp_dir, "nvim", "site")) local package_root = join_paths(temp_dir, "nvim", "site", "pack") local install_path = join_paths(package_root, "packer", "start", "packer.nvim") local compile_path = join_paths(install_path, "plugin", "packer_compiled.lua") local function load_plugins() vim.cmd([[ syntax off]]) vim.g.ts_highlight_lua = true require("packer").startup({ { "wbthomason/packer.nvim", "nvim-treesitter/nvim-treesitter", "kylechui/nvim-surround", }, config = { package_root = package_root, compile_path = compile_path, }, }) end _G.load_config = function() require("nvim-surround").setup({ keymaps = { -- vim-surround style keymaps insert = "ys", insert_line = "yss", visual = "S", delete = "ds", change = "cs", }, highlight = { -- Highlight text-objects before surrounding them duration = 0, }, }) end if vim.fn.isdirectory(install_path) == 0 then vim.fn.system({ "git", "clone", "https://github.com/wbthomason/packer.nvim", install_path }) load_plugins() require("packer").sync() else load_plugins() require("packer").sync() _G.load_config() end ```

Steps:

• Run minimal init.lua and open following text file using nvim -u minimal_init.lua test.txt

  "<cmd>lua require("telescope.builtin").command_history({sorter = require("telescope.sorters").get_substr_matcher()})<cr>",

• Go to this position:
• Hit cs"' to change double quotes to single quotes
• Notice it targets pair of double quotes to the right, instead of the double quotes surrounding the current letter

Expected behavior

Should prioritize pair surrounding the current position.

Actual behavior

Targets following pair

Additional context

No response

[kylechui]

Ok so this seems to be in conflict with #39; the original idea was that I would try to force the plugin to always change between pairs of quotes, e.g. cs"' with the cursor on cursor in

"hello" cursor "world"

would change the second pair of quotes. This is the exact same as in your example, since you nested everything in an outer set of double quotes. I hate to be the one to say "user error", but if I were you I would change the outermost pair of quotes to single quotes, or use [[ ... ]] to represent a string.

[jemag]

Obviously this is just a random example, perhaps not the best one, but still a valid workflow.

I agree that it is not optimal in your hello world example, although I guess for me the main goal is always the surrounding pair, and being able to execute pairs with your cursor outside is just a nice bonus.

What I like with the surrounding pair prioritization is that in both scenarios, I can move my cursor to the proper location and apply the intended modification, whereas this is not possible currently.

Either way, I understand that it might not be the preferred workflow for others. I don't know how complicated this would be, but is this something that could be configured perhaps through setup?

[kylechui]

I'll have to think through what the semantics should be for what actually makes the quote search "smart" and how to discern which pair of quotes the user is referring to. I agree with you that removing it would make the plugin easier to understand mentally/code-wise, although a bit inconvenient. I wonder if @williamgoulois has any thoughts on this? TBH selecting the immediate surrounding pair of quotes would also be more consistent with how the default a" and a' text-objects behave.

[kylechui]

Honestly there are quite a few things that are bad/wrong with quotes. The current implementation messes around with the definition of the a' text-object in order to allow for operations like ysa' to exist (as I envision them). I still am unsure of what to do regarding how "smart" the quotes should be.

[williamgoulois]

Sorry for the delay @kylechui I have quite a lot of work atm

Regarding this issue @jemag i agree with you it's a user error for me. You should not use the same level of quotes inside a string and this is not possible in lua. Here is a screenshot of lua code with LSP error from luacheck.

I encountered other bugs @kylechui with the lib and will open issues when I have more time to follow and answer. Here is a sneak peek video. Sometimes (can't figure out yet in what situations), the indentation is messed up when deleting quotes (and maybe other surroundings)

In this video I just do ds' https://user-images.githubusercontent.com/37271970/193628232-7e449f5f-84c0-4631-bb14-fddf3f67ad37.mov

[jemag]

@williamgoulois I obviously know that this is not proper lua, it is just an example of changing double quotes to singles quotes in a multiple choice scenario, and the fact that it is not currently prioritizing the surround pair.

Regarding your comment @kylechui :

I'll have to think through what the semantics should be for what actually makes the quote search "smart" and how to discern which pair of quotes the user is referring to. I agree with you that removing it would make the plugin easier to understand mentally/code-wise, although a bit inconvenient. I wonder if @williamgoulois has any thoughts on this? TBH selecting the immediate surrounding pair of quotes would also be more consistent with how the default a" and a' text-objects behave.

For me, I would tend to favor predictability, since, as a user, I can adapt my workflow to ensure my surround manipulations work 100% of the time, and over time build confidence and speed. However, I realize that this predictability might lower efficiency in some other scenarios, I am just not sure how often users make use of those smart prioritization and how big of a benefit it truly is for them.

[williamgoulois]

@jemag In this case it is not a surrounding pair, as this is not valid syntax for language. Can you show us a valid situation with valid syntax that does not work as intended ? I felt like the solution we currently have is quite predictible (but a little complex i agree)

[jemag]

@williamgoulois that does not make sense. The whole point of this plugin is to be able to change an existing pair to a preferred one. I would not need to change the pair in the first place if the syntax was already valid.

Wanting to go from : to: is a perfectly valid workflow.

Most uses cases of this plugin are probably similar workflows, wanting to go from invalid syntax to a valid one, therefore needing to make a change.

The fact is I cannot accomplish this scenario predictably as of right now.

[kylechui]

I think that's a very reasonable perspective; in any case, I am leaning towards the omission of such a feature for code cleanliness reasons... I'll try to think of some more/better ways to integrate "smart quotes" as something available to the user with a bit of configuration, rather than the default

Edit: For context, the current implementation works via "jumping" to the next quote character, before calling the non-smart version of cs. For some reason the a' text-object always selects a valid pair if the cursor resides on a quote character, and returns the literal surrounding pair otherwise.

[kylechui]

As per #166, it seems that more people are getting confused by this jumping behavior. Furthermore, for reasons that I have illustrated in the other issue, the code that implements this "smart jumping" behavior is very ugly/hard-coded in, and does not extend very well at all to a generic smart_jump flag on a per-surround basis. If nobody is interested in trying to come up with their own implementation of this, I think I will remove this quirk from the codebase, leaving it up to the user to first reposition their cursor between the intended delimiter pair, then modify.

Edit: @williamgoulois Should this breaking change take effect, you should be able to implement your own version of jumps by remapping ds' to a Lua function that first does the normal mode keymap f', followed by my plugin's ds'. It follows for the cs keymap, and for other quote types.

CC: @williamgoulois @jemag @zdcthomas

[kylechui]

Fixed as of 87839e1, see this message.

Thanks to people that are actively giving feedback on this project; it really helps out a lot for getting a sense of what direction I want to be taking the project in. :sparkling_heart:

[treipatru]

thank you for this fix! unfortunately "smart" features are always difficult to get right because "smart" can mean different things to different people in so many programming languages. i frequently edit strings in strings (i.e. single quoted string inside a double quoted one) and this smart feature was always picking the wrong pair of quotes.

thank you for making and maintaining the plugin, overall it works great 🙇

[kylechui]

Thanks for the kind words! As a note to everybody reading this: even if you don't have anything to say, simply leaving a reaction :+1: or :-1: on issues helps out a lot. Up until yesterday I was under the impression that only 1 or 2 people cared/noticed anything about this "feature", and now it seems that many more people are happy with the latest change. Had I known, I probably would have taken action sooner instead of waiting for so long.

[kohane27]

I was surprised to see that this smart-jump, lookahead feature was removed. Given the following,

"hello" I "world"
        ^
      cursor

I'd expect vi" to select world. I guess different people have different preference indeed. I read through this thread and found your recommendation about how to get this "smart lookahead jumping" feature back:

implement your own version of jumps by remapping ds' to a Lua function that first does the normal mode keymap f', followed by my plugin's ds'. It follows for the cs keymap, and for other quote types.

Could you @kylechui please advise how to do so? I read through the docs but still have no idea.

Thank you!

[kylechui]

This should do the trick:

local quote_chars = { "'", '"', "`" }
for _, char in ipairs(quote_chars) do
    vim.keymap.set("n", "ds" .. char, function()
        local curpos = require("nvim-surround.buffer").get_curpos()
        if vim.fn.searchpos(char, "cnW")[1] == curpos[1] then
            vim.fn.searchpos(char, "cW")
        end
        require("nvim-surround").delete_surround({ del_char = char, curpos = curpos })
    end)
end

I haven't really tested it but the gist of it is that you just check if the next quote character is on the same line, and if so, jump to it before calling delete_surround. The unfortunate part is that this exposes a little bit of the implementation to you, which is not officially supported; I reserve the right to change the internal API at any time and potentially break the above code, so use at your own risk!

Edit: Perhaps I need to spend some time thinking about how I would implement this "officially"

[kohane27]

@kylechui Thank you for getting back to me and providing a solution for ds! I tested and it works (I changed from ds to di.) Then I also tried to implement the same "prefer-lookahead-instead-of-between-quotes" logic when conflicting choices occur for ci and vi. I had troubling getting it work.

On the bottom left, it's the change_surround. I'm not familiar with the codebase to implement them now. The codebase is very clean; I need to look into it and implement later. In the mean time, please see my following rationale for this "smart-jump" feature.

Let's say we have the following js snippet:

function testing(arg1, arg2) {
  console.log(arg1, arg2);
}

testing("apple", "orange");

If the cursor is between the method's arguments:

testing("apple", I "orange");
                 ^
               cursor

When I ci", di" or vi", I expect nvim-surround would operate on "orange" like below:

testing("apple", "I");
                  ^
                cursor

But current implementation is that nvim-surround operates on the quote between "apple" and "orange", yielding the following:

testing("apple"I"orange");
               ^
             cursor

The same behaviors go with cs", ds" and vs".

imho, current implementation doesn't make much sense in the above scenario. It has to do with user's intent. Maybe we can use treesitter to parse the syntax tree when dealing with potential conflicting choices? tbh, I tried and nvim-surround's current implementation is the same as vim-sandwich, maybe I'm just too spoiled because when no conflict arises:

I testing("apple");
^
cursor

I simply ci" to change the method's argument, because both nvim-surround and vim-sandwich support this lookahead jumping, so I've come to expecting this jumping ahead behavior even when a conflicting scenario arises.

I've tried using the commit before this "smart" quotes feature was disabled:

  use({
    "kylechui/nvim-surround",
    commit = "df1c68f8fd6252a5657479aab88742f2f5f2c6b8",
  })

But it still has the same behaviors as the current latest version. Not sure if it's my problem with the understanding of "smart" quote...

Regardless, thank you again for this lua version of surround!

[kylechui]

@kohane27 Some answers to your questions/summarizing the above thread:

On the bottom left, it's the change_surround. I'm not familiar with the codebase to implement them now.

1. Look at the function type annotations in init.lua and annotations.lua, it should give you an idea for what parameters to pass into the function imho, current implementation doesn't make much sense in the above scenario. It has to do with user's intent.
2. Consider the string "He said "that was Ia mistake" to me", where the cursor is on the I. Upon input cs"', it's ambiguous whether or not the user intends to change the latter pair of quotes, or if they mean to change the middle pair to a different set of quotes (I would say that the "intent" is the latter). For every example where jumping is "sensible", there exists some example where jumping is not what I would consider to be "user intent". A few more reasons for not re-implementing this: a. There is a non-trivial amount of extra code to reason about and maintain. Why should nvim-surround jump for quotes but not for other characters? IMO (and it seems many other people concur), it makes more sense to have the following heuristic: Immediate surrounding pair, before surround after the cursor, before surround before the cursor. This reduces "edge cases" and situations where I would have to hard-code something into the plugin. b. There is no way to select the immediate surrounding pair if you enable "smart targeting", but vice versa you can just move the cursor first (non-ideal, but at least possible). c. Implementation-wise there is also no "good" way to discern user intent here. Tree-sitter is fallible because nodes may fail or error out due to incorrect syntax, there may be an odd number of quotes on a line, etc. Pattern-matching is susceptible to errors when escaped quote characters are introduced, etc. Even if all edge cases are handled, the semantic example that I provided has no solution (to my knowledge).

Even if there was some magical way to "just have it work" (which to my knowledge doesn't exist), I would then have to consider whether or not the hard-coding of various edge cases would be reasonable to maintain in the long-term. I hope this sheds a bit of light as for why this "feature" was deprecated to begin with.

[bew]

What would you think about adding the concept of next, as in in" for in next dquote ?

[kylechui]

What would you think about adding the concept of next, as in in" for in next dquote ?

@bew Probably better suited for a standalone textobjects/motions plugin, i.e. targets.vim or mini.ai