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

Custom prompt hooks protected from later overwriting #952

Merged
merged 6 commits into from
Oct 19, 2016

Conversation

Jackbennett
Copy link
Contributor

@Jackbennett Jackbennett commented May 12, 2016

In reference to issue #950 about PR #951 I have the following suggestion.

@centur I like your suggestion, I'm just uneasy out by the idea of running any function that is called User-PrePrompt, User-Prompt, User-PostPrompt as the prompt reloads. After trying that out I wasn't enthused to find the following idea has some fun effects.

function Write-Host {'Hello Boys, I'm Back'}

So with that, I propose:

  • Add a pre and post function hook around the Cmder prompt.
  • Specify the cmder prompt as a function that could be replaced by a user.
  • Write a friendly message when the user profile template is created.
  • Create the user profile with cmder prompt hooks ready to use.

Core functions have been explicitly called from their namespace like Microsoft.PowerShell.Utility\Write-Host to try and prevent clobbering with later defined functions.

User supplied functions are variables in as script blocks, created as the session runs the profile script. By creating them as constants these function names cannot be re-declared for the duration of the process.

Since the prompt function already exists by this time, set the readOnly flag so to re-declare the prompt requires the use of -force.

It is hoped these changes limit what could be the risk of any script redefining functions that are called automatically without user intent or input.

Add a pre and post function hook around the Cmder prompt.
Specify the cmder prompt as a function that could be replaced by a user.
Write a friendly message when the user profile template is created.
Create the user profile with cmder prompt hooks ready to use.

It was concerning to run any function with a specific name every prompt
with no guarantee it remains what it was initally created as.

Core functions have been explicitly called from their
namespace like Microsoft.PowerShell.Utility\Write-Host to try and prevent
clobbering.

User supplied functions are passed in as script blocks, created as the
session runs the profile script. By creating them as constants these
function names cannot be declared again for the duration of the process.

Since the prompt function already exists by this time, set the readOnly
flag so to re-declare the prompt requires the use of -force.

It is hoped these changes limit what could be the risk of any script
redefining functions that are called automatically without user intent or
input.
@Jackbennett
Copy link
Contributor Author

Jackbennett commented May 12, 2016

centur:

Invoke-Expression doesn't add much overhead compared to the direct function call. I quickly smoke measured it with Measure-Command before PR and saw that it adds about
0.001 to 0.003 secs compared to direct invocation of the function which measured, with an empty function, to ~0.008)

This is super interesting I'll experiment this. I haven't perf tested this PR yet it's welcome to changes.

@jankatins
Copy link
Contributor

There is also the idea to set the prompt in the clink lua code. at least for teh cmd path, this prevents the problem that you get a garbeled prompt when you call a new cmd:

c:\data\external\R\IRkernel (envs)
λ cmd
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation. Alle Rechte vorbehalten.

c:\data\external\R\IRkernel {git}{hg}
{lamb}

This problem really annoys me when I have conda build files which echo the commands they are using to build a package...

Not sure of that can be used for PS as well...

This is what I use (but the above is actually happening due to the changed prompt in init.bat -> That would have to be removed...):

---
 -- after conda activate: reset prompt 
