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

implement generic job_completed event #4319

Closed
nathanblair opened this issue Aug 12, 2017 · 9 comments · Fixed by #6959
Closed

implement generic job_completed event #4319

nathanblair opened this issue Aug 12, 2017 · 9 comments · Fixed by #6959

Comments

@nathanblair
Copy link

Hey fishers,

I was putting together a config.fish and wanted to include some functionality that could benefit from overriding the job-finished hook/event.

In the documentation on fishshell.com there is a sample function in the function section showing some function notify .... It defines a function function _notify_job$job --on-job-exit $job --inherit-variable job ... but I can't find any documentation regarding any job-exit hooks or events.

When I copied the function into my own config.fish file I did not notice any of the behaviors I might have expected to happen, notably an \a beep being sent when a job was completed. My process looked like this:

> sleep 15 &
> sleep 15 &
> sleep 15 &

...all executed consecutively (within 1 second). Of course, after 15 seconds, I started receiving notifications in my prompt that jobs were being finished (Job X, "sleep 15 &" has ended).

I'd like to tap into (override) the print statements that get issued when these jobs are finished to write something that fits my theme a little better. Am I missing documentation that explains how to do this? From what I've seen the only builtin events I can hook are pre-exec, post-exec, and the prompt.

Thanks for any input!

@krader1961
Copy link
Contributor

There is no way to override the message displayed when a background job completes. It's currently hardcoded in the C++ code; specifically in src/proc.cpp. If you think you can make a convincing argument for why that should be configurable we might consider it. But "fits (your) theme a little better" does not strike me as a compelling argument.

See man function for a list of events that fish will send. Event names however are just arbitrary strings. You can send you own named event using the emit command.

Technically the --on-job-exit flag does register an interest in a specific event (see the symbol EVENT_JOB_ID in the source) but because of how that event works you can't register an interest in it using the function --on-event flag.

@nathanblair
Copy link
Author

Yeah, I ran a check just now and realized jobs was actually builtin, not in the fish functions list. Which is fine, its understandable why its there. However, I wanted to point out (and forgot to when I wrote the report) that another reason I opened an issue was because in the fishshell.com Documentation there is a section on Event Handlers that explicitly states an event handler exists for "when a process or job exits".

Now, maybe the job exit event does actually exist - it would still be useful, I think, to be able to hook into that even if I couldn't override the stock job ending behavior. If it doesn't, I might suggest removing that line from the fishshell.com documentation so as not to confuse newcomers like myself trying to familiarize ourselves with fish's (already great) capabilities!

@krader1961
Copy link
Contributor

... there is a section on Event Handlers that explicitly states an event handler exists for "when a process or job exits".

Technically that statement is correct but I agree it is misleading. Because the event does not have a name you have to use function --on-job-exit rather than function --on-event. So the documentation should definitely be clarified. Thanks for pointing that out. We depend heavily on new fish users to bring these things to our attention so thanks for going to the trouble of opening this issue.

It might be worth creating a generic event name for when jobs complete. That would allow you to create a function that has $argv set to the job number, process group id, and job string (what jobs would show). If we did that we should probably modify the code in src/proc.cpp to only write its default hardcoded message if no function is listening for the job completion event. This seems like a niche feature that very few people would use. So I'm not going to spend my weekend implementing it 😄 Nonetheless, the idea has enough value that I'll leave this open as an enhancement request. If someone like yourself, @nathanblair, created a change for us to review we'd be happy to do so; otherwise it could be several years before someone tackles this enhancement.

@krader1961 krader1961 added this to the fish-future milestone Aug 12, 2017
@krader1961 krader1961 changed the title Documentation for Job Ending Hooks/Handler implement generic job_completed event Aug 12, 2017
@nathanblair
Copy link
Author

Right. It might be something I try to peek into at some point in the semi-near future but as you said it'll probably only ever be a handful that might exploit that new capability. It sounds to me like this issue/enhancement is similar to prepending the prompt mode which, coincidentally, I was also dealing with earlier today!

Thanks for your time and consideration; if you are okay with keeping this open for a possible enhancement in the future then I'll not close it either.

@krader1961
Copy link
Contributor

Let's leave it open. I think there is merit in the idea just not enough to make it likely any of the handful of core contributors will implement this in the near future. Which means that if you want it implemented sooner rather than later you'll probably have to do the work yourself, @nathanblair. Note that this should be a straightforward change once you get acquainted with the code and assuming you have minimal C++ skills.

@soumya92
Copy link
Contributor

soumya92 commented Mar 26, 2020

I think this would help both #2727 and #1018. It seems to me that this is just a matter of someone actually implementing the feature, and the feature itself is not contentious. Is that a reasonable reading of the current status?

That is, would the fish-shell maintainers accept a pull request that changed the current behaviour of printing the hardcoded string to instead call a function (fish_job_end_summary or something), with a default implementation of that function producing the same output?

Edit: as written earlier in #4319 (comment), except that instead of enumerating listeners, we would just move the print statement into the default function, since I see this as similar to fish_greeting or fish_title: sensible default out of the box, with the option to override by redefining.

If so, I would be wiling to take a look; on the surface it doesn't seem too complicated.

@soumya92
Copy link
Contributor

soumya92 commented Mar 26, 2020

Also, what do people think about handling the "terminated by signal" message the same way: fish_job_terminate_summary or similar? Or another argument to the same fish_job_end_summary? Or is leaving that hardcoded message in fine?

@zanchey
Copy link
Member

zanchey commented Apr 16, 2020

That is, would the fish-shell maintainers accept a pull request that changed the current behaviour of printing the hardcoded string to instead call a function (fish_job_end_summary or something), with a default implementation of that function producing the same output?

Yes, I think we'd take something along those lines. The current strings are translated, I think, but it should be possible to do that from the function.

@soumya92
Copy link
Contributor

Great! I'll try to put something together for review.

The current strings are translated, I think, but it should be possible to do that from the function.

Yeah, I think I've seen _ calls in fish functions, so hopefully that works the way I expect.

@krobelus krobelus modified the milestones: fish-future, fish 3.2.0 May 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants