Incorrect oh-my-posh formatting messes up Clink about clink HOT 46 CLOSED

It already is. Look at the source code from my cmder-powerline-prompt fork.

Oh shoot, I was somehow looking at an older committed file. Found it now!

It breaks backward compatibility and can easily actually totally break things if the user doesn't delete their existing config file.

Thanks, I understand.

If you're using the Clink 1.1.17 pre-release and the latest from my cmder-powerline-prompt fork then you already have the fixes.

I was talking about a (pre)release of your cmder-powerline-prompt fork 😅. I know that Clink already has a pre-release and an official release is just around the corner.

It's very close to an official release, so please keep reporting any issues you do happen to encounter! Thank you for helping it on its way towards an official release. :)

Thank you for keeping this alive! The least I could do was help polish it by reporting any issues :)

from clink.

Also may I suggest enabling Issues tab in your powerline fork? I think most of the conversation in this thread is not relevant for Clink's development. It will also help in reporting powerline-specific issues, like this one: the line here https://github.com/chrisant996/cmder-powerline-prompt/blob/f829e64a3f337dbefc7c540d356ab7bbeafeb2fc/_powerline_config.lua.sample#L56 mentions 'left' and 'priority' as possible date locations, but they don't seem to appear in the setting above this.

from clink.

1. oh-my-posh is pretty slow.

@chrisant996 glad to say I found the culprit. ...

Ah! Thank you, Jan -- both for finding and fixing it, and for validating my perception! It's reassuring to know I wasn't just imagining it. 😁

from clink.

1. oh-my-posh is pretty slow.

@chrisant996 glad to say I found the culprit. It seems the YouTube segment (that does a GET request on localhost) behaved rather differently on Windows vs other platforms. Go most likely has a different default timeout on Windows and other platforms, causing the segment to take up to 2 seconds when it could not reach the endpoint. This has been set to 50ms (sufficient for the use-cases) which should put everything back to normal.

@JanDeDobbeleer hi, just a quick follow-up: I downloaded the latest oh-my-posh and now it's indeed very fast! Thank you for tracking down that Windows-specific issue and fixing it! ❤

from clink.

@chrisant996 we've secretly done some more improvements, which hopefully resort to a good experience on Windows. Thanks for the feedback!

Yes, it's lickety-split now, even in a git repo directory. Maybe the parallel git commands work has made it in recently? In any case, well done!

I'm adding a section in the Clink docs for how to use oh-my-posh with Clink (it just takes a super-tiny script).

from clink.

Maybe the parallel git commands work has made it in recently

We've looked at that and decided it wouldn't do much because subsequent calls are based on context that builds on top of each other. What we did, which came as a suggestion from another contributor, was replace as much git calls as we could towards file system reads inside the .git folder. That made quite a significant impact.

That and command path caching are the two changes which assisted with improving performance on Windows. The effect on unix systems is negligible 😂

from clink.

Aw, there's no way to get the last exit code, so that module isn't possible.

It's only a pseudo-variable, and there's no way to retrieve it's value without running a command (in the current shell; can't just invoke a new shell to get it).

I could cobble together a way to forcibly run a echo %ErrorLevel% > _pipe_handle_ command after every command, and eat the output so that you can't see that Clink is injecting commands into the execution stream. But that has potential for side effects, and I'd want to be very careful about incorporating such an approach. Maybe sometime in the future, after the first official release.

Ok, I don't know how reliably this will work in practice, but I've hooked it up to do exactly that. A new cmd.get_errorlevel setting can make Clink run a hidden echo %errorlevel% command before every interactive prompt and retrieve the last exit code value for use by Lua scripts via a new os.geterrorlevel() function (and os.getenv("errorlevel") also returns it). It will be included in Clink v1.2.14 which will be released sometime in the next week or two.

from clink.

Note on how to repro the problem from point 3:

• Save the following ohmyposh.lua script to your Clink profile directory (or wherever your other Clink scripts are located).

local custom_prompt = clink.promptfilter(5)
function custom_prompt:filter(prompt)
    local p = io.popen("oh-my-posh"):read("*a")
    return p
end

• Start a new Clink instance.
• Enter echo hello
• Enter echo hello very_long_text_that_wraps_onto_a_second_line (the echo hello part needs to be on the first line, and the rest of the text needs to overflow onto a second line).
• Press Up; the command that wrapped should be displayed.
• Press Up; the input text will disappear.

This is because of the \x1b[K near the end of the prompt string produced by oh-my-posh.

The same kind of problem should reproduce in Bash as well, since Bash is also using Readline for its input prompt.

from clink.

@chrisant996 Since I finally decided to go with cmder-powerline-prompt, I saw that that project has been abandoned too :(

So I forked it, fixed some of the issues, merged all the pull requests on the original repo, and even added my own segment (battery) - clink-powerline.

It was a good thing that you fixed the HOME issue. It seems this project relied on it too :)

from clink.

I have some enhancements to the powerline scripts as well. I was going to send a PR soon; I hadn't noticed it was abandoned, but I think cmder has a fork that's maintained.

Maybe I'll fork it and revive it, too.

I've added a pushdir depth module, a date module, and I plan to add an exit code module soon. I was also thinking of enhancing the git module.

And I was thinking of making it read a config file for things like the prompt symbol, so people don't have to edit the scripts (which makes it hard to install updates to the scripts without losing your config). Actually I'd probably make the config file be a lua script, but named something like "powerline.config" and make powerline load it by name.

from clink.

I think cmder has a fork that's maintained.

I couldn't find it. Do you have a link?

Maybe I'll fork it and revive it, too.

That'd be super awesome. I was actually going to create a scoop manifest for my fork (so that it becomes easy to install), but I think I'll wait for your fork now.

And I was thinking of making it read a config file for things like the prompt symbol, so people don't have to edit the scripts.

It already includes a config file, you have to rename _powerline_config.lua.sample to _powerline_config.lua (this is added to gitignore so it doesn't get overwritten).

from clink.

I was also thinking of enhancing the git module.

I actually took inspiration from https://github.com/tadashi-aikawa/owl-cmder-tools for improving the git module, it's much much better.

from clink.

I think cmder has a fork that's maintained.
I couldn't find it. Do you have a link?

Cmder must have a way of modifying the vendor files.
I don't see offhand how Cmder retrieves/packages the vendor files, though.

Maybe I'll fork it and revive it, too.
That'd be super awesome. I was actually going to create a scoop manifest for my fork (so that it becomes easy to install), but I think I'll wait for your fork now.

Cool. Will probably get to it next week.

from clink.

And I was thinking of making it read a config file for things like the prompt symbol, so people don't have to edit the scripts.
It already includes a config file, you have to rename _powerline_config.lua.sample to _powerline_config.lua (this is added to gitignore so it doesn't get overwritten).

Yes but for example tadashi-aikawa's tools provide a patched powerline_git.lua file that changes the icons for some things. That's what I mean -- making those things configurable without having to patch the core scripts. Because if you patch a core script you're effectively forking the code, and then it's not possible to take updates to the core stuff without having to handle merging. I don't think it's a good user experience when end users have to fiddle with merging on every update in order to not break their stuff.

from clink.

Aw, there's no way to get the last exit code, so that module isn't possible.

It's only a pseudo-variable, and there's no way to retrieve it's value without running a command (in the current shell; can't just invoke a new shell to get it).

I could cobble together a way to forcibly run a echo %ErrorLevel% > pipe_handle command after every command, and eat the output so that you can't see that Clink is injecting commands into the execution stream. But that has potential for side effects, and I'd want to be very careful about incorporating such an approach. Maybe sometime in the future, after the first official release.

from clink.

@chrisant996 Since I finally decided to go with cmder-powerline-prompt, I saw that that project has been abandoned too :(

So I forked it, fixed some of the issues, merged all the pull requests on the original repo, and even added my own segment (battery) - clink-powerline.

It seems all the function comment headers got removed from powerline_git.lua somehow in your fork?

from clink.

It seems all the function comment headers got removed from powerline_git.lua somehow in your fork?

Yeah, I think https://github.com/tadashi-aikawa/owl-cmder-tools this person had removed them. Should I add them back?

from clink.

It seems all the function comment headers got removed from powerline_git.lua somehow in your fork?

Yeah, I think https://github.com/tadashi-aikawa/owl-cmder-tools this person had removed them. Should I add them back?

Let's defer that until after I push fixes and charges I've been making in my fork of the cmder-powerline-prompt scripts the past couple of months. I avoided removing the comments when I merged, and I've fixed some mistakes in the new changes from PRs (e.g. it's a bug to make git_root_dir be a global variable; if you cd directly from one repo to another, without first changing to a non-repo directory, it will get confused about the repo root).

from clink.

Let's defer that until after I push fixes and charges I've been making in my fork of the cmder-powerline-prompt scripts the past couple of months.

Sure, I'll just send over any pull requests to merge the changes that I have made to the powerline_git.lua file. Meanwhile, could you incorporate the changes that I have made to other segments, in your fork?

If you cd directly from one repo to another, without first changing to a non-repo directory, it will get confused about the repo root.

It's working fine for me, see below:

from clink.

Look in powerline_prompt.lua starting at line 61. Look at the conditions where gir_root_dir is set vs reused. Look at the substring index math. There is a clear bug, but it will only manifest under certain conditions.

You'll be able to see why it wouldn't manifest when changing between the directories in the screen shot. They share the same parent path. Try switching between two repo dirs under different parent paths of different lengths.

from clink.

I tried to follow your instructions, and got this:

Sorry if I sound nitpicky, but I'm just trying to reproduce the error. Might learn some Lua on the way.

from clink.

Ah, right. See these lines:

                -- if not git dir leave the full path and reset the git_root_dir
                git_root_dir = nil

It says "if not git_dir ... reset the git_root_dir". I took the comment at face value, but it's inaccurate. It always reset the git_root_dir. So while it won't malfunction, there's also no reason for it to be a global variable. Nothing else uses it, and this always clears it (and caching it wouldn't gain any performance benefit anyway). The code has some unnecessary complexities, making it costly to reason through what behaviors will occur.

Without the unconditional clearing of git_root_dir (which defeats the rest of the code that is trying to cache the value between calls), appended_dir could become mangled:

-- Suppose git_root_dir is "c:\abc", left over from a previous repo.
-- Suppose git_dir is "c:\12345678".
-- Suppose cwd is "c:\12345678\xyz".
Line 74:
local appended_dir = string.sub(cwd, string.len(git_root_dir) + 1)
-- Then appended_dir becomes:
-- string.sub("c:\12345678\xyz", string.len("c:\abc") + 1)
-- string.sub("c:\12345678\xyz", 7)
-- "45678\xyz"
Line 75:
cwd = get_folder_name(git_root_dir)..appended_dir
-- Then cwd becomes:
-- get_folder_name("c:\abc").."45678\xyz"
-- "abc".."45678\xyz"
-- "abc45678\xyz"
-- Which would be a jumble caused by an index math error
-- caused by an out of date cached variable.
-- But it ends up being ok because at the end there's an
-- issue that defeats the attempted caching, so the latent
-- bug is avoided in the current state of the code.

Anyway, I'm happy to discuss the various changes once I push them. :)

from clink.

Wow, thanks for the detailed explanation!

So in essence, the current implementation prevents malfunction, but also does not cache anything (even if it was supposed to), right?

from clink.

Wow, thanks for the detailed explanation!

So in essence, the current implementation prevents malfunction, but also does not cache anything (even if it was supposed to), right?

Yes. I just pushed my current set of changes; you can compare the code in my fork vs the code in your fork. Next I'm going to pick up the maven and python stuff and update those for the new API as well.

from clink.

Will you be porting the battery module (optional, off by default) too?

Since I have a laptop with weak battery life, that's one thing I absolutely need. I even ended up modifying the original oh-my-posh project (PowerShell one, not the Go one) just for this 😅

from clink.

Yes. And adding optional color thresholds and visibility threshold.

I backed out the HOME change, though. It's both unnecessary and mistaken. I added comments in that original PR explaining further.

from clink.

Yes. And adding optional color thresholds and visibility threshold.

Thank you for that!

I backed out the HOME change, though. It's both unnecessary and mistaken. I added comments in that original PR explaining further.

I understand, thanks again!

Also, may I suggest renaming your fork to something like 'clink-powerline' or 'powerline-prompt'? I feel that since Clink can be used outside Cmder too, this will be more apt (like the clink-completions project). I'll delete my fork to prevent any naming conflicts.

from clink.

There are pros and cons to changing the repo name, and therefore the product name.

I don't see a reason to rush into renaming the product.

from clink.

Sure, that's alright too!

from clink.

chrisant996/cmder-powerline-prompt has been updated.

See the revised _powerline_config.lua.sample file for documentation.

The new battery status support should hopefully be what you're looking for. There are multiple configuration settings for it, so be sure to check them all out.

from clink.

Awesome! The separate battery segment is the config I liked most, I went for it.

BTW, I just saw that you added a new function os.getbatterystatus to the Clink API. I did notice that wmic calls slowed down the prompt quite a bit. Will this new function be used in the prompt (of course, after checking Clink version)?

I think the default value here should be 'false' https://github.com/chrisant996/cmder-powerline-prompt/blob/1acfe6d9f13031ce991bba2b5ee6aebbb6d0a7c6/_powerline_config.lua.sample#L41 (it's not putting a newline by default).

Also, is putting the user config file in a static location (like %LOCALAPPDATA% or %USERPROFILE%\.config if you prefer Unix style) a feasible idea? That might help in persisting configurations across installs, and might even solve the issue of loading the config before the other main Lua scripts.

All that aside, eagerly waiting for a (pre)release! (so that scoop manifest can be made) 🎉

from clink.

BTW, I just saw that you added a new function os.getbatterystatus to the Clink API. I did notice that wmic calls slowed down the prompt quite a bit. Will this new function be used in the prompt (of course, after checking Clink version)?

It already is. Look at the source code from my cmder-powerline-prompt fork. :) It's instantaneous, rather than ~1/2 sec. Process launches are expensive. I also optimized the script a bit even for old Clink, by removing the pipes and doing corresponding processing in Lua (faster than it was, but still slow).

