Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

Commit

Permalink
Adds option to reset time segment on execute
Browse files Browse the repository at this point in the history
  • Loading branch information
Syphdias committed Mar 14, 2019
1 parent fabc54a commit fb1ef54
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 0 deletions.
1 change: 1 addition & 0 deletions segments/time/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ where you want to show this segment.
|`P9K_TIME_FORMAT`|`'H:M:S'`|ZSH time format to use in this segment.|
|`P9K_TIME_REALTIME`|`false`|Enabling this option will update your prompt every `P9K_TIME_REALTIME_DELAY` seconds to display the current time. Note: This will trigger a `.reset-prompt` and can lead to side effects like history getting "stuck" or suggestions disappearing. |
|`P9K_TIME_REALTIME_DELAY`|`60`|This only takes effect if `P9K_TIME_REALTIME` is `true` and is used to set the delay between updates.|
|`P9K_TIME_RESET_ON_EXEC`|`false`|This will resulting in the time segment displaying the time the command was executed. Note: This is done by rebinding the `enter` key to redraw your prompt.|

As an example, if you wanted a reversed time format, you would use this:
```zsh
Expand Down
13 changes: 13 additions & 0 deletions segments/time/time.p9k
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
p9k::set_default P9K_TIME_FORMAT "%D{%H:%M:%S}"
p9k::set_default P9K_TIME_REALTIME false
p9k::set_default P9K_TIME_REALTIME_DELAY 60
p9k::set_default P9K_TIME_RESET_ON_EXEC false

################################################################
# Register handler for realtime prompt resets
Expand Down Expand Up @@ -57,6 +58,18 @@
fi
rm -f "$fifo"
fi

################################################################
# Bind the enter key to redraw the prompt (this will update the time)
if [[ $P9K_TIME_RESET_ON_EXEC == true ]]; then
__p9k_renew_prompt_accept_line() {
zle .reset-prompt
zle accept-line
}
zle -N __p9k_renew_prompt_accept_line
bindkey "^M" __p9k_renew_prompt_accept_line
ZSH_AUTOSUGGEST_CLEAR_WIDGETS+=__p9k_renew_prompt_accept_line
fi
}

################################################################
Expand Down

12 comments on commit fb1ef54

@romkatv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentionally that for accept-line you allow user-defined widgets but for reset-prompt you don't? What's the logic behind it?

@Syphdias
Copy link
Member

@Syphdias Syphdias commented on fb1ef54 Mar 14, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, I picked up .reset-prompt from your code without knowing the details about the differences, tbh. I assume something like \ or command to not use aliases.
I'm not sure which one to prefer

@romkatv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't know what's the best practice here -- allow overrides or not? But at least be consistent.

@romkatv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P9K_TIME_RESET_ON_EXEC has suboptimal UX.

The simplest model of P9K_TIME_RESET_ON_EXEC for users to understand is turning time segment into "start time of the command". However, the code fails to deliver on this promise if the user has bound accept-command to some key other than [ENTER].

In addition, time segment on the last promot also don't conform to the model because in there time displays "end time of the previous command", unlike in all other prompts. To avoid breaking the model, the current prompt must either have no time (only previous prompts have "start time of the command") or the time must be ticking, indicating that the start of the command is moving into the future.

Another problem is that [ENTER]' isn't the only thing that accepts commands. There is Ctrl+O`, which is quite useful, and might be other things I don't know about.

@Syphdias
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to use the real-time version for a while now. It is really annoying that it breaks going through the history. Here an example of what I mean:

echo foo
ls /
echo bar
ls
echo

Two examples

  1. <press up (to get echo)><wait for prompt to rerender><press up (to get ls)>
    this will get echo bar since your current command starts with echo
  2. <press up twice (to get ls)><wait for prompt to rerender><press up (to get echo bar)>
    this will get ls /

Regarding the UX of P9K_TIME_RESET_ON_EXEC:
Looking at bindkey I found these which I could rebind as well. If there is a keybind missing people can always report it.

bindkey |grep accept
"^J" accept-line
"^M" accept-line
"^O" accept-line-and-down-history
"^[A" accept-and-hold
"^[a" accept-and-hold

