Add refresh-config and open-config command #1803

jharrilim · 2022-03-13T06:34:30Z

This PR aims to enable config refreshes while in the editor, and provide an in-editor alternative to --edit-config.

It adds two new :commands:

refresh-config
open-config

Basically what happens is:

When the editor receives the refresh-command, it fires off an event up to the application
The application sees the event and resets the config to the latest version from the config.toml file
The main config, editor config, and theme are refreshed

commentary:

~~I couldn't get dynamic dispatch working using DynAccess. This would presumably save a lot of work when updating the config and propagating changes.~~ Figured it out, but I had to remove #[derive(Debug)] from Editor.

This is also my first time using tokio and arc_swap. I actually don't know what I'm doing, I'm being carried heavily by the compiler rn

related issues:

helix-term/src/application.rs

archseer

Looks reasonable, just some cleanup:

helix-term/src/config.rs

helix-view/src/editor.rs

helix-term/src/application.rs

helix-term/src/commands/typed.rs

helix-term/src/config.rs

jharrilim · 2022-03-13T17:45:39Z

helix-term/src/commands/typed.rs

@@ -881,6 +879,9 @@ fn setting(
        _ => anyhow::bail!("Unknown key `{}`.", args[0]),
    }

+    cx.editor
+        .config_events
+        .push(tokio_stream::once(ConfigEvent::Update(runtime_config)));


With the change to put the editor config behind dynamic dispatch, we now have to send updates back up to the application, and let it make the changes there.

archseer

Sorry it took me a few days for another look, was trying to think of a way to avoid that extra channel in the event loop but I think this is the simplest implementation for now.

Just some minor changes

helix-term/src/application.rs

helix-term/src/commands.rs

helix-view/src/editor.rs

book/src/generated/typable-cmd.md

archseer · 2022-03-17T07:58:55Z

(By the way, feel free to join us on the Matrix channel!)

Co-authored-by: Blaž Hrastnik <[email protected]>

jharrilim · 2022-03-18T03:31:40Z

I've made all requested changes:

commands renamed to :config-reload and :config-save
config() helper created
keymaps now have dynamic access, tested that :config-reload works with this now
config_events uses an unbounded channel instead of SelectAll
The other refactors

And also:

Removed --edit-config
Added keymaps() helper
Refactored handle_keymap_event (this was necessary due to mut issues occuring after changing keymaps)

archseer

Getting closer, some thoughts about the keymaps type

archseer · 2022-03-18T03:35:08Z

helix-term/src/ui/editor.rs

+                            .keymaps
+                            .load()


keymaps()?

archseer · 2022-03-18T03:35:46Z

helix-term/src/ui/editor.rs

+                            .clone()
+                            .get_mut(&mode)


I'm not clear on why this was doing get_mut() before, you could probably avoid the .clone() by using .get()

From a glance, it looks like it was done because of the unwrap() after it
edit: The .get after the unwrap() is actually &mut self

archseer · 2022-03-18T03:38:15Z

helix-term/src/application.rs

+            Box::new(Map::new(Arc::clone(&config), |config: &Config| {
+                &config.editor
+            })),
        );

-        let editor_view = Box::new(ui::EditorView::new(std::mem::take(&mut config.keys)));
+        let keymaps = Box::new(Map::new(Arc::clone(&config), |config: &Config| {
+            &config.keys
+        }));


Can we avoid boxing by using Map<> directly? For that to work you will probably need to use a fn as the last parameter rather than a closure, so that you can name it on the type

helix-term/src/ui/editor.rs

This reverts commit 06bad8c.

jharrilim

@archseer made changes and actually got it working! I left some comments inline describing some of the changes.

jharrilim · 2022-03-23T02:39:55Z

helix-term/src/keymap.rs

