Skip to content

Add a "Go to Line/Column" dialog#133

Merged
lhecker merged 2 commits into
microsoft:mainfrom
diabloproject:main
May 24, 2025
Merged

Add a "Go to Line/Column" dialog#133
lhecker merged 2 commits into
microsoft:mainfrom
diabloproject:main

Conversation

@diabloproject

@diabloproject diabloproject commented May 20, 2025

Copy link
Copy Markdown
Contributor

Since issue had E-help-wanted, I figured I can try to implement the Goto. Would be glad to see the feedback. I haven't done any localization yet.

Closes #21

@diabloproject

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Comment thread src/bin/edit/draw_editor.rs Outdated
let parts: Vec<_> = line.splitn(2, ':').collect();
Ok(match parts.len() {
1 => {
if let Ok(line) = parts[0].parse::<i32>() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

FYI: this allows negative numbers, e.g. -123, 1:-32.

This seems to be handled well by the editor (looks like it just goes to the nearest valid spot), but it might be a better experience to mark negative numbers as invalid

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Parsing negative values as invalid does not sound like a right choice to me, but there are multiple ways to handle them:

  • wrap to 0 (e.g., vscode uses this logic for column number, my implementation)
  • use relative numbering (e.g., vim uses it)
  • use "python indexing", aka -1 == count from the end (vscode uses for lines)
    And probably maintainers should decide which one.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the "python indexing" idea would be interesting.

And I agree that the maintainers should have the final say.

It looks like the logic is here (clamp x and y to be positive):

let pos = Point { x: pos.x.max(0), y: pos.y.max(0) };

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably, if vscode was supposed to be a prototype in a sense. But I want to point out, that wrapping logic is not buffer's logic, it's the logic of the goto, so the calculation should be done in goto handling, in draw_editor.rs

@juemrami

juemrami commented May 20, 2025

Copy link
Copy Markdown
Contributor

I was also working on this. (Ended up using a top bar similar to the draw_search one instead of a modal)

But here some consideration I made that you may find useful, cheers 😄.

  • Negative values treated like reverse index as mentioned (i used the .stats.logical_lines as a line count)
  • "0" should be an alias for "1" (both treated as line @ index 0).
    • VSCode default is to treat "0" as invalid
  • Only ignore invalid column entries when the query contains a valid line, instead of invalidating the whole query.
    • ie "10:abc" will just navigate to "10:0"
    • This is the VSCode default behavior

@lhecker

lhecker commented May 20, 2025

Copy link
Copy Markdown
Member

I'm receiving a lot of comments and PRs at the moment, and I may need a few days to get back to this one. I apologize for that. (I took a cursory glance though and this looks super cool!)

@diabloproject

Copy link
Copy Markdown
Contributor Author

Is this PR alive?

@lhecker

lhecker commented May 23, 2025

Copy link
Copy Markdown
Member

Yes, but my intention was to check this out next week. I'll give it a look now.

@diabloproject

Copy link
Copy Markdown
Contributor Author

Oh, no rush, you just told that in few days and almost all other PRs had changes requested, so I thought that you may have overlooked it.

lhecker added a commit that referenced this pull request May 23, 2025
@lhecker

lhecker commented May 23, 2025

Copy link
Copy Markdown
Member

Yeah, that's a fair assumption. I do overlook PRs from time to time, because there's genuinely a lot of them. 😅

I made changes and wrote feedback over in this commit: c5f26b9
I did that because I felt like it may be difficult to explain it inside this PR. I left "NOTE" comments in each spot that I thought may be worth pointing out. You can adopt nothing, parts, or all of the code. I'll leave that up to you. If you do adopt parts of it, please feel free to remove the "NOTE" comments.

The important part is the early-return (which is a bug in your code) and the missing translations. The other bits can be considered suggestions. 🙂

@lhecker lhecker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll set this as "Changes requested" again. I do try to read every comment/update so I should not miss it when you respond / push changes. But if I do, please ping me again. Thanks!

@diabloproject

Copy link
Copy Markdown
Contributor Author

Made requested changes + rebased the branch to master (I lost my patched unix.rs and I am working on Mac, and mac pr was merged prior).

@diabloproject
diabloproject requested a review from lhecker May 24, 2025 07:30

@lhecker lhecker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for making these changes!

@lhecker lhecker changed the title goto prototype (#21) Add a "Go to Line/Column" dialog May 24, 2025
@lhecker
lhecker merged commit b284b79 into microsoft:main May 24, 2025
diabloproject added a commit to diabloproject/edit that referenced this pull request May 29, 2025
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.

Add a "Go to Line/Column" dialog

4 participants