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

Add DeepWave clinkprompt Module #1

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

starfishpatkhoo
Copy link
Contributor

Adding the DeepWave clinkprompt Module if it is ok with you...

Copy link
Owner

@chrisant996 chrisant996 left a comment

Choose a reason for hiding this comment

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

Nice! I'll check the demo issue later today and get back to you.

-- Get our current Path
local function get_cwd()
return os.getcwd()
-- This is Windows, 'cd ~' doesn't work, so why bother...
Copy link
Owner

Choose a reason for hiding this comment

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

With Clink, cd ~ should work. Are you saying it isn't working right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no, I mean that by default Windows doesn't recognise ~ as home... there is a reason why clink is good stuff! But I wanted this prompt to be true to what Windows is.. shortcomings and all.. LOL...

-- clinkprompt Module Handlers

-- Upon Prompt Show
-- 20241018: This does not seem to be executed upon 'clink config prompt show', so we call it from ondeactivate() instead
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I'll try it later today. Last I had checked the demo callback was being called, but maybe it got broken at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if you try the sample.clinkprompt it doesn't work either.. I'm not sure what is supposed to happen though..

Copy link
Owner

Choose a reason for hiding this comment

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

Yup, I broke it before the official release, as a side effect of some cleanup changes.

I'll publish a fix in a week or two.

In the meantime I wouldn't recommend using ondeactivate, because that gets called whenever you use clink config prompt use to switch to a different clinkprompt. Users will not expect to see a demo of this prompt shown whenever they select some other prompt.

Tonight I'll try to see if there's another way to detect that clink config prompt show is happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the meantime I wouldn't recommend using ondeactivate, because that gets called whenever you use clink config prompt use to switch to a different clinkprompt. Users will not expect to see a demo of this prompt shown whenever they select some other prompt.

Yeah, I agree.. that should be a demo thing only... So this ought to be revised... Activate and Deactivate should be used for only exactly that.

Tonight I'll try to see if there's another way to detect that clink config prompt show is happening.

Actually, I like what clink config prompt show is doing now.. It gives users a brief preview of the prompt before switching to it.

But perhaps there can be a clink config prompt info or something instead of demo.. that just queries the prompt for information or status/settings/etc. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

From the perspective of a user, I like the idea.

Only, I don't want to make it something that prompt authors feel they need to add. And I'm not sure what "automatic" info would be meaningful to generate automatically for prompts that don't implement their own custom info.

What I've been thinking about is a way for a prompt's demo function to let Clink automatically show the demo, and then the demo function could just add some custom info, like you wanted. To minimize work the prompt author would need to do.

I think clink config prompt show is a reasonable place to hook that into.

And anyway it's almost no extra work if the prompt code is structured like...

local function render()
    -- Build the prompt string...
    -- blah blah blah...
    return prompt_string
end

local p = clink.promptfilter()

function p:filter()
    return render()
end

local function demo_func()
    -- print stuff before ...
    print(render())
    -- print stuff after ...
end

return {
    demo = demo_func
}

Copy link
Owner

Choose a reason for hiding this comment

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

(Where it gets a little more involved is if the prompt has a :rightfilter function, because then the author has to do the math/etc for right aligning the right side prompt demo. And that's why I'm considering making a way for demo functions to essentially say "ok Clink, do your thing and print a prompt here before/after I print other things".)

Copy link
Owner

Choose a reason for hiding this comment

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

If you move the demo into the demo callback, I'm happy to Approve the PR. And then after the next update in a week or two it'll start actually properly invoking the demo callback.

Copy link
Owner

@chrisant996 chrisant996 Oct 19, 2024

Choose a reason for hiding this comment

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

Commit clink/d1f34d7f fixes the demo callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only, I don't want to make it something that prompt authors feel they need to add. And I'm not sure what "automatic" info would be meaningful to generate automatically for prompts that don't implement their own custom info.

What I've been thinking about is a way for a prompt's demo function to let Clink automatically show the demo, and then the demo function could just add some custom info, like you wanted. To minimize work the prompt author would need to do.

I think clink config prompt show is a reasonable place to hook that into.

Hmm, I understand... I liked that there was automatically a nice comparison between the old prompt and the new, and this was an apples to apples comparison. Same path, default settings etc etc. Like, this is what you have currently, and given everything the same, this is what it would look like.

But okie, that's workable too, to provide more flexibility to prompt authors to maybe showcase more different settings and options with a demo() if they wished... ^_^

I've been assuming the basic prompt from render() would automatically/always be there in prompt show .. but now I see that's different, demo() does everything. So give me a day or two to think about what I want to show.. I mean demonstrate.. ha ha!

Anyway, truthfully, I'm really supposed to be working on other things right now, but this little prompt thing has been a fun (I mean naughty) distraction for me. Ha ha .. Thank you very much for clink and the distraction.. ^_^

@starfishpatkhoo
Copy link
Contributor Author

starfishpatkhoo commented Oct 19, 2024

Actually another thing is the rightfilter() and transientrightfilter() ..

function p:rightfilter()
   return "aaa"
end

function p:transientrightfilter()
   return "aaa"
end

If the transient right is EXACTLY the same as the right, then it disappears completely. I am not too sure why that is, or if it really is a bug. I added an extra escape code that does nothing so it becomes "different", and then it shows up...

I didn't want to open an issue for this (and the demo() function) because these things don't really affect normal users, only people who write prompt modules... And there are workarounds..

@chrisant996
Copy link
Owner

chrisant996 commented Oct 19, 2024

I didn't want to open an issue for this (and the demo() function) because these things don't really affect normal users, only people who write prompt modules... And there are workarounds..

It's ok if you'd rather not, but please do feel free to open issues for them in the Clink repo (and for stuff in general).

I'll definitely fix them; they're core functionality that isn't working according to spec.

If the transient right is EXACTLY the same as the right, then it disappears completely. I am not too sure why that is, or if it really is a bug. I added an extra escape code that does nothing so it becomes "different", and then it shows up...

Interesting! I'll take a closer look later today or tomorrow and figure out what's going on. Probably some display cache isn't being fully cleared in between prompts. I made a lot of display changes and caching changes between 1.6.21 and 1.7.0, but it's possible the right transient prompt might have not been working properly even before that.

UPDATE: It was quick and easy to find and fix: commit clink/cb0093d5 fixes the right transient prompt. It was probably not ever working correctly. During the clinkprompt and clinktheme changes in v1.7.0 I discovered and fixed a bunch of edge case bugs in display caching, even specifically for the transient prompt, but I didn't notice this one. Thanks for reporting it!

@chrisant996
Copy link
Owner

@starfishpatkhoo After you push a change moving the demo into the demo callback, I'll Approve and Merge the PR.

@starfishpatkhoo
Copy link
Contributor Author

UPDATE: It was quick and easy to find and fix: commit clink/cb0093d5 fixes the right transient prompt. It was probably not ever working correctly. During the clinkprompt and clinktheme changes in v1.7.0 I discovered and fixed a bunch of edge case bugs in display caching, even specifically for the transient prompt, but I didn't notice this one. Thanks for reporting it!

ah ha ha aha! Okie, I will remove the extra SGR esc char I added to workaround this.. LOL...

@chrisant996
Copy link
Owner

ah ha ha aha! Okie, I will remove the extra SGR esc char I added to workaround this.. LOL...

It's completely fine to include a workaround. The fix is merely so workarounds aren't required.

@starfishpatkhoo
Copy link
Contributor Author

@starfishpatkhoo After you push a change moving the demo into the demo callback, I'll Approve and Merge the PR.

Done! Basically I ended up adding a whole bunch of other stuff. Duh. Need to learn to just leave things alone. LOL. But I also found another strange issue, and opened an issue for that: chrisant996/clink#696

Thanks a whole lot once more!

@chrisant996
Copy link
Owner

But I also found another strange issue, and opened an issue for that: chrisant996/clink#696

U+26A1 renders inconsistently in Windows consoles. Don't use it. I replied in the issue with other characters to use instead.