-                "a" => goto_prev_parameter,
-                "o" => goto_prev_comment,
-                "space" => add_newline_above,
+fn default_keymaps() -> HashMap<Mode, Keymap> {


(1/2) Had to move the default keymaps into its own function, this was to allow...

jharrilim · 2022-03-23T02:40:55Z

helix-term/src/keymap.rs

-    let mut delta = std::mem::take(&mut config.keys);
-    for (mode, keys) in &mut config.keys.map {
-        keys.merge(delta.map.remove(mode).unwrap_or_default())
+    let mut delta = std::mem::replace(&mut config.keys, default_keymaps());


(2/2) ...this merge to continue working

jharrilim · 2022-03-23T02:42:38Z

helix-term/src/config.rs

@@ -12,7 +14,7 @@ pub struct Config {
    #[serde(default)]
    pub lsp: LspConfig,
    #[serde(default)]
-    pub keys: Keymaps,
+    pub keys: HashMap<Mode, Keymap>,


The "Keymaps" struct is now divorced from the config. Since the Keymaps encapsulated view state alongside config, it wasn't quite fitting, and had to be changed for config projection to work accordingly.

edit: Oops just realized the #[serde(default)], that needs to be updated to return the proper default HashMap

edit edit: This has now been updated to use the proper default HashMap

jharrilim · 2022-03-23T02:45:59Z

helix-term/src/keymap.rs

    pub sticky: Option<KeyTrieNode>,
 }

 impl Keymaps {
-    pub fn new(map: HashMap<Mode, Keymap>) -> Self {
+    pub fn new(map: Box<dyn DynAccess<HashMap<Mode, Keymap>>>) -> Self {


I kept this as Box<dyn DynAccess<T>> because defining a projection function here would prevent using arc_swap::access::Constant which is useful for testing.

helix-term/src/keymap/default_keymaps.rs

helix-term/src/keymap.rs

archseer · 2022-03-24T05:33:58Z

helix-term/src/keymap/macros.rs

+pub use alt;
+pub use ctrl;
+pub use key;
+pub use keymap;
+pub use shift;


I don't think this is necessary? macro_export should be enough

This caught me off guard as well, macro_export is a bit different from macro_use:
https://stackoverflow.com/a/31749071/11886888

Without the pub use, it can't be imported from other files

archseer

Tested and works great! I'm quite happy with the keymap refactor too, thanks for working on this! 🎉

jharrilim added 2 commits March 13, 2022 01:05

Add refresh-config and open-config command

03bade9

clippy

4a0eff1

archseer reviewed Mar 13, 2022

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

archseer requested changes Mar 13, 2022

View reviewed changes

pickfire reviewed Mar 13, 2022

View reviewed changes

helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved

pickfire reviewed Mar 13, 2022

View reviewed changes

helix-term/src/config.rs Outdated Show resolved Hide resolved

jharrilim added 3 commits March 13, 2022 13:36

Use dynamic dispatch for editor config

209112b

Refactor Result::Ok to Ok

f1b21af

Remove unused import

124ea23

jharrilim commented Mar 13, 2022

View reviewed changes

jharrilim added 5 commits March 13, 2022 13:48

cargo fmt

3676e2e

Modify config error handling

7b74c43

cargo xtask docgen

6e4da94

impl display for ConfigLoadError

d7e552b

cargo fmt

a41c215

anthonyjclark mentioned this pull request Mar 15, 2022

Added flags for editing language configurations viewing the log file. #1797

Closed

archseer requested changes Mar 17, 2022

View reviewed changes

archseer mentioned this pull request Mar 17, 2022

Continue loading the config on error #1814

Closed

the-mikedavis mentioned this pull request Mar 18, 2022

changelog notes for 22.03 #1830

Merged

jharrilim and others added 6 commits March 17, 2022 22:48

Put keymaps behind dyn access, refactor config.load()

06bad8c

Update command names

ea39201

Update helix-term/src/application.rs

0bf7beb

Co-authored-by: Blaž Hrastnik <[email protected]>

Merge branch 'master' into feat/add-refresh-config-command

161f9b8

Switch to unbounded_channel

b459895

Remove --edit-config command

f930c77

jharrilim requested review from archseer and pickfire March 18, 2022 03:31

Update configuration docs

e6a2a60

archseer requested changes Mar 18, 2022

View reviewed changes

Revert "Put keymaps behind dyn access", too hard

c5d7242

This reverts commit 06bad8c.

archseer mentioned this pull request Mar 20, 2022

Actually split mi and ma into several subcommands #1723

Closed

archseer force-pushed the feat/add-refresh-config-command branch from 15d963b to c5d7242 Compare March 20, 2022 07:02

jharrilim added 2 commits March 22, 2022 19:04

Merge branch 'master' into feat/add-refresh-config-command

dbbea48

Add refresh for keys

7df2f0c

jharrilim commented Mar 23, 2022

View reviewed changes

Refactor default_keymaps, fix config default, add test

681b8c8

jharrilim commented Mar 23, 2022

View reviewed changes

helix-term/src/keymap/default_keymaps.rs Outdated Show resolved Hide resolved

archseer reviewed Mar 23, 2022

View reviewed changes

helix-term/src/keymap.rs Outdated Show resolved Hide resolved

archseer reviewed Mar 23, 2022

View reviewed changes

helix-term/src/keymap.rs Outdated Show resolved Hide resolved

jharrilim added 2 commits March 23, 2022 12:35

swap -> store, remove unneeded clone

ad5dc91

cargo fmt

23e96a6

jharrilim requested a review from archseer March 24, 2022 05:03

archseer reviewed Mar 24, 2022

View reviewed changes

Rename default_keymaps to default

252d746

jharrilim requested a review from archseer March 25, 2022 04:35

archseer approved these changes Mar 25, 2022

View reviewed changes

archseer merged commit bee05dd into helix-editor:master Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add refresh-config and open-config command #1803

Add refresh-config and open-config command #1803

jharrilim commented Mar 13, 2022 •

edited

Loading

archseer left a comment

jharrilim Mar 13, 2022

archseer left a comment

archseer commented Mar 17, 2022

jharrilim commented Mar 18, 2022

archseer left a comment

archseer Mar 18, 2022

archseer Mar 18, 2022 •

edited

Loading

jharrilim Mar 18, 2022 •

edited

Loading

archseer Mar 18, 2022

jharrilim left a comment

jharrilim Mar 23, 2022

jharrilim Mar 23, 2022

jharrilim Mar 23, 2022 •

edited

Loading

jharrilim Mar 23, 2022

archseer Mar 24, 2022

jharrilim Mar 25, 2022

archseer left a comment

Add refresh-config and open-config command #1803

Add refresh-config and open-config command #1803

Conversation

jharrilim commented Mar 13, 2022 • edited Loading

archseer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archseer left a comment

Choose a reason for hiding this comment

archseer commented Mar 17, 2022

jharrilim commented Mar 18, 2022

archseer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archseer Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

jharrilim Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jharrilim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jharrilim Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archseer left a comment

Choose a reason for hiding this comment

jharrilim commented Mar 13, 2022 •

edited

Loading

archseer Mar 18, 2022 •

edited

Loading

jharrilim Mar 18, 2022 •

edited

Loading

jharrilim Mar 23, 2022 •

edited

Loading