fish_mode_prompt should not always be prepended to fish_prompt #3641

Closed
andrew-kennedy opened this Issue Dec 12, 2016 · 10 comments

Comments

Projects
None yet
6 participants
@andrew-kennedy

When fish is in vi_mode, an ugly hack is required to move the position of the fish_mode_prompt since it is always called before fish_prompt in reader.cpp. You must define fish_mode_prompt as an empty function in config.fish and create another function with the same body to call where you please, whether it be somewhere within fish_prompt or maybe in fish_right_prompt.

As a more flexible alternative, fish_mode_prompt should be called from within the default fish_prompt, and thus more easily customizable for users building their own prompts.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Dec 12, 2016

Member

There is one thing here we should ensure: The mode_prompt should be strictly opt-out. If a user didn't take an explicit action to disable it, it should be displayed.

This is because being unsure which vi-mode you are in is rather confusing, and not knowing that you are in vi-mode at all is even more so. Which is mainly an issue because there's a variety of third-party prompts that "require" vi-mode (for some reason).

That means just adding it to our prompts isn't really an option. We might be able to use more complicated logic - maybe only show it if it hasn't been customized - but that might also be confusing.

I'm open to suggestions.

Member

faho commented Dec 12, 2016

There is one thing here we should ensure: The mode_prompt should be strictly opt-out. If a user didn't take an explicit action to disable it, it should be displayed.

This is because being unsure which vi-mode you are in is rather confusing, and not knowing that you are in vi-mode at all is even more so. Which is mainly an issue because there's a variety of third-party prompts that "require" vi-mode (for some reason).

That means just adding it to our prompts isn't really an option. We might be able to use more complicated logic - maybe only show it if it hasn't been customized - but that might also be confusing.

I'm open to suggestions.

@faho faho added this to the fish-future milestone Dec 12, 2016

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 12, 2016

Contributor

I agree with @faho that we need to retain the current default behavior of prepending the fish_mode_prompt output. Changing that behavior is not backward compatible.

It would be a bit of a hack but what if fish recognized a magic annotation on the function? The user would add it to indicate they intend to invoke it within one of the prompt functions. Perhaps via a magic function description like this:

function fish_mode_prompt --description "called from fish_right_prompt"
   # do stuff
end

Our exec_prompt() function then not only checks that a fish_mode_prompt function exists but its description is not called from fish_prompt or called from fish_right_prompt. Any other description results in the current behavior of prepending its output to the prompt.

Contributor

krader1961 commented Dec 12, 2016

I agree with @faho that we need to retain the current default behavior of prepending the fish_mode_prompt output. Changing that behavior is not backward compatible.

It would be a bit of a hack but what if fish recognized a magic annotation on the function? The user would add it to indicate they intend to invoke it within one of the prompt functions. Perhaps via a magic function description like this:

function fish_mode_prompt --description "called from fish_right_prompt"
   # do stuff
end

Our exec_prompt() function then not only checks that a fish_mode_prompt function exists but its description is not called from fish_prompt or called from fish_right_prompt. Any other description results in the current behavior of prepending its output to the prompt.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Dec 14, 2016

Member

We could imagine having fish_mode_prompt be a trampoline function to a backing implementation. This way you could redefine fish_mode_prompt yet still call the backing implementation.

In the mean time, note that functions --copy lets you copy fish_mode_prompt, so it's not quite as bad as having to manually cut and paste the implementation.

Member

ridiculousfish commented Dec 14, 2016

We could imagine having fish_mode_prompt be a trampoline function to a backing implementation. This way you could redefine fish_mode_prompt yet still call the backing implementation.

In the mean time, note that functions --copy lets you copy fish_mode_prompt, so it's not quite as bad as having to manually cut and paste the implementation.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 16, 2016

Contributor

Making fish_mode_prompt a trampoline function also solves this problem and is effectively what was proposed in the initial comment. The question is whether that solution or my proposal to define a fish_mode_prompt function in a manner that tells fish the user will take care of running it is simpler and clearer.

I recommend the trampoline solution rather than my proposal. I think documenting that solution will result in fewer questions and mistakes for a typical fish user.

Contributor

krader1961 commented Dec 16, 2016

Making fish_mode_prompt a trampoline function also solves this problem and is effectively what was proposed in the initial comment. The question is whether that solution or my proposal to define a fish_mode_prompt function in a manner that tells fish the user will take care of running it is simpler and clearer.

I recommend the trampoline solution rather than my proposal. I think documenting that solution will result in fewer questions and mistakes for a typical fish user.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Dec 16, 2016

Member

Sounds good! Maybe fish_mode_prompt trampolines to fish_default_mode_prompt. (Or is it the other way around?)

Member

ridiculousfish commented Dec 16, 2016

Sounds good! Maybe fish_mode_prompt trampolines to fish_default_mode_prompt. (Or is it the other way around?)

@krader1961 krader1961 removed the RFC label Dec 17, 2016

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 17, 2016

Contributor

It's the former.

We should introduce a fish_default_mode_prompt function that subsumes the body of fish_mode_prompt. Then have fish_mode_prompt do nothing other than call fish_default_mode_prompt. Users can continue to redefine fish_mode_prompt to get custom behavior including not displaying the vi mode indicator. After this is implemented users can redefine fish_mode_prompt to do nothing and explicitly call fish_default_mode_prompt from their custom prompt functions to place the indicator wherever they wish.

Contributor

krader1961 commented Dec 17, 2016

It's the former.

We should introduce a fish_default_mode_prompt function that subsumes the body of fish_mode_prompt. Then have fish_mode_prompt do nothing other than call fish_default_mode_prompt. Users can continue to redefine fish_mode_prompt to get custom behavior including not displaying the vi mode indicator. After this is implemented users can redefine fish_mode_prompt to do nothing and explicitly call fish_default_mode_prompt from their custom prompt functions to place the indicator wherever they wish.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Dec 18, 2016

Member

Make sense to me.

Member

ridiculousfish commented Dec 18, 2016

Make sense to me.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jan 11, 2017

Member

Last call for 2.5b1 - anyone want to try and get this in?

Member

zanchey commented Jan 11, 2017

Last call for 2.5b1 - anyone want to try and get this in?

@cprieto

This comment has been minimized.

Show comment
Hide comment
@cprieto

cprieto Jan 12, 2017

Contributor

I raise my hand (shyly) and looking at this atm, I am new to the Fish source code but I want to help.

Contributor

cprieto commented Jan 12, 2017

I raise my hand (shyly) and looking at this atm, I am new to the Fish source code but I want to help.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jan 12, 2017

Member

@cprieto: Sure! You'll want to look at share/functions. Every function there needs to be in a file named $funcname.fish, where $funcname is the name of the function.

You'll need to move the code in fish_mode_prompt to a new function, and then call that from fish_mode_prompt. You'll also need to adjust the documentation in doc_src/.

If you have any other questions, feel free to ask here or on https://gitter.im/fish-shell/fish-shell.

Member

faho commented Jan 12, 2017

@cprieto: Sure! You'll want to look at share/functions. Every function there needs to be in a file named $funcname.fish, where $funcname is the name of the function.

You'll need to move the code in fish_mode_prompt to a new function, and then call that from fish_mode_prompt. You'll also need to adjust the documentation in doc_src/.

If you have any other questions, feel free to ask here or on https://gitter.im/fish-shell/fish-shell.

@cprieto cprieto referenced this issue Jan 12, 2017

Merged

Added default mode prompt function #3727

1 of 2 tasks complete

faho added a commit that referenced this issue Jan 12, 2017

Added default mode prompt function (#3727)
* Added new function for the default prompt mode

Now fish mode prompt will call fish_default_mode_prompt, this will solve #3641

* Added function description

* Change wording for documentation about default mode prompt

* Finish changes requested in code review

@krader1961 krader1961 closed this Jan 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment