Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: The search overlay accepts IME composed input #5564

Merged

Conversation

kenchou
Copy link
Contributor

@kenchou kenchou commented Jun 14, 2024

  • The search overlay accepts IME composed input

#5333 (comment)
Based on other Writers, I wrote a writer for the search overlay.
The good news is that it appears to be working well.
However, as I am relatively new to Rust, there might be some bad code. I would greatly appreciate it if you could review the code for me.
Thank you. @wez

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

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

Thanks for this! A couple of minor comments!

wezterm-gui/src/overlay/copy.rs Outdated Show resolved Hide resolved
wezterm-gui/src/overlay/copy.rs Outdated Show resolved Hide resolved
wezterm-gui/src/overlay/copy.rs Outdated Show resolved Hide resolved
kenchou and others added 3 commits June 14, 2024 20:44
Co-authored-by: Wez Furlong <wez@wezfurlong.org>
Co-authored-by: Wez Furlong <wez@wezfurlong.org>
Comment on lines 1527 to 1530
let s = std::str::from_utf8(buf).map_err(|err| {
std::io::Error::new(std::io::ErrorKind::Other, format!("invalid UTF-8: {err:#}"))
})?;
render.pattern.push_str(s);
Copy link
Owner

Choose a reason for hiding this comment

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

actually, it might be possible to do this:

  render.pattern.write(buf)?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

emm, IDE hint
Type mismatch [E0308] expected `&mut TiffWriter<_>`, but found `&[u8]`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[MEMO]: After accepting PR #5416, render.search_line.insert_text(s) should be used here instead.

Copy link
Owner

Choose a reason for hiding this comment

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

Would you like to rebase and update this PR to do that now that #5416 is merged?

self.cursor
}
}
Movement::EndOfLine => self.line.len(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

EndOfLine is simply set to self.line.len()

BTW: self.cursor is a byte index, and the index(storage) is not related to graphemes(visual), so perhaps calling it cursor_index would be clearer.
@Mrreadiness @wez

Copy link
Contributor Author

Choose a reason for hiding this comment

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

emm... I think the crash might be caused by self.line.len().saturating_sub(1), as the offset is not at the boundary of a Unicode character, which causes cursor.next_boundary(&self.line, 0) to panic. Just like StartOfLine, EndOfLine doesn't need any extra calculation; simply using self.line.len() is the most straightforward approach.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, what I think we should do here is:

  • Remove this change from this PR, as it has a different thesis from the rest of the PR
  • Make a new PR that adds a couple of unit tests around is EndOfLine operation, and fixes the problematic case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

@wez wez merged commit 1533409 into wez:main Jun 15, 2024
13 of 15 checks passed
@wez
Copy link
Owner

wez commented Jun 15, 2024

Thank you!

wez added a commit that referenced this pull request Jun 15, 2024
saep pushed a commit to saep/wezterm that referenced this pull request Jul 14, 2024
* Feature: The search overlay accepts IME composed input
* use LineEditBuffer

Co-authored-by: Wez Furlong <wez@wezfurlong.org>
saep pushed a commit to saep/wezterm that referenced this pull request Jul 14, 2024
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.

2 participants