(I'll probably make a minimal repro and open an issue in the Windows Terminal repo, to report the U+26A1 issue there.)

@starfishpatkhoo
Copy link
Contributor Author

But I also found another strange issue, and opened an issue for that: chrisant996/clink#696

U+26A1 renders inconsistently in Windows consoles. Don't use it. I replied in the issue with other characters to use instead.

(I'll probably make a minimal repro and open an issue in the Windows Terminal repo, to report the U+26A1 issue there.)

I understand, so I closed the issue.. I chose a different symbol anyways, cos the bolt started to get too distracting... Maybe I will revisit it in the future. :)

I think it just felt really weird to me because if you just ran it, it was fine. It only went "off" when you run cmd inside of cmd.. So some calculation or some carried over value was not right...

@chrisant996
Copy link
Owner

chrisant996 commented Oct 27, 2024

I think it just felt really weird to me because if you just ran it, it was fine. It only went "off" when you run cmd inside of cmd.. So some calculation or some carried over value was not right...

That's especially strange indeed.

I hadn't encountered that specific issue yet. But I've observed several different problems under different circumstances with U+26A1.

It looks like Windows Terminal is evaluating the cursor positioning ANSI escape code incorrectly for some reason. I'll add that info in the minimal repo problem report I eventually create in the Windows Terminal repo.

UPDATE: Nope, somehow Clink is computing the cursor position differently.

@chrisant996 chrisant996 merged commit d0ced91 into chrisant996:main Oct 27, 2024
@chrisant996
Copy link
Owner

Inside the nested cmd.exe, Clink doesn't detect that it's hosted inside Windows Terminal. So Clink falls back to expecting that the console only supports UCS2, not full Unicode.

But also, even conhost is rendering U+26A1 as 2 cells. So, there's something else strange happening with U+26A1 as well.

I'll look into that further. Maybe I'll be able to come up with reliable rules for predicting how consoles will render U+26A1, and maybe there are some other characters that have similar issues. I wrote a test tool that helps with analyzing that stuff, so I'll try it in more different console contexts (conhost, conemu, winterm, etc etc / and on multiple different versions of Windows, since they behave differently).

@starfishpatkhoo
Copy link
Contributor Author

Yeah, wish I had more experience such that I could help you track down more information.. Its weirdness has me stumped.. My gut tells me it is something simple, but buried deep...

@chrisant996
Copy link
Owner

Yeah, wish I had more experience such that I could help you track down more information.. Its weirdness has me stumped.. My gut tells me it is something simple, but buried deep...

Thanks!

I understand how and why the discrepancy about width measurement happens only in a nested cmd.exe.

But I don't fully understand yet why U+26A1 gets rendered as 2 cells in conhost. Or which other codepoints may encounter a similar problem. How to address it will depend on how many different codepoints suffer in the same way.

I'll look into it further in the next couple of days.

The good news is it will likely be possible to come up with a way to measure the width accurately for U+26A1. But it will still render funny -- as a left-aligned single width glyph in a double width cell.

@chrisant996
Copy link
Owner

chrisant996 commented Oct 28, 2024

Indeed:

  • Win8.1: U+26A1 renders as a single-width character.
  • Win10: U+26A1 renders as a single-width character in conhost, and as a double-width character (as a single-width glyph followed by a space) in Windows Terminal.
  • Win11: U+26A1 renders as a double-width character in both conhost and Windows Terminal (as a single-width glyph followed by a space).

So Win8.1 isn't aware of double-width color emoji codepoints, and Win10 is only aware of them in Windows Terminal. But Win11 is aware of them in both conhost and Windows Terminal and respects their defined widths even when not rendering them as color emoji.

...At least for U+26A1, but there are a lot of other codepoints to test. Hopefully the results are consistent across all of them.

@chrisant996
Copy link
Owner

It turns out that on Win11 there are 122 double-width color emoji which render as double-width, but the console only gives them 1 cell -- not 2 cells as anticipated since they're double-width.

wcwidth-verifier
wcwv.exe --verbose --skip-all --no-skip-color-emoji --no-group-headers

26 of them are the "regional indicator symbol letter A-Z" glyphs. My understanding from the Unicode spec is that these are meant to be double-width. Windows Terminal renders them as a single-width glyph and they take only 1 cell. But maybe I've misunderstood the spec, and maybe these are supposed to a single-width.

The other 96 codepoints with reported width mismatches are rendered as double-width color emoji glyphs.
For example:
image

U+26A1 is in a group of 61 color emoji codepoints which on Win8.1 are 1 cell wide and render as missing glyphs (as expected), but on Win11 are 2 cells wide as expected (I checked many of them, but not all of them; there could be some overlap between the 61 and the 96 mentioned earlier).

I should be able to fix how the width is calculated for those 61 codepoints, at least for conhost and Windows Terminal. But maybe not for certain other combinations of versions of OS and terminal programs.



Sidebar: I noticed some funny things about the color emoji definitions. At first they confused me, and I thought my data set might be inaccurate, but after some research they made sense:
For example, the range U+1F170 through U+1F189 is "Negative Squared Latin Capital Letter A - Z".
So why are only "A", "B", "O", and "P" in that range rendered as color emoji?
Well, for "A", "B", and "O" it's because they are blood types -- which explains the red background color, and explains why U+1F18E ("AB") also renders as a color emoji with red background.
I don't know why "P" in that range is rendered as a color emoji.

But the whole range of U+1F170 through U+1F189 render as 2 cells wide ... but the console APIs report they are only 1 cell wide. Even for the ones that render as monochrome glyphs (rather than as color emoji).

Hopefully these examples illustrate some of why it's so hard for text mode programs to deduce the effective width of a given codepoint -- there are lots of departures from the Unicode spec, and different versions of the OS and DirectX and Windows Terminal can end up with different results. And also the Unicode spec is constantly evolving, and software is always trying to play catch-up, which further exacerbates the problems. I completely understand how and why it's so problematic, and I applaud the many people who are constantly trying to improve how all of this stuff works in practice.

In the meantime, however, there's only so much that Clink can realistically do to accurately measure the width of codepoints.

@chrisant996
Copy link
Owner

Clink v1.7.4 will include some improvements to width measurements for many color emoji codepoints (commit clink/ae5ea42).

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