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

Reuse table widget in picker #3053

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

sudormrfbin
Copy link
Member

@sudormrfbin sudormrfbin commented Jul 12, 2022

Continues #2814. This change reuses the table widget directly in the picker, finally allowing items to be formatted as columns 🎉. Column formatting can be done by implementing menu::Item::row() on the picker items.

Behavioral Changes

  • There was a subtle bug in rendering the highlights for matching text which produced incorrect highlighting with multi byte characters. The algorithm to calculate highlights had to be re-written anyway to be injectable into a row of the table, and the issue was fixed along with this:
    The fuzzy matcher returns char indices of the choice that matched
    the pattern, while highlighting can only be done on whole graphemes.
    The earlier code assumed char indices == grapheme indices which
    broke highlighting with characters like å.
    
    This commit solves this issue by converting highlighting char indices
    to byte ranges, finding the grapheme which has this byte range as
    it's subrange, and then highlighting the whole grapheme.
Before After
picker-unicode-highlighting-incorrect-before picker-unicode-highlighting-correct-after
  • Pickers that previously aligned items to the right (like the file picker which shows the end of the filename if the window is too narrow) cannot do so anymore; this will require a new align property on table cells.

Bugs

  • I noticed problems with some columns not being displayed randomly when moving up and down the picker (with a modified column formatted version of the diagnostics picker). Requires proper investigation.

Future Work

  • Use column formatting in buffer picker, symbol picker, etc.
  • Add align property to table::Cell

@sudormrfbin sudormrfbin force-pushed the reuse-table-in-picker branch from 7d53b58 to 4003241 Compare July 18, 2022 19:02
helix-tui/src/text.rs Outdated Show resolved Hide resolved
@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2022
@kirawi kirawi added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2022
@sudormrfbin sudormrfbin force-pushed the reuse-table-in-picker branch from 4003241 to 88181d1 Compare December 14, 2022 17:11
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. labels Dec 14, 2022
@sudormrfbin sudormrfbin force-pushed the reuse-table-in-picker branch from 87ef666 to 9962bc8 Compare December 18, 2022 12:47
@sudormrfbin
Copy link
Member Author

The table column width calculation code has been updated to match the upstream implementation in the tui crate (specifically fdehau/tui-rs@a68e38e), and the random swallowing of columns have been fixed.

@gibbz00
Copy link
Contributor

gibbz00 commented Dec 27, 2022

@sudormrfbin I'm working on a branch that aims to refine the keymap presentation in both the info boxes and the command palette. Wanted to see how the command palette could look with column formatting, so I've already started on an implementation for that hehe.

5f8ba8c

I will post the draft during the week, only marking it as a PR once this one gets merged. Feel free to use the code in the commit in the meanwhile (also in case mine doesn't go through), I'll just make sure to omit the commit from my draft if you do :)

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Great work as always!

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

(oops)

@archseer archseer merged commit 5c7db7a into helix-editor:master Jan 18, 2023
@lazytanuki lazytanuki mentioned this pull request Jan 18, 2023
15 tasks
sudormrfbin added a commit to sudormrfbin/helix that referenced this pull request Feb 2, 2023
Arranges the document id, file flags (is-modified, is-current) and
file path in a table leveraging helix-editor#3053.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants