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

Close popups automatically #1285

Merged

Conversation

bram209
Copy link
Contributor

@bram209 bram209 commented Dec 17, 2021

This PR adds two things:

  • EventResult::Ignored(Option<Callback>), to allow processing event result callbacks without consuming them
  • auto_close property on Popup, it closes the popup if an event happens that is ignored by its contents

fixes #326

@bram209 bram209 marked this pull request as ready for review December 17, 2021 16:56
@bram209 bram209 changed the title Feature/close popups automatically Close popups automatically Dec 17, 2021
helix-term/src/compositor.rs Show resolved Hide resolved
helix-term/src/compositor.rs Show resolved Hide resolved
helix-term/src/compositor.rs Outdated Show resolved Hide resolved
helix-term/src/compositor.rs Outdated Show resolved Hide resolved
helix-term/src/compositor.rs Outdated Show resolved Hide resolved
helix-term/src/compositor.rs Outdated Show resolved Hide resolved
@bram209 bram209 requested a review from archseer December 20, 2021 09:45
helix-term/src/ui/popup.rs Outdated Show resolved Hide resolved
@archseer
Copy link
Member

archseer commented Feb 6, 2022

Sorry for the delay here, I'm not sure on whether we want to enable this for all popups. it makes sense for documentation popups, but we want to keep popups with menus or popups with diagnostics info open for a while

@bram209
Copy link
Contributor Author

bram209 commented Feb 6, 2022

Hey,

In most situations, I find myself just needing a quick peek at the diagnostics message and would prefer to just manually reopen them if I needed.

I could see it being useful in some cases, but it could also be confusing if diagnostics stay open even after not being valid anymore. Especially if the latest code edit triggered a new, different, diagnostic at exactly the same location as an older diagnostic popup that has been left open.

Could add an additional property close_automatically: bool to Popup or decide if it should close based on the Popups id (might not be the most elegant solution since the id is in free-form...)

Thoughts?

@archseer
Copy link
Member

archseer commented Feb 7, 2022

That was just an example, we will be using popups to display various debug info once the debug adapter support gets merged.

Could add an additional property close_automatically: bool to Popup or decide if it should close based on the Popups id (might not be the most elegant solution since the id is in free-form...)

That seems reasonable, I'd add it as a builder method on Popup, defaulting to false. Like this:

pub fn margin(mut self, margin: Margin) -> Self {
self.margin = margin;
self
}

Popup::new(...).margin(..).auto_close(true)

@bram209 bram209 force-pushed the feature/close-popups-automatically branch from 428dabd to eb34c97 Compare February 22, 2022 11:50
Bram Hoendervangers added 4 commits February 22, 2022 12:54
…ups-automatically

# Conflicts:
#	helix-term/src/commands.rs
#	helix-term/src/ui/editor.rs
#	helix-term/src/ui/popup.rs
@bram209
Copy link
Contributor Author

bram209 commented Feb 22, 2022

@archseer added the auto_close property and enabled it for all hover popups.

Could you re-review it?

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Looks fine to me, thanks for your patience! 🎉

@archseer archseer merged commit 40eb126 into helix-editor:master Feb 23, 2022
@bram209 bram209 deleted the feature/close-popups-automatically branch February 23, 2022 07:06
the-mikedavis added a commit to the-mikedavis/helix that referenced this pull request Feb 23, 2022
supersedes helix-editor#1622

Builds on the work in helix-editor#1285. I want to allow Enter to create a newline
when there is no selection in the autocomplete menu.

This occurs somewhat often when using LSP autocomplete in Elixir which
uses `do/end` blocks (and I set the autocomplete menu delay to 0 which
exacerbates the problem):

```elixir
defmodule MyModule do
  def do_foo(x) do
    x
  end
  def other_function(y) do|
end
```

Here the cursor is `|` in insert mode. The LSP suggests `do_foo` but I
want to create a newline. Hitting Enter currently closes the menu,
so I end up having to hit Enter twice when the module contains any
local with a `do` prefix, which can be inconsistent. With this change,
we ignore the Enter keypress to end up creating the newline in this case.
archseer pushed a commit that referenced this pull request Feb 27, 2022
* ignore Enter keypress when menu has no selection

supersedes #1622

Builds on the work in #1285. I want to allow Enter to create a newline
when there is no selection in the autocomplete menu.

This occurs somewhat often when using LSP autocomplete in Elixir which
uses `do/end` blocks (and I set the autocomplete menu delay to 0 which
exacerbates the problem):

```elixir
defmodule MyModule do
  def do_foo(x) do
    x
  end
  def other_function(y) do|
end
```

Here the cursor is `|` in insert mode. The LSP suggests `do_foo` but I
want to create a newline. Hitting Enter currently closes the menu,
so I end up having to hit Enter twice when the module contains any
local with a `do` prefix, which can be inconsistent. With this change,
we ignore the Enter keypress to end up creating the newline in this case.

* pop compositor layer when ignoring Enter keypress

* move closing function out of consumed event result closure

* explicitly label close_fn as an 'Option<Callback>'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Close hover documentation when cursor leaves the symbol
2 participants