martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.25k stars 258 forks source link

ui: add bell notification (beep) #945

Closed joukewitteveen closed 7 months ago

joukewitteveen commented 3 years ago

The bell is sounded when there is nothing to undo/redo. This is consistent with vim, although vim has many more beeps cases.


I wanted to browse the vis code-base a bit and decided that this was a good excuse to do so. In VIM, the bell can be silenced by configuring a "visual bell" (screen flash) and setting the visual bell command to the empty string. I reckon implementing anything like a "visual bell" is out of scope for vis (it can be configured in your terminal emulator anyway), but we might want an option to silence the bell still.

rnpnr commented 1 year ago

I don't see anything wrong with this code but besides this pull request no one is complaining about vis' lack of a terminal bell. Personally I really don't like it so I'm not interested in applying this.

But, since this isn't rnpnr/vis I'm not closing this and will let others decide. Most people probably won't want to comment so you can leave a reaction to this post vote.

:+1: : I agree DON'T merge this. :-1: : I disagree DO merge this.

mcepl commented 1 year ago

My other problem with this PR is that it does more than what’s in the commit message. It seems to be playing with the colour palette more than just what’s necessary to introducing the bell notification (at least unnecessary formatting changes).

joukewitteveen commented 1 year ago

unnecessary formatting changes

There are two places where I put the { on the same line as the definition for consistency. Other apparent unnecessary changes are mainly just git not showing the diff the way you should think about it. There is no actual changing of colour palette code going on.

For me, the addition of a bell is a matter of accessibility as it allows me to get into a known state without looking at the screen: repeat until the bell sounds and you know where you are.

rnpnr commented 1 year ago

the addition of a bell is a matter of accessibility

Maybe I'm ignorant but is it possible to use vis without looking at the screen? If that's the case there is a possibility that I can be convinced. Otherwise I think I will close this.

joukewitteveen commented 1 year ago

is it possible to use vis without looking at the screen?

Sure! I'm a confident typist and can for instance transcribe or translate something without looking at either the screen or keyboard. With vim, the bell is really useful for such use cases. With vis the proposed change is actually very minor: it just sounds the bell at the end of the undo (and redo) stack. This allows you to quickly undo all changes by hammering u a few times until the bell sounds.

erf commented 1 year ago

With an option to silence the bell (defaulted to off), i don't mind this change.

joukewitteveen commented 1 year ago

With an option to silence the bell (defaulted to off), i don't mind this change.

I would consider that useless code that serves as nothing but a maintenance burden. The audible aspect of the terminal bell can be configured in your terminal emulator. The bell is sounded in many more prevalent circumstances than at the end of undo/redo sequences in your editor. Tab completion for example also typically sounds the bell. Have you actually tried vis with the change? The change is fairly minimal.

erf commented 1 year ago

With an option to silence the bell (defaulted to off), i don't mind this change.

I would consider that useless code that serves as nothing but a maintenance burden. The audible aspect of the terminal bell can be configured in your terminal emulator. The bell is sounded in many more prevalent circumstances than at the end of undo/redo sequences in your editor. Tab completion for example also typically sounds the bell. Have you actually tried vis with the change? The change is fairly minimal.

Yes I've tried it and don't mind the default bell sound, especially when i see i can configure my terminal (Wezterm) on how to respond to it. Not sure all users agree though .. and it does require some additional effort by the user to return to the previous behavior

mcepl commented 7 months ago

I think we need a decision about this issue, @rnpnr , either merge this or reject it, but this lingers here.

rnpnr commented 7 months ago

It looks to me like most people do not want this patch. For the time being it will have to remain local for anyone who wants it. Feel free to link it in the wiki if you want.