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

suggestion: added display #692

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

rsteube
Copy link
Contributor

@rsteube rsteube commented Dec 30, 2023

Full inserted value:
image

Display value:
image

TODO

  • fix column width calculation

related nushell/nushell#5292
needs #691

@rsteube rsteube force-pushed the suggestion-display branch 2 times, most recently from 5826d06 to 5e7a9ea Compare January 24, 2024 14:40
@rsteube rsteube marked this pull request as ready for review January 24, 2024 14:41
@rsteube
Copy link
Contributor Author

rsteube commented Jan 24, 2024

@stormasm this one can be tested with nushell/nushell#11444

@stormasm
Copy link
Contributor

stormasm commented Jan 24, 2024

@rsteube @fdncred Thanks for bringing this one up to speed too...
Unfortunately we are waiting on this PR to land sharkdp/lscolors#81
[As I am erroring on the side of caution and don't want to get too far ahead of ourselves]
before we can move forward on landing any more Reedline PRs...
I want to be able to test on the nushell side of the world and nushell is currently blocked from updating Reedline 😢

@rsteube
Copy link
Contributor Author

rsteube commented Jan 24, 2024

Yeah, i ran into that as well (duh!). Didn't expect that the patching does not work for the transitive dependency.

@stormasm
Copy link
Contributor

Now that I think about this some more I am hesitant to add another field to Suggestion...
I don't want Suggestion to get bigger than it needs to be...

Why not just use the

 pub extra: Option<Vec<String>>,

extra instead ?

It can be viewed as kind of an additional (generic) location to grab stuff from ?

And then you can just grab your display value from the first item in the extra vector.

And the end user of Carapace has no idea where that data is coming from anyway right ?

Thoughts on this...
Maybe I am not understanding the concept completely...

@rsteube
Copy link
Contributor Author

rsteube commented Jan 25, 2024

Yeah I think there might be some misunderstanding.
The issue is that reedline doesn't discern between what is inserted and what is shown during completion.
When you cd /some/super/long/path/with/many long segments the menu becomes confusting at first and broken when it exceeds the available space.
Parent directories also don't really matter to the user (as those are visible on the command line), just the last completed segment is relevant.

Truncating isn't really an option. Fish does this and it gives awful results as it can't know where to make the cut.
Completion scripts however do know this and just need a way to pass this to reedline.

The string vector extra seems pretty cryptic to be honest.

@stormasm
Copy link
Contributor

Completion scripts however do know this and just need a way to pass this to Reedline.

Thanks for helping me understand the problem better...

So if "extra" was called "display" but we kept it a Vector like it is right now would that work ?

instead of this...

/// Optional display value for the replacement
    pub display: Option<String>,

do it this way...

    pub display: Option<Vec<String>>,

Or to phrase it another way

Completion scripts however do know this and just need a way to pass this to Reedline.

Do the completion scripts care whether they are passing a String or a Vector of Strings ?

@rsteube
Copy link
Contributor Author

rsteube commented Jan 25, 2024

Can't really make much sense of the vector here. What is it for?

@fdncred
Copy link
Collaborator

fdncred commented Jan 25, 2024

I kind of like the idea of having a display and a full path for certain situations. it makes it more intuitive like the screenshots show. i'd like to see an new example to demonstrate this better though.

I tried the nushell PR and expected to not see crates in all the suggestions.
image

@rsteube
Copy link
Contributor Author

rsteube commented Jan 25, 2024

@fdncred cd is the nushell completer which doesn't make use of this (yet?). Try bat <TAB> or something else.

@rsteube
Copy link
Contributor Author

rsteube commented Jan 25, 2024

image

image

image

image

@fdncred
Copy link
Collaborator

fdncred commented Jan 25, 2024

cd is the nushell completer which doesn't make use of this (yet?). Try bat or something else.

I guess what I'm saying is, if we're going to land this feature, nushell should support it, maybe with an option.

I'm not getting much fun with bat either.
image

@fdncred fdncred marked this pull request as draft January 25, 2024 21:54
@stormasm
Copy link
Contributor

Can't really make much sense of the vector here. What is it for?

I guess what I am saying is that if you think about the Suggestion as a Container in which to pass
more generic type of information...

Where the end user of the Suggestion may have different needs in their application

The Style PR that we landed yesterday and this Display PR currently could have been represented
in the "extra" Vector of Strings where the first field of the Vector was the "Style" and the second field
of the Vector is the "Display"...

I believe that was the intention of the "extra" field in the first place.....

Because other applications will come along later and want other stuff in their Suggestions possibly...

But I could be wrong --- why is extra there anyway ---- what can it be used for ? Is it named properly ?

@fdncred
Copy link
Collaborator

fdncred commented Feb 9, 2024

@rsteube Are you still wanting to move forward with this?

@rsteube
Copy link
Contributor Author

rsteube commented Feb 9, 2024

Sure, but how do you want to go forward with it?

@fdncred
Copy link
Collaborator

fdncred commented Feb 9, 2024

I think it's fine with the display member of the suggestion structure, but we don't want to have breaking changes either. @stormasm may have different suggestions.

@stormasm
Copy link
Contributor

stormasm commented Feb 9, 2024

@rsteube I am fine with moving forward with this PR as well...
Please go ahead and resolve the conflicts...
Thank you !

@rsteube rsteube force-pushed the suggestion-display branch 2 times, most recently from 1f89ec3 to a5e0b9f Compare February 9, 2024 17:45
@fdncred
Copy link
Collaborator

fdncred commented Feb 9, 2024

I'll probably be ok with this PR once I can see it work and understand if there are bugs or not. I can't get the nushell PR that references this PR to work. So, I'm glad these are draft. I'm also a tiny bit concerned that this is another breaking change.

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.

3 participants