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 infobox with register contents #980

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

sudormrfbin
Copy link
Member

Fixes #965.

Screenshot_2021-11-05_00-36-16

Comment on lines 1086 to 1097
if let Some(mut info) = cx.editor.autoinfo.take() {
info.render(area, surface, cx);
cx.editor.autoinfo = Some(info)
}
Copy link
Member

Choose a reason for hiding this comment

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

Why the need for take() here?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, it sits on the editor now

Copy link
Member

Choose a reason for hiding this comment

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

It could be an extra field on the context? That way UI is fully stored on the ui::Editor

Copy link
Contributor

@EpocSquadron EpocSquadron left a comment

Choose a reason for hiding this comment

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

So there is an issue that occurs if one of the registers contains more than one line that doesn't get truncated away by the character limit. In that case, the infobox will not be high enough to fit all of the info text, as the height is set to the number of strings in the body vec passed to it. There's two ways around this -- we can alter the calculation of the height to include newlines in each body item, or we can truncate the register contents at whichever comes first of the character limit or first instance of a newline. I would argue for the former, as there could conceivably be other cases where the description of an item should wrap in the future, and the info box should be robust against that.

Copy link
Contributor

@EpocSquadron EpocSquadron left a comment

Choose a reason for hiding this comment

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

In current state, this works pretty well. One corner case I mentioned in the original issue was that some registers can contain multiple selections. I don't know how much effort it would be to add some description of the number of selections to the infobox somewhere. In my head I see a table setup where there's a third aligned column with "24 sels" and the like for registers that have them. That column would not appear if none of the registers had more than one sel. If we want that to be a separate issue so we can merge this sooner, that's cool too.

@EpocSquadron
Copy link
Contributor

EpocSquadron commented Nov 5, 2021

Building on both my previous comments, it might be nice to show an ellipsis with the number of lines occupied by a register when there are more than 3 total lines. This could be combined with the selection number hint in something like this:

"     <style>
        .someClass {
   … (16 more lines)
: theme everforest_dark
/ style
b [5 sels]
      :v-if="language == 'en-US'"
      :checked="checked"
   … (2 more lines in first sel)

It's a bit ugly, so it might be worth some more noodling over format. I can also see it being a little finicky. For example, if the second line of a register's contents cause the character limit to take effect, we still need to display the number of additional lines below it, which might be missed if we specifically look for more than 3 lines.. that kind of thing.

Maybe we could colorize the hints and buffer identifiers to make things a little clearer?

@Omnikar
Copy link
Contributor

Omnikar commented Nov 6, 2021

On the issue of registers containing multiple selections, perhaps the infobox could just show the contents of the first selection, and then we can have a command like :reg which can be used to pull up a full view of all the selections of a given register?

@EpocSquadron
Copy link
Contributor

On the issue of registers containing multiple selections, perhaps the infobox could just show the contents of the first selection, and then we can have a command like :reg which can be used to pull up a full view of all the selections of a given register?

I think down the road it would be cool to have something like that that uses the picker interface to see the history of items in that register (like yank-ring), which would both give a better view of the contents as well. For this particular pull request though, I'm looking for a quick reminder on-the-fly of what registers are available and what's in them presently. I think because of that intended purpose, it makes sense sense to also hint the number of selections, but I wonder if we save that at this point for a separate pull request and stick to just addressing the bug with multi-line contents. Once the basic functionality is out in the wild, more people might pop up with ideas on how to evolve the functionality.

helix-view/src/info.rs Outdated Show resolved Hide resolved
@sudormrfbin
Copy link
Member Author

Rebased and pushed. It's usable enough in the current form, and I think we can address the suggested improvements separately so that this can get merged sooner.

@sudormrfbin sudormrfbin marked this pull request as ready for review February 4, 2022 16:14
@EpocSquadron
Copy link
Contributor

So there is an issue that occurs if one of the registers contains more than one line that doesn't get truncated away by the character limit. In that case, the infobox will not be high enough to fit all of the info text, as the height is set to the number of strings in the body vec passed to it. There's two ways around this -- we can alter the calculation of the height to include newlines in each body item, or we can truncate the register contents at whichever comes first of the character limit or first instance of a newline. I would argue for the former, as there could conceivably be other cases where the description of an item should wrap in the future, and the info box should be robust against that.

I think this one never got resolved. I can't recall if it caused a panic, but probably should double check before we merge.

@sudormrfbin
Copy link
Member Author

Thanks for reminding @EpocSquadron :) Fixed in dbdb645.

@sudormrfbin sudormrfbin requested a review from archseer February 8, 2022 16:22
@archseer
Copy link
Member

Great work! 🎉

@archseer archseer merged commit fa83426 into helix-editor:master Feb 10, 2022
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 register contents when typed
5 participants