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

Address VisualBell Feedback #430

Closed
wants to merge 6 commits into from

Conversation

markandrus
Copy link
Contributor

@jonhoo raised some useful feedback in #406, namely

  1. Behaviors like "brighten text", "dim text", etc., would be useful.
  2. Colors should be customizable to accommodate different color schemes.
  3. Non-flashing behaviors like "pixelate" would be useful.

This PR addresses 1 and 2 by converting VisualBellConfig into an enum representing one of three effects:

  • None: does nothing
  • FlashText: flashes the text a configurable color
  • FlashBackground: flashes the background a configurable color

Example video (warning: flashing lights): https://gfycat.com/IdealLazyDairycow

Other changes:

  • Renamed VisualBellAnimation to "VisualBellEasing" and animation to easing.
  • Moved the cubic_bezier code to src/config.rs and added an ease function to the VisualBellEasing implementation.

The VisualBellConfig enum could be expanded in the future to support a Pixelate option, e.g. something like

visual_bell:
  Pixelate:
    size: 24 # px
    duration: 150

Or perhaps even a Blur could be supported; however, I think these would require adding a rendering pass, so I haven't implemented these yet.

@lord-re
Copy link

lord-re commented Feb 20, 2017

The flashing background with customizable color is way better than the great white flash ! In fact i was trying to add this feature. Glad you did it.

@jwilm
Copy link
Contributor

jwilm commented Feb 23, 2017

Thanks for the PR! I've tried this out, and it all works as described. Having control of the flash color is much nicer than the great white flash.

I'm not sold on having advanced effects like the text flashing or even more complex effects like pixelate. The color overlay can be implemented concisely. Mostly, I worry about taking on a bunch of code for something simple like the bell.

Going to think on this some more.

@markandrus
Copy link
Contributor Author

I'm not sold on having advanced effects like the text flashing or even more complex effects like pixelate. The color overlay can be implemented concisely. Mostly, I worry about taking on a bunch of code for something simple like the bell.

Understood 👍 FWIW, I'm just playing around with this feature in order to learn a bit more about Rust (and to play to some degree with OpenGL shaders). I'm happy to explore what can be done, even if little or none of it should be merged. I'm also happy to cut the text flashing or other parts of the PR if you'd like to accept some form of it.

@OJFord
Copy link
Contributor

OJFord commented Feb 23, 2017

The FlashText option looks fantastic, big 👍 for that.

@NickeZ
Copy link
Contributor

NickeZ commented Feb 23, 2017

It would have been awesome with some more subtle bell like pixelate or blur, but I fully understand that it feels like bloat..

edit: you could also have like a gradient with some lighter color towards the edges..

@pokey
Copy link

pokey commented May 4, 2017

@jwilm any updates on this? All I'd be after is configurable background color for the flash. This seems like a good compromise between avoiding bloat / addressing the visually disturbing flash for those who dislike it.

@unphased
Copy link

The great white flash is headache-inducing! I'll take anything that can be configured to be as subtle as i need (which is very subtle). Also nice would be if it could play sound like a bell traditionally does, although I COMPLETELY understand if we want to keep everything simple and visual only for dependency/portability/aesthetic reasons.

chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Dec 24, 2017
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Jan 18, 2018
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Jan 21, 2018
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Jan 29, 2018
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Jan 30, 2018
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
@chrisduerr
Copy link
Member

#963 should implement this.

chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Feb 16, 2018
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Feb 20, 2018
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Mar 25, 2018
This addresses the main feedback in alacritty/pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
@chrisduerr chrisduerr closed this Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants