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

Show OSC8 link URL at bottom of screen on mouse hover #5830

Closed
wants to merge 1 commit into from

Conversation

ppwwyyxx
Copy link
Contributor

@ppwwyyxx ppwwyyxx commented Dec 28, 2022

Fix #5827

URL is displayed at bottom by default. If cursor is at the bottom, then URL is displayed at top. Below are screenshots of what it looks like in a terminal with two windows.

Image 1

2022-12-28_16-51

Image 2

2022-12-28_16-51_1

@page-down
Copy link
Contributor

screen_draw_overlay_text is mainly used for drawing IME pre-edited text, you can't just use it like that.

This leads to at least the following problems:

  • Pre-edited text disappears.
  • Cursor is in the wrong position after canceling input.
  • During input, the displayed URL address flies to the cursor position.

When displaying URL addresses, I believe that some common practices to prevent phishing should be used rather than just outputting the original address as is.
For example, Punycode should be used when displaying domain names, and the host name part may also be bolded. Also, when displaying the path, it should be decoded instead of displaying Unicode in URI encoded form.

@page-down
Copy link
Contributor

When I was trying to test "URLs that exceed the screen width and height", kitty just crashed.
I think when implementing this, the length should be taken into account, e.g. only half the screen height can be used and the longer part should be cropped.

@ppwwyyxx
Copy link
Contributor Author

Thanks for the comment. I am aware that screen_draw_overlay_text was mainly used for drawing IME pre-edited text. I'm giving it more capabilities here. Of course, it's debatable whether to redo the overlay or reuse the existing one.

Pre-edited text disappears.
Cursor is in the wrong position after canceling input.

Thanks for mentioning this issue. I think the latest commit fixed it.

During input, the displayed URL address flies to the cursor position.

I'm not sure I understand what it means. If I do the following:

  1. hover on a URL
  2. start typing with IME

The URL will just disappear and IME overlay will appear. This behavior is reasonable to me.

I believe that some common practices to prevent phishing should be used rather than just outputting the original address as is.

  1. To prevent phishing, having the URL displayed is much better than not. So at least it's an improvement in security.
  2. A user is not expected to be able to copy or click the displayed URL (I'd be surprised if it's copyable or clickable). In that case, it doesn't feel as necessary to me to apply sophisticated rules on how to display the URL.

When I was trying to test "URLs that exceed the screen width and height", kitty just crashed.

I'm not able to reproduce this:
2022-12-27_18-54

The last character that's displayed in the overlay looks weird (this happens for IME as well), but I didn't observe a crash.

@kovidgoyal
Copy link
Owner

I dont think using overlay line is the right way to do this. Instead use
existing facility to draw window titles when doing a visual window select. See
the function render_window_title.

Basically, the algorithm would be:

  1. When hovering over a hyperlink (as opposed to a detected URL) set a
    property on the Screen object with the URL
  2. In draw_cells check if the property exists and if so use a function
    similar to render_window_title to draw the URL
  3. (2) should come with a setting in kitty.conf to turn it off
  4. The location of the rendered URL should be either top or bottom of
    window depending on where the cursor is currently.

@page-down
Copy link
Contributor

I'm not sure I understand what it means.

Switch step 2 with step 1. This is for the first commit, and after the second commit I think you have resolved it.

In any case, if you use this, you want to make sure that at all states, you don't enter a state that is not expected. As kovid said, this is not a proper facility to implement this feature.

... some common practices to prevent phishing should be used ...

... it doesn't feel as necessary to me to apply sophisticated rules on how to display the URL ...

I don't quite understand.
The reason you suggested the need for this feature is precisely to avoid phishing. And what I am proposing is the most basic thing that needs to be considered for this after implementing "Show URL".
To be honest, personally, I don't care much about fancy emoji domains, I just want to make sure that, for example, U+0430 is distinguishable from U+0061.
And for the path part, if you insist on using URI encoding, then for Unicode this is not for human eyes and it makes no sense to display it.

@kovidgoyal
Copy link
Owner

Regarding the display of the URL I suggest you just display it as is. I will take care of sanitizing it to make it useful for phishing detection.

@ppwwyyxx
Copy link
Contributor Author

Sorry, render_window_title looks quite difficult to me as I'm not familiar with glfw.

  1. In draw_cells check if the property exists and if so use a function similar to render_window_title to draw the URL

Would it make sense for me to do the following instead?
In handle_move_event if a hyperlink is found, use a function similar to screen_draw_overlay_text to draw the URL.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Dec 28, 2022 via email

@ppwwyyxx
Copy link
Contributor Author

@kovidgoyal thanks for the guidance. I added render_hyperlink_preview similar to render_window_title. Please let me know if it looks reasonable. Thanks!

@kovidgoyal
Copy link
Owner

OK, I will review when I have a moment.

@page-down
Copy link
Contributor

page-down commented Dec 29, 2022

@ppwwyyxx
I have tested the latest commit and found an issue.

When the mouse cursor is on top, the URL is still displayed on top, leading to the area pointed by the mouse being covered.

The location of the rendered URL should be either top or bottom of
window depending on where the cursor is currently.

I assume that we are talking about the mouse cursor here.

Steps to reproduce:

  • Create a new window
  • ls --hyperlink=always
  • Point to hyperlink with the mouse and you can see the URL displayed at the bottom.
  • ls -l; ls --hyperlink=always
  • Use ls to output the text that fills the window. Now the URL is displayed at the top.
  • Scroll the window to the top and move the mouse cursor to point to the hyperlink output by the first ls command located at the top of the window. However, the URL is still displayed at the top and covers the text that the mouse is pointing to.

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.

Show URL when hovering on OSC8 hyperlink
3 participants