I think the default value here should be 'false' https://github.com/chrisant996/cmder-powerline-prompt/blob/1acfe6d9f13031ce991bba2b5ee6aebbb6d0a7c6/_powerline_config.lua.sample#L41 (it's not putting a newline by default).

Whups, yes. Missed converting that over from settings.get(), thanks! Fixed (and pushed).

Also, is putting the user config file in a static location (like %LOCALAPPDATA% or %USERPROFILE%\.config if you prefer Unix style) a feasible idea? That might help in persisting configurations across installs, and might even solve the issue of loading the config before the other main Lua scripts.

The load order problem was solved by rewriting how the other scripts access the config variables. Details are in the commit history in my fork of cmder-powerline-prompt.

re: the suggestion to move the config script to a static location -- Not so much. Moving the file location has the same drawbacks renaming the file. It breaks backward compatibility and can easily actually totally break things if the user doesn't delete their existing config file (it's not a data config file, it's an actual code script, so having two would be highly problematic, especially since the old one has a name that comes last in the load order and therefore will override any attempts at configuring via the new file name).

All that aside, eagerly waiting for a (pre)release! (so that scoop manifest can be made) 🎉

Just in case it wasn't clear, the issues were already solved. If you're using the Clink 1.1.17 pre-release and the latest from my cmder-powerline-prompt fork then you already have the fixes.

It's very close to an official release, so please keep reporting any issues you do happen to encounter! Thank you for helping it on its way towards an official release. :)

from clink.

BTW, I just saw that you added a new function os.getbatterystatus to the Clink API. I did notice that wmic calls slowed down the prompt quite a bit. Will this new function be used in the prompt (of course, after checking Clink version)?

Just had a thought -- did you update to Clink 1.1.7 and then update the cmder-powerline-prompt scripts was it was already running? If so then the scripts won't be reloaded unless you press Ctrl+X,Ctrl+R.

from clink.

BTW, I just saw that you added a new function os.getbatterystatus to the Clink API. I did notice that wmic calls slowed down the prompt quite a bit. Will this new function be used in the prompt (of course, after checking Clink version)?

Just had a thought -- did you update to Clink 1.1.7 and then update the cmder-powerline-prompt scripts was it was already running? If so then the scripts won't be reloaded unless you press Ctrl+X,Ctrl+R.

The scoop manifest for Clink hasn't been updated yet on the bucket, so I'm still on 1.1.16.

from clink.

I'm thinking of making a list of some clink script collections. The list could be embedded in the docs and/or in the Clink web site. But it looks a little lean when I only know of two: clink-completions and cmder-powerline-prompt. 😜

from clink.

I only know of two: clink-completions and cmder-powerline-prompt.

Haha, same. But coming to think of it, both are super useful and succinct.

from clink.

@chrisant996 Might be unrelated, but can I ask a question?

Is there a system call (that you implemented in Clink) for getting battery status through C++? The PowerShell method I know of also queries the WMI, and is likewise slower. The improvement in 1.1.17 with os.getbatterystatus is noticeably huge. I tried reading the 'clink/lua/src/os_api.cpp' but it just includes structs and function calls that I could find a reference for.

from clink.

Is there a system call (that you implemented in Clink) for getting battery status through C++?

Googling "getting battery status through C++" finds it. ;)

I tried reading the 'clink/lua/src/os_api.cpp' but it just includes structs and function calls that I could find a reference for.

Maybe you were looking at an older revision of the file? I don't know how else searching for "getbatterystatus" in the file wouldn't find the call.

from clink.

Thanks, found it! Apparently it's a function defined in winbase.h

from clink.

Oh, snap. Cmder has an alternative version of cmder-powerline-prompt built into its own custom clink.lua script. Ugh...

UPDATE: oh my goodness, it's both better and worse: Cmder has built in scripts for its own non-powerline prompt. But installing the cmder-powerline-prompt scripts doesn't prevent Cmder's built in scripts for running. So building the prompt takes about 2x longer than it needs to! 😢

from clink.

But installing the cmder-powerline-prompt scripts doesn't prevent Cmder's built in scripts for running.

Yeah I noticed this too a couple days ago, when I installed Cmder for getting new screenshots for the improved fork. Even after setting clink.path, Cmder's own prompt was showing up. I ignored this as I thought this might be due to conflicting installation of two different Clink versions and scripts.

from clink.

But installing the cmder-powerline-prompt scripts doesn't prevent Cmder's built in scripts for running.

Yeah I noticed this too a couple days ago, when I installed Cmder for getting new screenshots for the improved fork. Even after setting clink.path, Cmder's own prompt was showing up. I ignored this as I thought this might be due to conflicting installation of two different Clink versions and scripts.

I didn't mean Cmder's prompt would show up. I meant it computes its prompt string, including running git and etc, and then the powerline scripts run and discard + replace the results with their own results.

It sounds like maybe the powerline scripts weren't even being loaded, if they were having no effect. Or maybe while picking up the changes from your fork I somehow accidentally made it stop discarding the Cmder prompt.

I was calling out a performance issue. You seem to be alluding to a malfunction. Is more information available about what went wrong and how it was configured?

from clink.

1. oh-my-posh is pretty slow.

@chrisant996 glad to say I found the culprit. It seems the YouTube segment (that does a GET request on localhost) behaved rather differently on Windows vs other platforms. Go most likely has a different default timeout on Windows and other platforms, causing the segment to take up to 2 seconds when it could not reach the endpoint. This has been set to 50ms (sufficient for the use-cases) which should put everything back to normal.

from clink.

Yeah I noticed this too a couple days ago, when I installed Cmder for getting new screenshots for the improved fork. Even after setting clink.path, Cmder's own prompt was showing up. I ignored this as I thought this might be due to conflicting installation of two different Clink versions and scripts.

Never mind this, I think I wasn't following the cmder instructions properly.

from clink.

@chrisant996 BTW I found one more (possible) problem with cmder-powerline. I've opened an issue in your fork for the same.

from clink.

@chrisant996 we've secretly done some more improvements, which hopefully resort to a good experience on Windows. Thanks for the feedback!

from clink.