Only extend the completion range by 2 if there are closing brackets already present

[viktomas]

This PR started as a fix for https://github.com/Feel-ix-343/markdown-oxide/issues/278

But in the end, it's fixing a bug when completion replaces characters after the completed link

https://github.com/user-attachments/assets/4081b35c-4783-4d5b-bc3b-7dbc81a2ab74

The full understanding of the bug and relevant context starts with comment https://github.com/Feel-ix-343/markdown-oxide/pull/279#issuecomment-3191471763

Original (out-of-date) description

https://github.com/user-attachments/assets/98e15e1b-b1be-4e22-ba15-1394ad247116

Full disclaimer, this PR is 🤖 AI-generated 🤖 . The code doesn't look clean and it can be probably simplified, I'm not familiar with rust development or LS libraries in rust so I'm not the right person to do this.

Feel free to close the MR if the fix is a one-liner.

---

The fix works for me, when I use this MR version, I don't get duplicated closing brackets.

Testing

This PR can be tested using the script from https://github.com/Feel-ix-343/markdown-oxide/issues/278, only uncomment the cmd = {vim.fn.expand('~/path/to/markdown-oxide')}, line and point the config to your binary.

[viktomas]

@yutanagano @ellisra pinging you, since you faced this in the past

[Feel-ix-343]

Hey! Thank you;

[Feel-ix-343]

In this case, closing block completions are handled by replacing the full span of opening completions and closing completions if present.

I think what's happening is that the LSP client gets confused sometimes. Most of the time this should work though.

First, I'd recommend that you reproduce the issue then test out (or have claude test out) various solutions

[viktomas]

First, I'd recommend that you reproduce the issue then test out (or have claude test out) various solutions

@Feel-ix-343 thank you for having a look 🙇

do you mean reproducing the issue beyond https://github.com/Feel-ix-343/markdown-oxide/issues/278

In this case, closing block completions are handled by replacing the full span of opening completions and closing completions if present.

I think what's happening is that the LSP client gets confused sometimes. Most of the time this should work though.

Do you mean some mechanism is already taking care of de-duplicating the closing brackets and my reproduction scenario is misconfigured? I believe the server should be responsible for knowing the semantics (i.e. when to add and not add the closing brackets) and looking at the line 419 in the old diff the server always appends the ]] string.

[yutanagano]

@yutanagano @ellisra pinging you, since you faced this in the past

always appends the ]] string.

I have not tested this new branch nor have I read the code but I can say from my end that the issue has since resolved (on the current main) and I do not experience the issues documented in the root issue #278. So this PR might not be necessary?

[viktomas]

@Feel-ix-343 I spent some time debugging this more. I know what was happening

• completion mini ignores the ranges and only uses insert text
• we were incrementing the end character position in the editText completion range always by 2
• completion mini ignored the increased range, only used insert text and duplicated the closing brackets

We could say that completion.mini should respect ranges and close this issue, however, the existing oxide implementation is a bug

• we were eating two characters after the link (because we always incremented the range by two)
• we could increment the index past the line and that's why there was the min invocation

"eating characters":

https://github.com/user-attachments/assets/4081b35c-4783-4d5b-bc3b-7dbc81a2ab74

So the fix in this PR is this:

• if there are trailing brackets, create the insert text without brackets
• if there aren't trailing brackets, create the insert text with them
• keep the same position for start and end of the range and only insert the text

This works well for me (with completion.mini), but it also works well with the VS Code Extension that's in this repo:

https://github.com/user-attachments/assets/f7d20fc7-7837-4ee0-b338-ad25bdc369f6

WDYT?

[viktomas]

@Feel-ix-343 Ugh, but I see an issue, now the cursor is within the [[]] (e.g. [[Page<here>]]), which is not too nice because you can't start typing straight away. That's why we were probably replacing the closing brackets in the first place 😢

[Feel-ix-343]

What an investigation hah

---

What was the issue with replacing characters beyond the current line? Also mini.completion seems to be fixed in their master branch fyi

[viktomas]

What was the issue with replacing characters beyond the current line?

@Feel-ix-343 The issue is that when we defined the character index of the end position, we always incremented the index by 2 (to replace the preexisting ]]). But we did this regardless of whether the brackets were present in the document or not.

So when there were some different characters after the completed text (e.g. 123), we replaced the first two characters (e.g. 12)

You can see the error reproduced in the first video in this comment https://github.com/Feel-ix-343/markdown-oxide/pull/279#issuecomment-3191471763

---

I updated this PR to only increment the end character position if the brackets actually exist in the document.

I tested the fix in the official VS Code Extension, and it works

https://github.com/user-attachments/assets/366a9e06-4b06-4782-993a-a554bc1aa815