New editor config #2523

stefanhaller · 2023-03-29T10:48:29Z

PR Description

This implements the new editor configuration as discussed on Discord:

EditCommand and EditCommandTemplate were replaced by Edit, EditAtLine, and EditAndLineAndWait.

In addition, there's a new EditInTerminal boolean to say whether lazygit should suspend to background before calling the editor.

Also, there's a new EditPreset config that can set all four of the above settings at once.

Finally, OpenCommand and OpenLinkCommand were renamed to Open and OpenLink, respectively, for consistency with the others.

Please check if the PR fulfills these requirements

Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
Code has been formatted (see here)
Tests have been added/updated (see here for the integration test guide)
Text is internationalised (see here)
Docs (specifically docs/Config.md) have been updated if necessary
You've read through your own file changes for silly mistakes etc

github-actions · 2023-03-29T10:51:17Z

Uffizzi Preview deployment-20642 was deleted.

jesseduffield

Great work :) I left some comments

jesseduffield · 2023-04-02T00:44:15Z

docs/Config.md

-or
+If for some reason you are not happy with the default commands from a preset, or
+there simply is no preset for your editor, you can customize the commands by
+setting the `edit`, `editAtLine`, and `editAtLineAndWait` options, e.g.:


I'd also mention that they can go and update the getPreset function if they want to add a preset to lazygit for all others to use.

Added in 7cd3fb9

jesseduffield · 2023-04-02T00:46:15Z

pkg/config/editor_presets.go

+	}
+
+	editorToPreset := map[string]string{
+		"vim":    "vim",


we should also add nvim and vi as those were previously covered in GetEditCmdStr

This took three fixup commits :) I took the opportunity to simplify the code a bit, see 1ba3f10, f5c1a05, and 7685a04.

jesseduffield · 2023-04-02T00:48:11Z

pkg/config/user_config.go

+
+	// Whether the given edit commands use the terminal. Used to decide whether
+	// lazygit needs to suspend to the background before calling the editor.
+	// Pointer to bool so that we can distinguish unset (nil) from false.


makes sense

jesseduffield · 2023-04-02T00:48:51Z

pkg/config/user_config.go

+	EditInTerminal *bool `yaml:"editInTerminal,omitempty"`
+
+	// A built-in preset that sets all of the above settings. Supported presets
+	// are "vim", "emacs", "nano", "vscode", "sublime", "bbedit", "xcode".


I wouldn't enumerate the presets here given they'll change over time and it'll be one more place we need to keep in sync. I'd instead reference the place where the presets are defined

Makes sense, see ed17d96.

jesseduffield · 2023-04-02T00:50:39Z

pkg/config/user_config.go

@@ -570,7 +608,7 @@ func GetDefaultConfig() *UserConfig {
 				BulkMenu: "b",
 			},
 		},
-		OS:                           GetPlatformDefaultConfig(),
+		OS:                           OSConfig{},


For posterity I'll just state that (if I understand correctly) we're not using the default here because it prevents us from knowing whether a user has explicitly set one of the configs or not.

Exactly, that's how I tried to explain it in the commit message. 😉

jesseduffield · 2023-04-02T00:57:42Z

pkg/gui/layout.go

@@ -222,6 +222,9 @@ func (gui *Gui) onInitialViewsCreation() error {
 		if storedPopupVersion < StartupPopupVersion {
 			popupTasks = append(popupTasks, gui.showIntroPopupMessage)
 		}
+		if deprecatedEditConfigPopup := gui.makeDeprecatedEditConfigPopup(); deprecatedEditConfigPopup != nil {


We should only show this popup once: it would be annoying to get the popup every time you open lazygit until you've resolved the issue. We could add a new field to our AppState to support this

I disagree pretty strongly here. If we warn only once, people will ignore it the one time they see it, and then likely forget about it. They will then be very annoyed after a year when we remove the old configs and break their lazygit. I think this is not much better than not warning at all. That's exactly what I meant when we discussed the different options on Discord.

I could imagine throttling the warning to once a day, or maybe once a week, but I'm not sure it's worth the extra logic.

Cc @mark2185 who also had a strong opinion on Discord.

How about an alternative approach: rather than show a popup, we print a deprecation notice after the gui closes, so the message appears in the user's terminal. This means we'll still be able to nag the user, but it won't actually interfere with their flow (many people such as myself will start a new lazygit session frequently and having the popup there will be very annoying).

I would personally like the popup at startup better, as I'm afraid the terminal output is too easy to miss (especially when there's a lot of terminal output already, e.g. from multiple "commit with editor" commands).

But I'd be fine with it if you prefer it; see 2e4ce16.

jesseduffield · 2023-04-13T11:12:56Z

@stefanhaller fantastic work! Sorry it took me so long to get around to re-reviewing, but this is good to merge. Feel free to clean up your commits and I'll hit the button

These files were renamed from os_windows_test.go to os_test_windows.go (etc.) in 95b2e95. Since then, the tests have no longer run, since go only looks for tests in files ending with "test.go". It isn't important that the file name ends with "_windows.go", since there are already build constrains in the files themselves.

Now that the tests run again, it turns out that they actually fail, so fix them.

The "open" command is supposed to behave in the same way as double-clicking a file in the Finder/Explorer. The concept of jumping to a specific line in the file doesn't make sense for this; use "edit" instead.

Instead, query the platform defaults only if the config is empty. This will be necessary later to distinguish an empty config from a default config, so that we can give deprecation warnings.

We do this for consistency with the edit settings. The old names are kept as a fallback for now.

We print this to the terminal after lazygit quits rather than showing it in a panel at startup, so as to not annoy people too much. Hopefully it will still be prominent enough this way.

stefanhaller · 2023-04-13T11:17:35Z

Done. Thanks!

stefanhaller force-pushed the editor-config branch from 4562345 to 70b95c2 Compare March 29, 2023 12:47

jesseduffield requested changes Apr 2, 2023

View reviewed changes

stefanhaller added 10 commits April 13, 2023 13:14

Fix windows tests

24de156

Now that the tests run again, it turns out that they actually fail, so fix them.

Cleanup: fix copy/paste error in comment

7bbcec9

Implement edit presets

659d668

Add support for falling back to legacy edit config

2947b56

Remove line number support for "open" command

08d679c

The "open" command is supposed to behave in the same way as double-clicking a file in the Finder/Explorer. The concept of jumping to a specific line in the file doesn't make sense for this; use "edit" instead.

Don't set platform defaults on OSConfig struct immediately

b7e029a

Instead, query the platform defaults only if the config is empty. This will be necessary later to distinguish an empty config from a default config, so that we can give deprecation warnings.

Change OpenCommand to Open and OpenLinkCommand to OpenLink

e4e16fa

We do this for consistency with the edit settings. The old names are kept as a fallback for now.

Update documentation

e3c5e07

Show warning about deprecated edit configs

046b0d9

We print this to the terminal after lazygit quits rather than showing it in a panel at startup, so as to not annoy people too much. Hopefully it will still be prominent enough this way.

stefanhaller force-pushed the editor-config branch from 2e4ce16 to 046b0d9 Compare April 13, 2023 11:16

jesseduffield merged commit 04e0a9b into jesseduffield:master Apr 13, 2023

stefanhaller deleted the editor-config branch April 13, 2023 11:44

stefanhaller mentioned this pull request Mar 29, 2024

Deprecate "null" keybindings more strongly? #3458

Closed

Uh oh!

New editor config #2523

New editor config #2523

Uh oh!

Conversation

stefanhaller commented Mar 29, 2023

Uh oh!

github-actions bot commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield commented Apr 13, 2023

Uh oh!

stefanhaller commented Apr 13, 2023

Uh oh!

Uh oh!

github-actions bot commented Mar 29, 2023 •

edited

Loading

jesseduffield Apr 2, 2023 •

edited

Loading