---
function reset_prompt_filter()
    -- reset to original, e.g. after conda activate destroyed it...
    if string.match(clink.prompt.value, "{lamb}") == nil or not string.starts(clink.prompt.value,"\x1b[") then
        -- orig: $E[1;32;40m$P$S{git}{hg}$S$_$E[1;30;40m{lamb}$S$E[0m
        -- color codes: "\x1b[1;37;40m"
        cwd = clink.get_cwd()
        prompt = "\x1b[1;32;40m{cwd} {git}{hg} \n\x1b[1;30;40m{lamb} \x1b[0m"
        new_value = string.gsub(prompt, "{cwd}", cwd)
        clink.prompt.value = new_value
    end
end

clink.prompt.register_filter(reset_prompt_filter, 10)

@centur
Copy link

centur commented May 12, 2016

Cool, I like your way more than my proposed hack. I haven't put much thinking into side effects and robustness, just sought some ways to preserve my prompt customization outside of profile.ps1. Happy to pass this feature initiative to you.

Side note - I played a bit with simple Stopwatch timing inside profile.ps1 and found that it's pretty slow, on my laptop it runs for almost 2 seconds - very noticeable for a fast and lightweight console. Removing some backward-compatibility bits and modules I shaved off more than a second from a console start time. It might be worth to do some testing and restructure it a bit - so an interested user can enable\disable certain features (e.g. posh-git is the beast) from his own profile and improve PowerShell start experience.
I wish someone who knows how these things are tied together would do the same for pre-posh scripts - because even with very lightweight configuration new console doesn't start immediately, start delay is very noticeable, and it's a painful experience on Windows.

@Jackbennett
Copy link
Contributor Author

Those . .\imports can get killed on the profile startup. They're only needed for W7 so we should really do some feature detection for that. Even so, PS still isn't an instant-on thing which I do find very annoying. But I pretty much never close PS.

My checkGit function in there should fend off posh-git as long as your startup dir isn't a git directory and needs it.

@JanSchulz clink isn't injected at all for powershell it doesn't need it. That was my point behind the failed move to PS Default because we keep getting really weird issues because of clink.

@jankatins
Copy link
Contributor

jankatins commented May 12, 2016

clink isn't injected at all for powershell it doesn't need it.

Ok, didn't know, I only use cmd :-) Sorry for the noise!

@Jackbennett
Copy link
Contributor Author

I don't use cmd, it's nice to hear where we've had similar problems. Never sorry needed for jumping in.

@centur
Copy link

centur commented May 13, 2016

@Jackbennett disabling posh-git itself is a good step towards speeding up as it's slows down the profile boot time. I'd rather have some explicit way to load it (I have it anyway Import-Module posh-git) into a given console than to load it everywhere... Let's see how this PR goes through and then see how we can optimise other steps.
Having 3-10 seconds startup time for a new console is not a great experience. It might be a powershell fault by itself, but it's extremely sad

Import-Git now finds if the module isn't installed at all and alerts the
user. But only when in a git folder.
@Jackbennett
Copy link
Contributor Author

Latest commits will require this posh-git PR landing and a module update.

Just noticed Post-git now warn they don't support PS2.0 So this might be a bigger pain than we like. If PS2.0 is a big deal for us we'll have to have our own fork of Posh-Git.

@Jackbennett
Copy link
Contributor Author

PR has been merged in :) Just need a release to be wrapped up for whenever they do that.

@Jackbennett
Copy link
Contributor Author

Might as well tackle #528 while I'm at it. Brings PS more into line with cmd.

@Jackbennett
Copy link
Contributor Author

How do you poke CI to run again? That looks like some download fail. @MartiUK

@jankatins
Copy link
Contributor

Close and reopen or run git commit --amend followed by a git push -f

Unfortunately doesn't apply to conEmu's tab name.
@MartiUK
Copy link
Member

MartiUK commented Oct 9, 2016

Will we need to include something to tell users to update posh-git @Jackbennett? I foresee people having broken git statuses if they haven't.

As the prompt function is called all the time, specifically namespace the
cmldets it uses to avoid them being hijacked in the user session.
Posh-Git before this release does not export `Write-VcsStatus` thus
powershell's autoloading cannot find the reqired module for the function.

note that `get-module -listAvailable` can return an array of multiple
versions.
@MartiUK
Copy link
Member

MartiUK commented Oct 10, 2016

Okay to merge @Jackbennett ?

@Jackbennett
Copy link
Contributor Author

Jackbennett commented Oct 10, 2016

Yes I've tested what I can think of without any issues. Be nice for more people to have a look now.

@MartiUK MartiUK merged commit 5597aa6 into cmderdev:development Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants