Skip to content

Add live preview to theme picker - #1798

Merged
archseer merged 32 commits into
helix-editor:masterfrom
jharrilim:feat/add-theme-picker-with-live-preview-jharrilim
Jul 5, 2022
Merged

Add live preview to theme picker#1798
archseer merged 32 commits into
helix-editor:masterfrom
jharrilim:feat/add-theme-picker-with-live-preview-jharrilim

Conversation

@jharrilim

@jharrilim jharrilim commented Mar 12, 2022

Copy link
Copy Markdown
Contributor

livepreview

I'm probably annoying at this point, as I'm spamming this repo with my experiments. This one was fun though. It even reverts back to the original theme if you didn't select any. Perhaps it's a good start?


misc stuff:

  • Also added theme caching to reduce FS reads (although tbh it might be better to load them at the start)
  • Added more callbacks to the picker
  • A last_theme property is used now, this is useful for when a user cancels the theme preview so we can reset back to that theme
  • TypableCommands now must declare a list of PromptEvents they want to react upon

@sudormrfbin

Copy link
Copy Markdown
Member

A better way to approach this would be to implement live preview on :theme <TAB>, so that you can see how the syntax highlighting for code changes on each theme. IIRC it was attempted before but dropped because of some architectural challenges, but I think it might be easier to implement now. And experiments are never a bad thing :)

@jharrilim

Copy link
Copy Markdown
Contributor Author

A better way to approach this would be to implement live preview on :theme , so that you can see how the syntax highlighting for code changes on each theme.

Agreed, that seems like a good choice. I was going to fiddle with setting a smaller height to the picker, but the :theme command has that going for it naturally. I'll try poking around with that too!

@jharrilim

Copy link
Copy Markdown
Contributor Author

livepreview2

It's all coming together

@jharrilim

Copy link
Copy Markdown
Contributor Author

Removed the picker variant and kept the :command variant

@jharrilim
jharrilim marked this pull request as ready for review March 13, 2022 19:40
Comment thread helix-term/src/ui/picker.rs Outdated
Comment thread helix-term/src/commands/typed.rs
Comment thread helix-view/src/editor.rs Outdated
@jharrilim
jharrilim requested a review from sudormrfbin March 18, 2022 03:40
Comment thread helix-term/src/application.rs Outdated
Comment thread helix-view/src/editor.rs Outdated
Comment thread helix-view/src/theme.rs Outdated
@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2022
@jharrilim
jharrilim requested a review from pickfire June 18, 2022 22:23
@jharrilim

Copy link
Copy Markdown
Contributor Author

helix-editor/website#10 (comment)

@the-mikedavis sorry for the wait! This is ready to be re-reviewed.


(I had initially been distracted by Elden Ring if that's fair)

Comment thread helix-term/src/commands/typed.rs Outdated
Comment thread helix-view/src/editor.rs Outdated
Comment thread helix-view/src/theme.rs Outdated
@jharrilim
jharrilim requested a review from pickfire June 22, 2022 03:59

@sudormrfbin sudormrfbin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, awesome work @jharrilim !

@jharrilim jharrilim changed the title [Proposal] Add theme picker with live preview Add live preview to theme picker Jun 24, 2022

@archseer archseer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any other typable commands we could live preview?

Comment thread helix-term/src/commands/typed.rs Outdated
Comment on lines +16 to +17
/// Use `prompt_events` to declare which events this command will respond to.
pub prompt_events: u8,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this really worth it? It adds extra complexity. I'd just always call the prompt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And then return early inside the function. That way it behaves exactly like regex_prompt and other command prompts.

@jharrilim jharrilim Jun 26, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had that initially, but swapped to this approach to avoid touching the existing functions: #1798 (comment)

I don't think it's going to be a clean revert at this point unfortunately
8dfd840

I don't have strong opinions on either approach. If you confirm that you'd prefer what was done originally I'll change it back, but I'll only do it one more time or else I might get tendonitis 😳

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I'd prefer to handle these inside existing functions, it's a bit more flexible. Sorry for the trouble!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I'd prefer to handle these inside existing functions, it's a bit more flexible. Sorry for the trouble!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched it back, should be good to go!

Comment thread helix-term/src/commands/typed.rs
Comment thread helix-view/src/editor.rs Outdated
@jharrilim

Copy link
Copy Markdown
Contributor Author

Is there any other typable commands we could live preview?

goto_line_number is a great candidate for that IMO.

For not-so-live-previews, I could imagine it might be nice a nice option for showing the contents of the clipboard if the size of the yank isn't too large.

…ithub.com:jharrilim/helix into feat/add-theme-picker-with-live-preview-jharrilim
@pickfire

pickfire commented Jun 27, 2022

Copy link
Copy Markdown
Contributor

Is there any other typable commands we could live preview?

Indent style (we don't change existing stuff) and substitution (we don't have this) that I know of? If anything I find only live preview for theme picker far more useful than anything, at least don't have to select theme and go out, and select another one. The others that I know live preview is not that useful since we already have an expected result in mind.

@archseer

archseer commented Jul 1, 2022

Copy link
Copy Markdown
Member

goto_line_number is a great candidate for that IMO.

That's a good point! Feel free to exclude that from this initial PR though.

contents of the clipboard

We do show a register preview window you can use when selecting registers before pasting ("). But it doesn't cover the system clipboard.

@jharrilim

Copy link
Copy Markdown
Contributor Author

That's a good point! Feel free to exclude that from this initial PR though.

Agreed, let's keep that separate, it'll be easier to review that way

@pickfire pickfire left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@archseer archseer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for working on this!

@archseer
archseer merged commit b26e7e2 into helix-editor:master Jul 5, 2022
@jharrilim

Copy link
Copy Markdown
Contributor Author

Tagging this here for posterity. PR for goto preview:
#2982

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.

5 participants