In addition, time segment on the last promot also don't conform to the model because in there time displays "end time of the previous command", unlike in all other prompts. To avoid breaking the model, the current prompt must either have no time (only previous prompts have "start time of the command") or the time must be ticking, indicating that the start of the command is moving into the future.

I'm not sure what you mean by that. Do you want to say that it should be either the time the command ended or always the time the command started and not a hybrid? If so, playing devil's advocate here, you could also say, since you didn't execute a command yet the time shouldn't tick either but not displayed because you didn't execute a command yet.

For me, and this is a very subjective view, the reset on accept-line is the perfect hybrid. If you scroll up, you see the start of execution and for the last one, you see when it ended. The ticking is a nice addition, to turn on or off.

So two questions. How would you tackle the UX side? And do you think the history bug can be fixed (that imho is a bigger threat to UX)?

@romkatv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I'm working on something else right now but I might get back to this later.

@romkatv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I fixed the history thing here: https://github.com/romkatv/zsh/tree/gentle-reset-prompt. Diff against upstream: zsh-users/zsh@master...romkatv:gentle-reset-prompt.

My changes do two things. First, they add -W to zle widget builtin that causes LASTWIDGET to be restored after the call. Second, it forces reset-prompt widget to always be called with this flag.

Before this change up-line-or-beginning-search (this is what your '[up]key is bound to) was broken, as you've aptly noticed. It was losing its context not only on explicitzle rest-prompt` calls but also when background jobs complete (background job completion will also wipe your completion menu off the screen). This change fixes the bug and also fixes a long-standing problem that I had with user-defined widgets (see below).

I want my [up] key to behave as up-line-or-beginning-search with local history turned on, so that it doesn't see history from other sessions, and my [ctrl+up] key to behave as up-line-or-beginning-search with local history turned off, so that it does see history from other sessions. This is very difficult to achieve. The only solution I found is to fork up-line-or-beginning-search and edit its source code (you probably noticed I fork code a lot). You can see it in my .zshrc. With the new -W flag that I added to zle this problem has a clear solution. Like this:

# This zle widget is the same as the standard up-line-or-beginning-search
# but restricted to local history.
function up-line-or-beginning-search-local() {
  zle .set-local-history -W 1
  up-line-or-beginning-search
  zle .set-local-history -W 0
}
zle -N up-line-or-beginning-search-local

I also tried to fix the bug with completion menu disappearing but the code in there is really complex, so I gave up after an hour.

@Syphdias
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romkatv Quick note: I haven't forgotten about this. I'm currently just a little short on time :(

@romkatv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I started a thread with zsh-workers about upstreaming my fix for history being broken by reset-prompt. See https://www.zsh.org/mla/workers//2019/msg00204.html.

@romkatv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: After you've forked the code from p10k, I've implemented several fixes.

  1. This code triggers a bug in zsh. I've reported it here: https://www.zsh.org/mla/workers//2019/msg00205.html. I've implemented a workaround for it, which you can find in p10k.
  2. This code leaves behind a background process if you run exec zsh. If you run it several times, you can have many of the same rogue processes hanging around. This is fixed in p10k.

@Syphdias
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romkatv, thx for letting me know. Currently, I put this code into the time segment but I think this can be useful for more than just this setting to rerender the prompt (as you did for background_jobs as well, I think).

Maybe I will need it to fix #1196 and #1201 (though I will try to do it without it)

Reminds me, I should join the zsh mailing list...

@romkatv
Copy link

@romkatv romkatv commented on fb1ef54 May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about zle-line-finish. Might be useful. Quick proof of concept (requires ZSH >= 5.3):

local d=$(mktemp -d) &&
  >$d/.zshrc <<<'
    function archive-prompt() {
      zle || return
      local p=$PROMPT
      PROMPT="[%D{%H:%M:%S}] $p"
      zle reset-prompt
      zle -R
      PROMPT=$p
    }
    autoload -Uz add-zle-hook-widget
    add-zle-hook-widget line-finish archive-prompt' &&
  ZDOTDIR=$d zsh; rm -rf "$d"

Please sign in to comment.