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

Move commands into the helix-view crate #5555

Open
the-mikedavis opened this issue Jan 16, 2023 · 6 comments
Open

Move commands into the helix-view crate #5555

the-mikedavis opened this issue Jan 16, 2023 · 6 comments
Labels
A-gui Area: Helix gui improvements C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@the-mikedavis
Copy link
Member

This has been discussed before in #39 (comment) but it would be a large effort, so this issue focuses specifically on the change for moving commands into helix-view.

Commands currently exist in helix-term but should be moved to helix-view. Moving commands to helix-view should make it possible to switch between a terminal backend (helix-term) and a GUI backend. These changes may also help the macro keybindings effort (#4709) and fix some bugs with the command palette (#5294).

The difficult part of this change is that the commands currently manipulate the UI directly for creating Pickers and other UI elements or setting callbacks. The Compositor may need to be refactored as a trait and then components can be implemented separately by TUI and GUI backends.

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate A-gui Area: Helix gui improvements labels Jan 16, 2023
@nrabulinski
Copy link
Contributor

I'd like to take on at least part of this issue

@kirawi
Copy link
Member

kirawi commented Jan 16, 2023

There was previously an attempt w/ 1aa2b02

@gibbz00
Copy link
Contributor

gibbz00 commented Jan 16, 2023

Hii,

Some devs have been asking about some work I've been working on recently, so I thought I might explain it here.

I've probably spent 150 hours or so the last month working on a refactor that aims to allow for an easier implementation of many larger features. So lot of work so far has been put into cleaning up the code base whilst and reducing its technical debt. One mistake I've made, however, is not splitting up the work into multiple smaller PRs, that I then send in incrementally.

I'll try to change that moving forward, but right now I have some other things that have unfortunately come up. But I'll do my best to send them in as time allows.

Anyhow, here is a rough summary of some things I've worked on so far:

  • Keymap internal data structure representation and external display (info boxes and command palette). Made it possible to move keymap* to helix_view.
    Features that fell out from this were for example being able to bind nested keybinding to escape or digits.

  • Unified config loading for a consistent use in helix_term::main, helper.rs, docgen.rs etc.

  • Began moving commands to helix_view. (Very early stages.) One part of this work involved removing the distinction between static and typeable commands as there are quite a few commands that overlap, and the distinction adds a lot of duplicate code for little gain, other than initial implementation ease.

One feature that I wanted from this was triggering completion of say theme selection from a keybinding, whilst also being able to write multiple commands in command line (which would also work in config). It's experimental the syntax would look something like: (shown commands are arbitrary)

:theme - :config-reload :set-language - :goto_definition

Each dash triggers a completion pop-op, and commands are linked with the :. Which is basically a short-circuiting && operator. Using colon becomes okay if there is no distinction between static and typable commands.

Furthermore, there has also been talks about how some config commands have confusing and ambiguous names. But one typeable command feature is aliasing. So another part of this change has been to merge aliasing to the static commands for the use of clearer command names. Which would not create any large breaking changes to existing configs.

  • Another piece of the puzzle is to move much of the presentation logic from helix_term to view. This has involved moving more and more state fields to the helix_view::Editor. I'm calling it UITree on my branch (mostly to guide me in what does what), but the idea is to only require GUI implementation to be able to render all the contents of the ui_tree.

A lot of things build on top of each other, so the development has been quite frantic heuheu. Little effort has been placed into a compiling working state, and more so a separation of concerns. But like I said before, I will change this moving forward into some smaller PRs, even if it will be hard to make them completely separate.

@archseer
Copy link
Member

@gibbz00 I'd say some of this discussion is orthogonal to this issue and should be discussed separately. But I do recommend that you open up a RFC where we can discuss the design and split the work into smaller review chunks. It's unlikely we'll be able to merge a massive refactor that does a whole bunch of things because it will be a nightmare to review.

@archseer
Copy link
Member

Regarding https://github.com/helix-editor/helix/compare/gui, the attempt in 1aa2b02 solves it, we'd just need to recreate the same changes on the latest master. But I'm not sure if that's the best approach since it also required to move the ui components into view. It's also going to be a pain to merge since it'll break some of the open PRs.

@NikitaRevenco
Copy link
Contributor

NikitaRevenco commented Dec 19, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-gui Area: Helix gui improvements C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants