Revamp commit message window by seand52 · Pull Request #2390 · jesseduffield/lazygit

seand52 · 2023-01-25T19:56:23Z

PR Description
Fixes Revamping the commit message window #2324
Press up/down arrows to cycle through previous commits
Instead of hitting alt + enter, press tab to toggle a new panel where you can add a commit description
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

mark2185 · 2023-01-25T21:26:02Z

Few things:

I would leave the second panel visible at all times, there's no need to toggle it, we don't gain anything by doing that. We should have a tooltip somewhere (if popups like that can have tooltips) that pressing tab moves the cursor to the next panel.

I'd also expect that rewording an existing commit (pressing r in the Commits panel) uses the same interface.

Other than that, looks great, well done! :)

jesseduffield

Looking great so far! I've left some comments.

Also, see pkg/gui/commit_message_panel.go: we'll need to update handleCommitMessageFocused now that you can't enter a newline from the summary panel.

Lemme know if you have any questions

pkg/integration/tests/commit/commit_multiline.go

pkg/integration/tests/commit/commit.go

pkg/gui/views.go

pkg/gui/controllers/commit_message_controller.go

pkg/gui/context/commit_message_context.go

pkg/gui/controllers/commit_description_controller.go

Ryooooooga · 2023-01-26T09:11:24Z

Is there really a case where we want to enter the same commit message as a previous commit?

I can still understand if it works like history of shell.

jesseduffield · 2023-01-26T09:12:30Z

@Ryooooooga we would rarely enter the exact same message but if you're creating a sequence of commits with a pattern to them it's easier to start with the previous one and tweak it than to start from scratch

mark2185 · 2023-01-26T09:12:43Z

Is there really a case where we want to enter the same commit message as a previous commit?

Enter? No.
But just ~~read~~ edit it? I think so.

seand52 · 2023-02-04T14:56:26Z

@jesseduffield @mark2185 I've addressed the initial comments and implemented these features since you last had a look:

Reword commit flow now uses the same commit message panel (with the toggle to add a description etc)
Hitting tab toggles between the commit summary and description (rather than hiding the description), and hitting escape closes the full thing
If a commit has a description it opens up both panels, else just the summary

Before making the PR ready for review I just wanted to get your thoughts on how I'm handling the switching between summary/description. This has been implemented mostly in this commit. I'm just pushing to the panels to context every time we hit tab, and when closing the panels I added a new function that deactivates all popups. Is this a feasible solution? It seems to work fine, but I'm not sure if maybe I'm missing something!

jesseduffield

Nice work, we're getting there! I've left some comments.

A couple things extra things:

Whenever we bring up the commit message panel we should also show the commit description panel at the same time, rather than only showing it upon the user pressing 'tab'
Currently if you go to write a commit message, type some stuff, then escape out, then open and close the same panel via the reword flow, we lose the stuff you originally typed. So we should store the message/description outside the view (when not rewording) and just clear the view itself whenever we close it. Then we can restore the message/description if we're opening the panel in the context of a non-reword commit.

Lemme know if you need any clarification/pointers

pkg/gui/files_panel.go

pkg/gui/context.go

pkg/gui/controllers/helpers/commits_helper.go

seand52 · 2023-02-07T18:36:22Z

@jesseduffield thanks for the tips! I've made the changes you suggested and it's looking better now! Would be great if you could take a look 🙂

mark2185 · 2023-02-07T20:08:53Z

If it's worth anything - I like it! Looks pretty good! :)

github-actions · 2023-02-07T20:17:01Z

Uffizzi Preview deployment-14719 was deleted.

JonatasAmaral · 2023-02-08T19:32:19Z

Hey, i think it's still time to suggest somethings, based on what i've just wrote in #2423, and was pointed here, for almost the same being done already:

make ctrl + ⏎ "submit" the commit, so no need to tab back and then ⏎. Just a nice touch i think
allow for still edit message in external editor, really necessary for better editing experience of long descriptions
as it is right now, i'm not sure how exactly do this, but here's a fell simplory options to start the discussion:
- add a "button" bellow message box, so ⥂, ⥂, ⏎ opens external
- a different key combination to open external instead write message. ex: alt + ⏎
and, as asked on that tag editor layout #2423 issue, enable that same editor to add tags

mark2185 · 2023-02-08T19:36:00Z

* make `ctrl + ⏎` "submit" the commit, so no need to tab back and then ⏎. Just a nice touch i think

That seems simple enough to be added in this PR as well.

* allow for still edit message in external editor, really necessary for better editing experience of long descriptions
  as it is right now, i'm not sure how exactly do this, but here's a fell simplory options to start the discussion: 
  * add a "button" bellow message box, so `⥂,  ⥂, ⏎` opens external
  * a different key combination to open external instead write message. ex: alt + ⏎

