franciscolourenco / done

A fish-shell package to automatically receive notifications when long processes finish.
MIT License
781 stars 71 forks source link

Executing function on start #12

Closed franciscolourenco closed 7 years ago

franciscolourenco commented 7 years ago

Current version defines multiple functions which need to be loaded in order for the plugin to work. Fish however, only loads automatically the function which name matches the file's name (_done in this case).

One option would be to define all the functions inside a main function, but then, the main function needs to also be executed when the shell is started, not only loaded.

@jbucaran Any idea on how to work around this?

Example:

function _done

    set _initial_window_id ''

    function _get_window_id
        if _is_available lsappinfo
            lsappinfo info -only bundleID (lsappinfo front) | cut -d '"' -f4
        else if _is_available xprop
            xprop -root 32x '\t$0' _NET_ACTIVE_WINDOW | cut -f 2
        end

    end

    function _is_terminal_focused
        test $_initial_window_id = (_get_window_id)
    end

    function _is_available
        type -q $argv
    end

    function _started --on-event fish_preexec
        echo "_ended"
        set _initial_window_id (_get_window_id)
    end

    function _ended --on-event fish_prompt
        echo "_ended"
        if test $CMD_DURATION
            # Store duration of last command
            set duration (echo "$CMD_DURATION 1000" | awk '{printf "%.3fs", $1 / $2}')
            set notify_duration 10000

            if begin
                    test $CMD_DURATION -gt $notify_duration  # longer than notify_duration
                    and not _is_terminal_focused  # terminal or window not in foreground
                end

                set -l title "Finished in $duration"
                set -l message "$history[1]"

                if _is_available terminal-notifier  # https://github.com/julienXX/terminal-notifier
                    terminal-notifier -message "$message" -title "$title" -sender "$_initial_window_id"

                else if _is_available osascript  # AppleScript
                    osascript -e "display notification \"$message\" with title \"$title\""

                else if _is_available notify-send # Linux notify-send
                    notify-send --icon=terminal --app-name=terminal "$title" "$message"

                else  # anything else
                    echo -e "\a" # bell sound
                end

            end
        end
    end
end
jorgebucaran commented 7 years ago

Hey @aristidesfl.

I think this plugin needs some refactoring. First, the following functions:

_get_window_id
_is_terminal_focused
_is_available

Split them apart into separate files (file name and function name must match). Use a better prefix, like _done_FUNCTION_NAME, e.g, _done__is_terminal_focused, and so on.

Then, the following functions:

_started
_done

Again, better to go with _done_started. _done is probably fine. Should go into a snippet.

Make a directory conf.d at the root of the plugin's directory and create a file done.fish. The name of the file is irrelevant, fish simply sources the contents of all the files it finds inside ~/.config/fish/conf.d during shell start, which is where this file will be placed by fisherman when the plugin is installed.

mkdir conf.d
touch conf.d/done.fish
$EDITOR conf.d/done.fish

Inside that file, add the definitions of _done_started and _done. Also initialize _initial_window_id to an empty string.

By the way, declare the scope of the variable and add a prefix too.

set -g _done_initial_window_id ""

I think that should do it. Let me know if something is missing or didn't work.

franciscolourenco commented 7 years ago

Didn't know multiple function files were supported by fisherman. Also didn't know about conf.d.

Two questions:

  1. Why not define _done_finished and _done_started in separate function files like the other functions too?
  2. In alternative why not define all the functions in conf.d/done.fish?
jorgebucaran commented 7 years ago

@aristidesfl

Didn't know multiple function files were supported by fisherman. Also didn't know about conf.d.

Reminds me that I need to write a fish plugin authorship handbook.

Why not define _done_finished and _done_started in separate function files like the other functions too?

Good question. That's Because those functions register events, and the event emitter has no mechanism to determine whether a function in a file is a listener or not without sourcing the file first, so fish does nothing. Since we know those two are event listeners, we define them in the snippet.

In alternative why not define all the functions in conf.d/done.fish?

For the same reason you should not use alias in your config. If you put those functions in the snippet, then they will be eagerly loaded, completely missing the point of fish excellent lazy loading mechanism.

franciscolourenco commented 7 years ago

Good question. That's Because those functions register events, and the event emitter has no mechanism to determine whether a function in a file is a listener or not without sourcing the file first, so fish does nothing. Since we know those two are event listeners, we define them in the snippet.

If we name the function according to the file where it is saved, they will be loaded and the event hooks will still work, no?

For the same reason you should not use alias in your config. If you put those functions in the snippet, then they will be eagerly loaded, completely missing the point of fish excellent lazy loading mechanism.

https://github.com/jbucaran/fish-shell-cookbook/blame/master/README.md#L698 You mention aliases are not available for new shells, but that doesn't seem to be the case, unless you only set them for the login shell, right?

jorgebucaran commented 7 years ago

If we name the function according to the file where it is saved, they will be loaded and the event hooks will still work, no?

No. The event emitter will fail (edit: not fail just won't do anything) to call the --on-event listener function because such function is not loaded in the session and will never be unless, of course, you source the file in which it is saved or invoke it interactively causing it to be lazy-loaded.

You mention aliases are not available for new shells, but that doesn't seem to be the case.

This means they do not persist.

> alias x "echo ok"
> x
> ok
> fish
> x
> fish: Unknown command 'x'
franciscolourenco commented 7 years ago

No. The event emitter will fail (edit: not fail just won't do anything) to call the --on-event listener function because such function is not loaded in the session and will never be unless, of course, you source the file in which it is saved or invoke it interactively causing it to be lazy-loaded.

But this is the way _done has been working so far, and correctly. Are we talking about the same thing? https://github.com/fisherman/done/blob/0.3.0/_done.fish#L1

This means they do not persist.

I though the cookbook was talking about defining aliases in ~/.config/fish/config.fish. I wouldn't expect an alias defined in the terminal to get persisted across sessions any more than a function, without using funcsave. Also because "~/.config/fish/config.fish" is referred in the end. Maybe rephrasing a bit would make things more clear?

franciscolourenco commented 7 years ago

On a related topic, how does fisherman handle updated of scripts where the file names have changed. If I create new files with different names, will the old file stay installed when the user fisher update ?

jorgebucaran commented 7 years ago

But this is the way _done has been working so far, and correctly. Are we talking about the same thing?

fish_prompt is special, because it's already available at the start of the shell. My advice applies to custom events or functions that are not loaded or those that you have no way to know whether they are loaded or not. Even if using fish_prompt, I would still use conf.d because it's the most appropriate, "fishy" way to do it.

My guide does not talk about snippets yet, and it seems a lot of people don't know about them.

I wouldn't expect an alias defined in the terminal to get persisted across sessions any more than a function, without using funcsave.

Me neither. The link just recommends you to not use then in your config.fish because it doesn't make sense and explains why. And yeah, I haven't had time to foolproof read the document, getting there tho hehe.

franciscolourenco commented 7 years ago

fish_prompt is special, because it's already available at the start of the shell. My advice applies to custom events or functions that are not loaded or those that you have no way to know whether they are loaded or not. Even if using fish_prompt, I would still use conf.d because it's the most appropriate, "fishy" way to do it.

But this will incur in slower initialisation like you mentioned with aliases correct? I'm only using native events in this case (pre-exec and prompt) On what basis do you suggest this to be the most "fishy" way to do it?

My guide does not talk about snippets yet, and it seems a lot of people don't know about them.

With snippets you mean the initialisation scripts in conf.d?

jorgebucaran commented 7 years ago

But this will incur in slower initialisation like you mentioned with aliases correct?

There's been a misunderstanding. Let me explain it one more time. See, this function:

function _started --on-event fish_preexec
    set _initial_window_id (_get_window_id)
end

If you put that function in ~/.config/fish/functions/_stated.fish you'd think it would be called before fish runs a command, correct? But it won't.

Why? Because it's not loaded in the session.

screen shot 2017-02-07 at 4 04 32

See the screenshot above. Do you understand what's going on now?

The only reason you see "HELLO" later on is because I forced the hello function to be loaded in the session by calling it myself. Before that, every time I pressed enter (causing fish_prompt to run) didn't emit the event (didn't call hello).

With snippets you mean the initialisation scripts in conf.d?

Yup. That's the "official" name. It's derived from the same terminology used in some Linux distro or soemthing.

franciscolourenco commented 7 years ago

If you put that function in ~/.config/fish/functions/_stated.fish you'd think it would be called before fish runs a command, correct? But it won't.

This used to work, since that is how it was working until done v0.3. However now it doesn't seem to work anymore indeed. Could it be related to the fish version?

jorgebucaran commented 7 years ago

Nope, it has always been like that. At least as far as I can remember since >=2.2.

Perhaps you had the function loaded in your session and never realized 🤔

franciscolourenco commented 7 years ago

This doesn't make any sense. I just rolled back to v2.1.2 and indeed not working. But how do you explain all the people who installed the plugin and had it working?

jorgebucaran commented 7 years ago

I can only speculate. If we think hard enough I'm sure we could come up with a hypothesis. Do you find it so odd? hehe

EDIT: The function needs to be loaded only once. Perhaps they somehow loaded the function and experienced odd behavior from time to time, but never really paid too much attention.

franciscolourenco commented 7 years ago

Moving forward, did you answer this question?

On a related topic, how does fisherman handle updated of scripts where the file names have changed. If I create new files with different names, will the old file stay installed when the user fisher update ?

I'm thinking about putting the following in conf.d/done.fish. It's just one extra function in the snippets, and think it doesn't warrant spreading the code over multiple files.

function __done_get_window_id
  if type -q lsappinfo
    lsappinfo info -only bundleID (lsappinfo front) | cut -d '"' -f4
  else if type -q xprop
    xprop -root 32x '\t$0' _NET_ACTIVE_WINDOW | cut -f 2
  end
end

function __done_started --on-event fish_preexec
  set -g __done_initial_window_id (__done_get_window_id)
end

function __done_ended --on-event fish_prompt
  if test $CMD_DURATION
    # Store duration of last command
    set duration (echo "$CMD_DURATION 1000" | awk '{printf "%.3fs", $1 / $2}')
    set notify_duration 10000

    if begin
        test $CMD_DURATION -gt $notify_duration  # longer than notify_duration
        and test $__done_initial_window_id != (__done_get_window_id)  # terminal or window not in foreground
      end

      set -l title "Finished in $duration"
      set -l message "$history[1]"

      if type -q terminal-notifier  # https://github.com/julienXX/terminal-notifier
        terminal-notifier -message "$message" -title "$title" -sender "$__done_initial_window_id"

      else if type -q osascript  # AppleScript
        osascript -e "display notification \"$message\" with title \"$title\""

      else if type -q notify-send # Linux notify-send
        notify-send --icon=terminal --app-name=terminal "$title" "$message"

      else  # anything else
        echo -e "\a" # bell sound
      end

    end
  end
end
jorgebucaran commented 7 years ago

It's just one extra function in the snippets, and think it doesn't warrant spreading the code over multiple files.

Yeah, I can see where you're coming from.

On a related topic, how does fisherman handle updated of scripts where the file names have changed. If I create new files with different names, will the old file stay installed when the user fisher update ?

Good question. We do nothing, so old files will stay in the plugin directory. Maybe we should delete them.

franciscolourenco commented 7 years ago

Yeah, because I remember specifically testing, when I renamed done to _done, there were conflicts. Even though I cannot reproduce it anymore, I'm afraid people will start getting multiple notifications.

How would you implement something like that? Currently fisherman only copies files existing in master right? It doesn't even use tags to update? Have you considered requiring release tags?

jorgebucaran commented 7 years ago

It does tags IIRC, or perhaps that's only fin.

Can't remember right without looking at the code cause it's just not used so often.

EDIT: I think it's in.

franciscolourenco commented 7 years ago

Is fin a replacement for fisherman? What is the idea?

EDIT: I think it's in.

Fisherman seems to fetch master's HEAD even without tags, no?

jorgebucaran commented 7 years ago

@aristidesfl fin is an another plugin I made after fisherman. The story is more complicated than that, but I'll spare you the details.

fin has the cleanest and best design considering size and number of features I still managed to support.

Fisherman handles more edge cases of course, supports oh-my-fish plugins more thoroughly and can query the remote org index. However, both download and install plugins concurrently.

Fisherman seems to fetch master's HEAD even without tags, no?

For self-update yes. When you install a package I am sure support for tags was included/request by @colstrom IIRC.

franciscolourenco commented 7 years ago

For self-update yes. When you install a package I am sure support for tags was included/request by @colstrom IIRC.

I just tested this once again, and fisher update still pulls from the tip of master even if there no tag.

jorgebucaran commented 7 years ago

@aristidesfl You need to specify the branch when installing, see the code here.

franciscolourenco commented 7 years ago

@jbucaran I didn't intend to install specific tags, but prevent lose commits from being installed until a new release is created. The workaround it not committing directly to master. However using tags would enforce proper versioning of plugins. This could be interesting to show the user release notes and so on update.

franciscolourenco commented 7 years ago

Thanks for the help! Version 1.0.0 uses a snippet instead of function files.