# Add column and line numbers for suggested edits

[marrej]

This PR addresses the addition of column & line numbers to the Refactor Preview.

• Adds user setting "Refactor Preview: Show Line And Column Numbers"
  • This user setting is turned on by default (allows the user to disable if they find it disturbing), as we want the users to get this feature & decide whether it doesn't make sense for them to see it.
• Renders the [Ln x Col x] only for parts of edit that contain select/insert (not rendered for prefixes to declutter ui) (see image below)
  

Technical notes:

• Select range is not modified for TextEditElement, as the Select (left) side does not change with active edits
• Insert range is calculated using line & column offsets
  • Originally wanted to pull this information from the model, but seemed to create more unreadable code & tangled BulkEditPreview with BulkEditTree
• Offsets are calculated only from checked edits (as they are rendered on the textModel & will impact other edits)
• Ln/Col position are refreshed on every check/uncheck
• Select & Insert ranges are undefined unless there is some string inserted or selected. This makes it easier for the label generation to directly generate the [Ln x Col x] only on defined ranges.

Fixes #165759

[jrieken]

Thanks for kicking this off. Generally, I think that we should align this with search.showLineNumbers, e.g off by default, maybe lines only, and special rendering

[lamm45]

I think it would be great to have special handling for renames, because in those cases Ln/Col is always the same for select and insert. I would remove the insert Ln/Col so that "before" and "after" are next to each other as they are currently.

I would also like to have all Ln/Col values to be horizontally aligned next to the checkbox, because that would make it easier to visually search for a specific value or range. However, I don't know how this would work in non-rename situations where the insert Ln/Col is present.

[marrej]

Hey @jrieken Thanks for the quick reply & for the wait untill I came back from my Xmass vacation :)

I drew the base inspiration from the Problem Panel (see below) where the Ln/Col would be explicitly called out.
The main reasoning here was that the :123 positioning might not work great for the double occurences which happen on the WorkspaceEdit replacements (In the image in the description only the deletes/inserts are shown)

• I agree that the [Ln x Col x] could be highlighted, e.g. by bolding (I would stay away from color highlighting since that is already used in the Refactor Preview panel)
• For the column numbers I don't have a strong opinion on keeping them (since users rarely care about the actual column position of token), but I am pro for keeping them if we keep the Problems panel as the base (rather than the Search panel)

Do you think there is neccessity to rather align it with the search panel?
If not should we then align the Markers details/Problem panel design with the Refactor Preview [Ln x Col x]?
WDYT?

Hey @lamm45
I don't neccessarily want to introduce non needed complexity to the code, that does not add too much value. I would rather keep the rules to minimum & not introduce edge cases.
Also keeping the panel consistent makes sense, I wouldn't want users to get confused why there is or isn't the positioning shown at some cases.
What is your reasoning for adding this rule? Do you think that having the [Ln x Col x] in the insert part as well would be confusing? detrimental to the UX?

I didn't completely understand the positioning issue you mentioned. At the moment the label is horizontally aligned with the checbox. And As far as I know the label is clipped with ellipsis if the window gets smaller (no linebreaks).

[lamm45]

I don't think renaming is an edge case. In fact, it is the only type of VS Code refactoring I have ever applied. I don't even know how to do any other refactoring, but apparently that would be possible with some language/extension combinations that I'm not using. To clarify, I am talking about renaming which occurs when you right-click a symbol and select "Rename Symbol" (that's also editor.action.rename, default key F2). As shown below, this will show up in the "refactor preview", even though renaming is not even considered a "refactoring action" in this page: https://code.visualstudio.com/docs/editor/refactoring

This is how the refactor preview currently works, when renaming xy to z:

This can be presented textually as:
static const unsigned long long xyz[] = {0, 1, 2};
if (!xyz[*xyz])
if (!xyz[*xyz])

I was suggesting simply this (the position info in square brackets is only schematic - I'm not addressing how it should look like or whether column numbers should be shown):
[2] static const unsigned long long xyz[] = {0, 1, 2};
[5] if (!xyz[*xyz])
[5] if (!xyz[*xyz])

By horizontal alignment I mean that every position info starts from the same horizontal coordinate in the screen. Just like [2], [5] and [5] above. According to the picture in the PR, this is not always the case (only the first three position infos are aligned, whereas the fourth and fifth position infos are placed more towards the right of the screen).

Placing a redundant position info between the "select" and "insert" parts would, in my opinion, unnecessarily separate those two parts and make it slightly more difficult to comprehend the changes while taking into account the surrounding context. It would also be weird if the position info for "select" is always next to the checkbox but the position info for "insert" is in another place in the middle of the text, but having both of them next to the checkbox would be even more confusing. Moreover, position info which is placed in the middle of the text (as opposed to the proximity of the checkbox) may cause confusion if whitespaces are involved (either in the renamed text or adjacent to the renamed text). Currently, whitespaces are very easily recognized since the preview does not add any extra whitespaces in the middle of the text, as can be seen from the image above.

Either way, I'm going to use this feature, because having no line numbers for the rename preview is by far the worst option. I wonder why they haven't been added before.

[marrej]

Hey @lamm45, Thank you very much for the detailed response, I very much appreciate it. You indeed raise a good point.

I agree that for the case of renames, adding the [Ln x Col x] right before the insert might not be perfect UX.

So I see these ways forward:

• Use the :123 positioning, as is used in search panel, containing the Select position as primary, and in case of no Select Position (Insert only) contain the Insert position. The placement should be on the end, to be consistent with the way how its used in search panel (and even Problems panel,even though that is using different notation), but I agree that placing at the start would make most sense and I would go for this option if possible.
• Continue using the [Ln x Col x] (With or without the Column position), with the same rules as proposed for the :123 positioning. (Displayed only at the start, and not for select and insert both)

@jrieken do you have a strong opinion about using the :123 notation? If so should we change it as well for the Problems panel so it can be consistent with the other views?