* and, as asked on that [tag editor layout #2423](https://github.com/jesseduffield/lazygit/issues/2423)  issue, enable that same editor to add tags

This would maybe be better as a separate PR on top of this one since the commit window revamp is a big change in and of itself.

seand52 · 2023-02-08T20:43:36Z

I agree with @mark2185 , this PR is already pretty big (much bigger than I initially expected 😆 ). So we should add those extra features in a separate PR to avoid scope creep

I like the ctrl + ⏎ idea though! Will implement that one here

mark2185 · 2023-02-10T10:44:36Z

Just noting that the Staged changes window isn't being updated after committing, even though I think that was fixed (?)

jesseduffield · 2023-02-10T23:43:30Z

@seand52 I'm in the process of testing this out locally: so far so good!

BTW: one of the linting failures is resolved on master so if you rebase onto master it'll be fixed

seand52 · 2023-02-10T23:48:29Z

@seand52 I'm in the process of testing this out locally: so far so good!

BTW: one of the linting failures is resolved on master so if you rebase onto master it'll be fixed

I was fixing some comments that @Ryooooooga left in the PR and changed quite a bit of stuff to fix this #2390 (comment).

In addition to that, I've also noticed that when a commit fails it seems to clear the context and the message is not preserved 😢 . I haven't figured out why this is the case yet but I was going to work on that tomorrow as well.

I have my changes locally though, and was going to push tomorrow. Maybe you can hold off on the testing until I do that ?

jesseduffield · 2023-02-10T23:50:30Z

@seand52 that's fine with me :)

seand52 · 2023-02-11T16:09:19Z

pkg/gui/view_helpers.go

 func (gui *Gui) resizePopupPanel(v *gocui.View, content string) error {
-	x0, y0, x1, y1 := gui.getConfirmationPanelDimensions(v.Wrap, content)
-	_, err := gui.g.SetView(v.Name(), x0, y0, x1, y1, 0)
+	x0, y0, _, y1 := v.Dimensions()


There's currently an issue with the resizing when we have the setup of commit message panel and description below it. When we cycle through commits the resizing of the description goes wrong because it can't seem to calculate the correct height (see video).

This workaround works for this particular usecase because with the new setup we don't have to resize the height vertically (still resizes horizontally though). I don't know if this will be an issue with other popups though, what do you suggest we do here?

simplescreenrecorder-2023-02-11_15.12.49.mov

We can't rely on the existing function because we need to resize the message and description view independently, but I still would like us to have code for making the commit description view taller if there's more content there. I would have a separate function specifically for resizing both the message and description views. We can also say within resizeCurrentPopupPanel that if the current view is the message or description view then we call our new resize method

seand52 · 2023-02-11T16:12:27Z

pkg/gui/controllers/helpers/commits_helper.go

+}
+
+func (self *CommitsHelper) UpdateCommitPanelView(message string, description string) {
+	// first try the passed in message, if not fallback to context -> view in that order


This function is called from the files controller/local commits controller before opening the panel to set the messages. It needs to be done from there because the message will come from different places (for reword it comes through the git command, but for normal commits it comes from context).

Ideally I would to simply set whatever message and description comes through the args. However, From what I've seen, when a commit fails, the commit message context is cleared at some point, is that correct? Since that's the case, we need to fallback to render whatever is in the view, because that's not cleared. It would be much simpler if the context wasn't cleared when there is an error though 🤔

I still prefer to the clear the context whenever we need to close is given that depending on what's in the view always ends up causing issues down the line.

If the commit fails, I would not expect the message to go away. Is that currently happening?

seand52 · 2023-02-11T16:14:51Z

@jesseduffield I've made some additional changes/fixes and run all the tests locally. I've rebased and run the unit and new integration tests are all passing, but there are a couple of old tests that fail but they seem unrelated, it could be a local configuration thing. Can we run the pipeline again?

Feel free to have a look now. I left a couple of comments in the PR, would be good to get your point of view on those topics to see if there's a simpler solution:

Ryooooooga · 2023-02-14T09:41:21Z

pkg/config/user_config.go

+	ConfirmAlt2                  string   `yaml:"confirm-alt2"`
 	ConfirmAlt1                  string   `yaml:"confirm-alt1"`


Suggested change

ConfirmAlt2 string `yaml:"confirm-alt2"`

ConfirmAlt1 string `yaml:"confirm-alt1"`

ConfirmAlt1 string `yaml:"confirm-alt1"`

ConfirmAlt2 string `yaml:"confirm-alt2"`

please update Config.md.

Ryooooooga · 2023-02-14T10:02:53Z

pkg/gui/controllers/commit_message_controller.go

+func (self *CommitMessageController) setCommitMessageAtIndex(index int) error {
+	msg, err := self.git.Commit.GetCommitMessageFromHistory(index)
+	if err != nil {
+		if err.Error() == "invalid commit index" {


diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index e7ef056c..3758dbb7 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -178,11 +178,13 @@ func (self *CommitCommands) CreateFixupCommit(sha string) error { return self.cmd.New(fmt.Sprintf("git commit --fixup=%s", sha)).Run() } +var ErrInvalidCommitIndex = errors.New("invalid commit index") + func (self *CommitCommands) GetCommitMessageFromHistory(value int) (string, error) { hash, _ := self.cmd.New(fmt.Sprintf("git log -1 --skip=%d --pretty=%%H", value-1)).DontLog().RunWithOutput() formattedHash := strings.TrimSpace(hash) if len(formattedHash) == 0 { - return "", errors.New("invalid commit index") + return "", ErrInvalidCommitIndex } return self.GetCommitMessage(formattedHash) }

Suggested change

if err.Error() == "invalid commit index" {

if err == git_commands.ErrInvalidCommitIndex {

Ryooooooga · 2023-02-14T10:05:06Z

pkg/commands/git_commands/commit.go

 }
+
+func (self *CommitCommands) GetCommitMessageFromHistory(value int) (string, error) {
+	hash, _ := self.cmd.New(fmt.Sprintf("git log -1 --skip=%d --pretty=%%H", value-1)).DontLog().RunWithOutput()


git log fails when a repository is empty.

Suggested change

hash, _ := self.cmd.New(fmt.Sprintf("git log -1 --skip=%d --pretty=%%H", value-1)).DontLog().RunWithOutput()

hash, _, err := self.cmd.New(fmt.Sprintf("git log -1 --skip=%d --pretty=%%H", value-1)).DontLog().RunWithOutputs()

if err != nil {

return "", ErrInvalidCommitIndex

}

Ryooooooga · 2023-02-14T10:07:15Z

pkg/gui/controllers/commit_message_controller.go

+			self.context().SetIsAtFirstCommit(true)
+			return nil
+		}
+		return self.c.ErrorMsg(self.c.Tr.CommitWithoutMessageErr)


Is this error message appropriate?

Ryooooooga · 2023-02-14T10:09:23Z

pkg/gui/controllers/helpers/commits_helper.go

+
+func (self *CommitsHelper) UpdateCommitPanelView(message string, description string) {
+	// first try the passed in message, if not fallback to context -> view in that order
+	if message != "" {


message can be empty.

jesseduffield · 2023-02-15T10:41:46Z

@seand52 I've spent some time with your PR as a way to better understand/review it and I'd like to push some commits. Is that alright with you?

seand52 · 2023-02-15T10:50:34Z

@seand52 I've spent some time with your PR as a way to better understand/review it and I'd like to push some commits. Is that alright with you?

Sure, feel free to push changes! The latest changes are all pushed 👍

jesseduffield · 2023-02-15T11:15:47Z

Sweet, pushed :) Let me know if you have any questions about those changes.

My main hangup now is around the context stuff and how we're pushing both contexts at once. I wanna spend a bit more time thinking if there's a better way to go about that.

jesseduffield · 2023-04-29T10:06:42Z

I'm pushing some small improvements. Haven't decided whether this should go in before the big refactor PR or if the refactor PR should go in first and I should rebase this on top of that. I'll probably do the latter (don't worry about handling that rebasey stuff yourself @seand52 , I won't inflict that upon you!)

… panel When we use the one panel for the entire commit message, its tricky to have a keybinding both for adding a newline and submitting. By having two panels: one for the summary line and one for the description, we allow for 'enter' to submit the message when done from the summary panel, and 'enter' to add a newline when done from the description panel. Alt-enter, for those who can use that key combo, also works for submitting the message from the description panel. For those who can't use that key combo, and don't want to remap the keybinding, they can hit tab to go back to the summary panel and then 'enter' to submit the message. We have some awkwardness in that both contexts (i.e. panels) need to appear and disappear in tandem and we don't have a great way of handling that concept, so we just push both contexts one after the other, and likewise remove both contexts when we escape.

When cycling history, we want to make it so that upon returning to the original prompt, you get your text back. Importantly, we don't want to use the existing preservedMessage field for that because that's only for preserving a NEW commit message, and we don't want the history stuff of the commit reword flow to overwrite that.

We now refresh the staging panel when doing an unscoped refresh, so that if we commit from the staging panel we escape back to the files panel if need be. But that causes flickering when doing an unscoped refresh from other contexts, because the refreshStagingPanel function assumes that the staging panel has focus. So we're adding a guard at the top of that function to early exit if we don't have focus.

jesseduffield · 2023-04-30T02:22:53Z

Merged! Thanks again @seand52 for making this

seand52 changed the title ~~Revamp commit message~~ Revamp commit message window Jan 25, 2023

seand52 force-pushed the revamp-commit-message branch from acf6fb4 to 4099305 Compare January 25, 2023 20:38

jesseduffield requested changes Jan 26, 2023

View reviewed changes

seand52 force-pushed the revamp-commit-message branch 7 times, most recently from 7b938cb to df231a1 Compare February 4, 2023 14:48

jesseduffield requested changes Feb 5, 2023

View reviewed changes

pkg/gui/files_panel.go Outdated Show resolved Hide resolved

pkg/gui/context.go Outdated Show resolved Hide resolved

pkg/gui/context.go Outdated Show resolved Hide resolved

pkg/gui/controllers/helpers/commits_helper.go Outdated Show resolved Hide resolved

seand52 force-pushed the revamp-commit-message branch 2 times, most recently from b10cd60 to 6471c51 Compare February 6, 2023 21:03

mark2185 mentioned this pull request Feb 7, 2023

Inconsistent behavior when writing commit comments #1056

Closed

seand52 force-pushed the revamp-commit-message branch from 0d7d9e9 to bb03c19 Compare February 7, 2023 18:34

seand52 marked this pull request as ready for review February 7, 2023 18:36

seand52 requested a review from jesseduffield February 7, 2023 18:36

mark2185 mentioned this pull request Feb 8, 2023

tag editor layout #2423

Closed

seand52 force-pushed the revamp-commit-message branch 2 times, most recently from baa7ac8 to 9b4fb9c Compare February 11, 2023 16:04

seand52 commented Feb 11, 2023

View reviewed changes

mark2185 mentioned this pull request Feb 11, 2023

Pull patch reword commit #2436

Closed

6 tasks

jesseduffield mentioned this pull request Feb 12, 2023

Ask if user wants to create empty commit when there are no staged changes #2439

Open

Ryooooooga reviewed Feb 14, 2023

View reviewed changes

jesseduffield force-pushed the revamp-commit-message branch from 2efc39b to 0a4f529 Compare April 29, 2023 03:42

mark2185 mentioned this pull request Apr 29, 2023

Multiline commit messages #28

Closed

jesseduffield force-pushed the revamp-commit-message branch from 3d6d781 to 16bb777 Compare April 29, 2023 10:05

jesseduffield force-pushed the revamp-commit-message branch from db2ba37 to e3ccd0d Compare April 30, 2023 02:11

seand52 and others added 3 commits April 30, 2023 12:17

jesseduffield force-pushed the revamp-commit-message branch from e3ccd0d to 9adbef4 Compare April 30, 2023 02:17

jesseduffield merged commit 8ed29f2 into jesseduffield:master Apr 30, 2023

Saafo mentioned this pull request May 15, 2023

About the new commit message panel #2631

Closed

mark2185 mentioned this pull request Nov 4, 2023

Commit message history on commit #199

Closed

		ConfirmAlt2 string `yaml:"confirm-alt2"`
		ConfirmAlt1 string `yaml:"confirm-alt1"`

	if err.Error() == "invalid commit index" {
	if err == git_commands.ErrInvalidCommitIndex {

Uh oh!

Conversation

seand52 commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mark2185 commented Jan 25, 2023

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ryooooooga commented Jan 26, 2023

Uh oh!

jesseduffield commented Jan 26, 2023

Uh oh!

mark2185 commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seand52 commented Feb 4, 2023

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seand52 commented Feb 7, 2023

Uh oh!

mark2185 commented Feb 7, 2023

Uh oh!

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

Uh oh!

Uh oh!

JonatasAmaral commented Feb 8, 2023

Uh oh!

mark2185 commented Feb 8, 2023

Uh oh!

seand52 commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mark2185 commented Feb 10, 2023

Uh oh!

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

Uh oh!

Uh oh!

seand52 commented Feb 10, 2023

Uh oh!

jesseduffield commented Feb 10, 2023

Uh oh!

seand52 Feb 11, 2023

Choose a reason for hiding this comment

Uh oh!

jesseduffield Feb 15, 2023

Choose a reason for hiding this comment

Uh oh!

seand52 Feb 11, 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!

jesseduffield Feb 15, 2023

Choose a reason for hiding this comment

Uh oh!

seand52 commented Feb 11, 2023

Uh oh!

Ryooooooga Feb 14, 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!

Ryooooooga Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

Ryooooooga Feb 14, 2023

Choose a reason for hiding this comment

seand52 commented Jan 25, 2023 •

edited

Loading

mark2185 commented Jan 26, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

seand52 commented Feb 8, 2023 •

edited

Loading

jesseduffield commented Feb 10, 2023 •

edited

Loading

seand52 Feb 11, 2023 •

edited

Loading

Ryooooooga Feb 14, 2023 •

edited

Loading