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

Make showing commit message body in the revision graph optional #9301

Merged
merged 5 commits into from Jun 21, 2021

Conversation

jwfx
Copy link
Contributor

@jwfx jwfx commented Jun 18, 2021

Original code by @FodderMK

Fixes #9298

Proposed changes

  • Read revision graph configuration option that was previously removed
  • Enabled by default as it currently is without the option

Screenshots

image


✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned jwfx Jun 18, 2021
@jwfx
Copy link
Contributor Author

jwfx commented Jun 18, 2021

Hm, not sure what to make of that AppVeyor build break. Could use some guidance on what is missing here.

@pmiossec
Copy link
Member

Hm, not sure what to make of that AppVeyor build break. Could use some guidance on what is missing here.

The translation file is not good.
In the build log,you have the link to follow to get the command to run to fix it: https://github.com/gitextensions/gitextensions/wiki/Translations

new MenuCommand
{
Name = "showMultiLineCommitMessagesToolStripMenuItem",
Text = "Show multi-line commit messages",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Text = "Show multi-line commit messages",
Text = "Show commit body",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmiossec If this would also be ok for you, I will also change all the related variables, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I will also change all the related variables, etc.

Yes, please. For AppSettings, I prefer to be even more specific (and to use Delphi-case for the new setting):
"showmultilinecommitmessages" --> "ShowCommitBodyInRevisionGrid"

Copy link
Member

Choose a reason for hiding this comment

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

Should probably be clearer...

Suggested change
Text = "Show multi-line commit messages",
Text = "Show commit message body",

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

LGTM

new MenuCommand
{
Name = "showMultiLineCommitMessagesToolStripMenuItem",
Text = "Show multi-line commit messages",
Copy link
Member

Choose a reason for hiding this comment

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

I will also change all the related variables, etc.

Yes, please. For AppSettings, I prefer to be even more specific (and to use Delphi-case for the new setting):
"showmultilinecommitmessages" --> "ShowCommitBodyInRevisionGrid"

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

Probably OK, one question.
Need to be rebased.
Have not run.

@@ -849,7 +849,7 @@ private void RefreshSelection()
FillCommitInfo(selectedRevision);

// If the revision's body has been updated then the grid needs to be refreshed to display it
if (selectedRevision is not null && selectedRevision.HasMultiLineMessage && oldBody != selectedRevision.Body)
if (AppSettings.ShowCommitBodyInRevisionGrid && selectedRevision is not null && selectedRevision.HasMultiLineMessage && oldBody != selectedRevision.Body)
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct when toggling the option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also applied this from the original commit, but I think it is wrong.
Even if the option is disabled, the grid should be refreshed to display the multi-line indicator in case someone changed a commit message from single to multi-line, e.g. during amend.
This should probably be removed unless I'm wrong about the mechanics.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is correct. The comment above explains why.
[Toggle]ShowCommitBodyInRevisionGrid() calls RevisionGridControl.Refresh().

@jwfx jwfx force-pushed the 9298-option-disable-multi-line branch from 56c86b8 to 8cb54db Compare June 20, 2021 14:08
@jwfx
Copy link
Contributor Author

jwfx commented Jun 20, 2021

Need to be rebased.

Done

GitCommands/Settings/AppSettings.cs Outdated Show resolved Hide resolved
GitUI/UserControls/RevisionGrid/RevisionGridControl.cs Outdated Show resolved Hide resolved
@@ -849,7 +849,7 @@ private void RefreshSelection()
FillCommitInfo(selectedRevision);

// If the revision's body has been updated then the grid needs to be refreshed to display it
if (selectedRevision is not null && selectedRevision.HasMultiLineMessage && oldBody != selectedRevision.Body)
if (AppSettings.ShowCommitBodyInRevisionGrid && selectedRevision is not null && selectedRevision.HasMultiLineMessage && oldBody != selectedRevision.Body)
Copy link
Member

Choose a reason for hiding this comment

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

I think it is correct. The comment above explains why.
[Toggle]ShowCommitBodyInRevisionGrid() calls RevisionGridControl.Refresh().

jwfx and others added 3 commits June 20, 2021 19:40
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

LGTM, have not run

@mstv mstv changed the title Added support for showing multi-line commit messages in the revision graph Make showing commit message body in the revision graph optional Jun 21, 2021
@mstv mstv merged commit 2c32e7e into gitextensions:master Jun 21, 2021
@ghost ghost added this to the 3.6 milestone Jun 21, 2021
@mstv
Copy link
Member

mstv commented Jun 21, 2021

Thank you

@nickb-carallon
Copy link

Can this be cherry picked to the 3.5 branch? It seems like a minor change and I think there are quite a few people (inc. myself) who would really like to see it included in 3.5.2.

mstv added a commit to mstv/gitextensions that referenced this pull request Jul 13, 2021
@mstv
Copy link
Member

mstv commented Jul 13, 2021

#9374, @nickb-carallon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return the option to disable showing multi-line commit messages in the revision graph
6 participants