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

Add command to open a popup as a new buffer #9311

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jerabaul29
Copy link
Contributor

closes #8748

The idea is to make it possible to press ctrl-o to open the popup content in a new buffer. Once this work, I can easily add ctrl-h and ctrl-v to open in horizontal and vertical splits :) .

This is still an early WIP; what remains to be done:

  • create the new buffer as read only to avoid the need to use ! to exit etc
  • find how to grab the content of the popup to put it into the new buffer
  • find how to grab a meaningful title for the popup to put it as the new buffer title
  • find how to enable the correct coloring of the new buffer

I will look into this more in the days to come :) .

@jerabaul29
Copy link
Contributor Author

For now, starting from a popup, this opens a new buffer with a preset title and content. This compiles fine.

@jerabaul29
Copy link
Contributor Author

jerabaul29 commented Jan 13, 2024

I am trying to understand the structure of the classes that would be involved in getting this to work, and if changes should be made, what is the best way. Looking in particular into https://github.com/helix-editor/helix/blob/master/helix-term/src/ui/lsp.rs#L50 and related.

So it looks like the popup content: T where T: Component has no way to get the actual string-like content of the popup from the content as it is now; all it can do is ask the content to render itself.

I see 2 ways further then:

  • ask the content to render itself in my new buffer, rather than putting a string in the new buffer; but then, I guess moving around, copy pasting, etc, will not work? Then this would not be so useful. actually I do not think I could / should ask for a render at this location in the code: it can be done only from higher up in the app.

  • add some Options<ContentAsStrings> or similar to the Popup struct, to contain, if relevant (not all popups are of a nature that makes them relevant to know their content to make it possible to open in a new buffer; i.e., a long documentation is relevant to open in a new buffer; a list of selections or other hoovers, likely not), the title and main body content and language information of a popup content. I.e., this would mean, in order to be able to open for example the documentation of a function in a new popup, to change the popup to being:

pub struct ContentAsString {
    title: String,
    body: String,
    language: String,
}

pub struct Popup<T: Component> {
    contents: T,
    position: Option<Position>,
    margin: Margin,
    size: (u16, u16),
    child_size: (u16, u16),
    position_bias: Open,
    scroll: usize,
    auto_close: bool,
    ignore_escape_key: bool,
    id: &'static str,
    has_scrollbar: bool,
    // fields used in case the Popup can be opened in a new buffer; e.g., lsp documentation
    content_as_strings: Option<ContentAsStrings>,
}

and then this content_as_string could be set to Some only if relevant in the Popup::new, or by default set to None.

With this information, it would be trivial to open for example doc popups in a new buffer.

Would this make sense from an architecture point of view? Is it worth to give it a try?

@jerabaul29
Copy link
Contributor Author

This is now able to open doc into a new markdown buffer :) .

@jerabaul29
Copy link
Contributor Author

This now has the following functionality:

  • starting from e.g. this:

Screenshot from 2024-01-14 16-43-19

  • I can now press ctrl-o to get to:

Screenshot from 2024-01-14 16-43-24

The main things I do not manage to get at the current time:

  • making the file really read-only from helix viewpoint; I tried setting readonly to true, but helix still allows to edit the content of the buffer
  • getting a more meaningful title for the new buffer; for example doc-println! in this case; I can easily replace "hover" with any string, but I have no idea how I could get that I am getting information about the println! element from the context I have.

Any advice on these 2 points (and the general draft of course :) ) would be welcome; @the-mikedavis any advice? :)

@jerabaul29
Copy link
Contributor Author

(of course refactoring, supporting splits etc, will come soon, just trying to find how to do the last "blocker actions" before refactoring and extending :) ).

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I don't intend on properly looking at or merging this feature until #5505 is possible. With #5505 we could make this possible only on popups where this makes sense, like hover, and we could cleanly downcast the type to a concrete Popup<Markdown>, where we would be able to read the contents. Without it we need to duplicate the contents and store it on the Popup as you have here with StringsContent which IMO is too hacky.

// put a meaningful title
// TODO: get a meaningful buffer title
let stringified_buffername = Some(Path::new(&stringified_buffername));
doc.set_path(stringified_buffername);
Copy link
Member

Choose a reason for hiding this comment

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

The path of a buffer isn't meant to be a name - this should be left as a scratch buffer instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree / understand this is hacky... But at the same time, I would like to have a more illustrative buffer name than "scratch", so that if I have several docs open, I can navigate easily between them. I.e. instead of having "scratch", "scratch", "scratch", I would find it much easier to navigate between several docs named "doc_NAME1", "doc_NAME2", "doc_NAME3". Any idea / recommendation / "non hacky way" you would do this?

Copy link
Member

Choose a reason for hiding this comment

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

No, these docs shouldn't be opened for very long if they're just being used for reference anyways so I think scratch is fine

Comment on lines +262 to +275
cx.editor.new_file(Action::Replace);

// TODO: how can we meaningfully stringify the content of a Popup? for now dummy
// TODO: is this an un-necessary copy? ...
// put the content in the new buffer
let (view, doc) = current!(cx.editor);
paste_impl(
&[stringified_body],
doc,
view,
Paste::Before,
1,
cx.editor.mode,
);
Copy link
Member

Choose a reason for hiding this comment

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

The new document should be created with Document::from and then added to the editor with Editor::new_file_from_document (which is currently private but could be pub). That will remove the need to modify the selection, jumplist and last-saved-revision.

doc.set_last_saved_revision(1);
// make readonly
// TODO: this does not make really readonly from helix viewpoint, why?
doc.readonly = true;
Copy link
Member

Choose a reason for hiding this comment

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

This is set for files that have readonly permissions. A readonly "mode" for buffers that prevents editing isn't implemented yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I guess we could consider implementing some form of "make readonly" function? Should I open a new issue / PR to discuss? Any recommended direction to implement something like this? :)

Copy link
Member

Choose a reason for hiding this comment

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

See #7128. I suspect that it would be a large change and it needs some design before anyone goes about making a PR that attempts it

Comment on lines +302 to +303
// close the popup since we just opened it in a new buffer
let _ = self.contents.handle_event(event, cx);
Copy link
Member

Choose a reason for hiding this comment

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

This passes the C-o event to the contained component (a Markdown in hover's case) which isn't necessary. The popup is handling the event so it shouldn't pass it down. Closing the popup is done below by returning Some(close_fun)

@jerabaul29
Copy link
Contributor Author

and we could cleanly downcast the type to a concrete Popup, where we would be able to read the contents

Interesting; is there already some "machinery" / tooling in place for this downcast + getting the contents from it (could you point me to it if so), or is it something that would be implemented following #5505 , or something to be implemented after it? :)

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Jan 23, 2024
@the-mikedavis
Copy link
Member

Here's a block that finds and downcasts a component in the compositor into a Overlay<Picker<T>> or Overlay<DynamicPicker<T>>:

let picker = match compositor.find::<Overlay<Self>>() {
Some(Overlay { content, .. }) => Some(content),
None => compositor
.find::<Overlay<DynamicPicker<T>>>()
.map(|overlay| &mut overlay.content.file_picker),
};

The compositor uses std::any::Any for dynamically typing the components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide ways to open pop ups in a new buffer / split
3 participants