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

Add settings to configure blame display #6430

Merged
merged 11 commits into from May 4, 2019

Conversation

@pmiossec
Copy link
Member

commented Apr 2, 2019

Proposed changes

  • Add settings to customize blame display (see screenshot for list of settings)
  • Also improve a little more memory usage when building the commiter text

Screenshots

Before

image

After

image

Result with some settings enabled:

image

Test methodology

  • Manual

Test environment(s)

  • Git Extensions 3.1.0
  • Build 84b65c3
  • Git 2.21.0.windows.1
  • Microsoft Windows NT 10.0.17134.0
  • .NET Framework 4.7.3362.0
  • DPI 192dpi (200% scaling)
@ghost ghost added the 👓 status: needs review label Apr 2, 2019
@codecov

This comment has been minimized.

Copy link

commented Apr 2, 2019

Codecov Report

Merging #6430 into master will decrease coverage by 0.04%.
The diff coverage is 5.79%.

@@            Coverage Diff             @@
##           master    #6430      +/-   ##
==========================================
- Coverage   46.71%   46.67%   -0.05%     
==========================================
  Files         688      688              
  Lines       51887    51943      +56     
  Branches     6821     6827       +6     
==========================================
+ Hits        24241    24242       +1     
- Misses      26329    26384      +55     
  Partials     1317     1317
Flag Coverage Δ
#production 36.1% <5.79%> (-0.05%) ⬇️
#tests 97.52% <ø> (-0.03%) ⬇️
@codecov

This comment has been minimized.

Copy link

commented Apr 2, 2019

Codecov Report

Merging #6430 into master will increase coverage by 0.08%.
The diff coverage is 63.31%.

@@            Coverage Diff            @@
##           master   #6430      +/-   ##
=========================================
+ Coverage   47.02%   47.1%   +0.08%     
=========================================
  Files         696     697       +1     
  Lines       52403   52554     +151     
  Branches     6884    6891       +7     
=========================================
+ Hits        24644   24758     +114     
- Misses      26433   26465      +32     
- Partials     1326    1331       +5
Flag Coverage Δ
#production 36.47% <42.96%> (+0.03%) ⬆️
#tests 97.58% <100%> (+0.01%) ⬆️
@gerhardol

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Is it reasonable to address the CodeFactor warning?

@pmiossec

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Yes. When I will be in front of a computer 😊

Copy link
Member

left a comment

Seem OK to me (untested)

@RussKie

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

What is displayed when all new settings are off?

🤔 Just looking at the screenshot, we need to do few tweaks.

  • All "Hide xxx" must be turned into "Show xxx" to make it consistent with other menus, and make them sound positive (presence) as oppose to negative (absence).
  • All appsettings need to be tweaked accordingly.

I'd like to see tests for ProcessBlame and BuildAuthorLine

Thank you

@RussKie RussKie changed the title Add settings to configure blame display WIP: Add settings to configure blame display Apr 8, 2019
@pmiossec pmiossec force-pushed the pmiossec:user_blame_settings branch 2 times, most recently from aa2b36f to bd13ff7 Apr 9, 2019
@pmiossec pmiossec force-pushed the pmiossec:user_blame_settings branch 3 times, most recently from 71e76dd to 9eee294 Apr 22, 2019
@pmiossec

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

What is displayed when all new settings are off?

The UI doesn't allow that. When you uncheck date, author is check (and inverse is true also).

🤔 Just looking at the screenshot, we need to do few tweaks.

done.

I'd like to see tests for ProcessBlame and BuildAuthorLine

done.

@pmiossec pmiossec changed the title WIP: Add settings to configure blame display Add settings to configure blame display Apr 24, 2019
@pmiossec pmiossec closed this Apr 24, 2019
@pmiossec pmiossec reopened this Apr 24, 2019
@pmiossec pmiossec closed this Apr 24, 2019
@pmiossec pmiossec reopened this Apr 24, 2019
@pmiossec

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

I don't know if there is a problem with GitHub or AppVeyor but the build is no more triggered 😢

@RussKie RussKie closed this Apr 24, 2019
@ghost ghost removed the 👓 status: needs review label Apr 24, 2019
@RussKie RussKie reopened this Apr 24, 2019
@ghost ghost assigned RussKie Apr 24, 2019
@ghost ghost added the status: ready label Apr 24, 2019
@pmiossec

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@RussKie We will retry in some hours 😉

@RussKie RussKie self-requested a review Apr 28, 2019
Copy link
Member

left a comment

Very good, easy to follow the train of thought 👍

Few comments. Also I don't think the original screenshots are valid, could you please update them.
Thank you

GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
GitUI/UserControls/BlameControl.cs Show resolved Hide resolved
GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved
@@ -265,6 +270,10 @@ private void ProcessBlame(string filename, GitRevision revision, IReadOnlyList<O

GitBlameCommit lastCommit = null;

var dateTimeFormat = AppSettings.BlameHideAuthorTime
? CultureInfo.CurrentCulture.DateTimeFormat.ShortDatePattern
: CultureInfo.CurrentCulture.DateTimeFormat.ShortDatePattern + " " + CultureInfo.CurrentCulture.DateTimeFormat.ShortTimePattern;

This comment has been minimized.

Copy link
@RussKie

RussKie Apr 30, 2019

Member

We should also honour user's date presentation settings (Settings > Appearance > General)
image

This comment has been minimized.

Copy link
@pmiossec

pmiossec May 2, 2019

Author Member

I'm not against it's a new feature (as we didn't do that at the moment and this PR was just about adding some configuration on existing feature). Even if I'm pleased to do it, I will let it for another PR ;)

And, also, I see it as a distinct blame settings (why not initially set with this settings value) because we could like to display as relative in the application and the opposite in the blame (or the other way). Does it make sense?

This comment has been minimized.

Copy link
@RussKie

RussKie May 2, 2019

Member

Yes, no worries.
Just flagging that we have an inconsistent UX.

GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved

void BuildAuthorLine(GitBlameLine line)
private void BuildAuthorLine(GitBlameLine line, StringBuilder lineBuilder, string dateTimeFormat, string filename)

This comment has been minimized.

Copy link
@RussKie

RussKie Apr 30, 2019

Member

To make this more testable (e.g. less dependent on the shared state - AppSettings) I recommend passing in all AppSettings.* flags.
This way it is easy to add test for all sorts of permutations.

@pmiossec pmiossec force-pushed the pmiossec:user_blame_settings branch from 290db96 to db2bc4d May 2, 2019
@pmiossec pmiossec force-pushed the pmiossec:user_blame_settings branch from db2bc4d to 45a24ca May 2, 2019
@pmiossec

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Done most except 2 things!
Let's talk!!! 😄

@RussKie
RussKie approved these changes May 2, 2019
Copy link
Member

left a comment

Awesome work mate!

@RussKie RussKie merged commit 3d5da22 into gitextensions:master May 4, 2019
3 checks passed
3 checks passed
CodeFactor No issues found.
Details
WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@ghost ghost removed the 👓 status: needs review label May 4, 2019
@RussKie RussKie added this to the 3.2.0 milestone May 4, 2019
@pmiossec pmiossec deleted the pmiossec:user_blame_settings branch May 6, 2019
@RussKie

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Looks like the master is failing after this PR got merged
Could you please have a look https://ci.appveyor.com/project/gitextensions/gitextensions/builds/24310002 ?

@pmiossec

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

I saw that even if I didn't had a careful look at it.
I don't think it is caused by this PR. It seems not linked.
I made 2 PRs based on master that built successfully.

I don't know if it's possible to trigger another build before investigating...

@vbjay

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@RussKie

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@pmiossec

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@vbjay we can't close and open master 😉

@vbjay

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@pmiossec

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Yes. I thought I didn't have access but after sign in, I could have started a rebuild.

Good to know for the future.

Anyway, the build succeeded. 👍
Time to sleep 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.