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

Improve "Limit number of commits" option #8084

Merged
merged 1 commit into from Jul 20, 2020
Merged

Improve "Limit number of commits" option #8084

merged 1 commit into from Jul 20, 2020

Conversation

bansan85
Copy link
Contributor

@bansan85 bansan85 commented May 8, 2020

Fixes #8083

Proposed changes

  • It shouldn't be necessary to restart GE to apply the new value.
  • Disable limitation should not be an undocumented feature.

Screenshots

Before

image

After

image

Test methodology

None

Test environment(s)

  • Git Extensions 3.3.0
  • Build d47d27e
  • Git 2.26.2.windows.1
  • Microsoft Windows NT 6.2.9200.0
  • .NET Framework 4.8.4150.0
  • DPI 96dpi (no scaling)

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

@ghost ghost assigned bansan85 May 8, 2020
@mstv
Copy link
Member

mstv commented May 8, 2020

Is the limit applied now?

The label is too short now IMO, i.e. add " to be loaded" or the like.

I guess disabling will result in 🤔:

grafik

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #8084 into master will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #8084      +/-   ##
==========================================
- Coverage   52.75%   52.74%   -0.02%     
==========================================
  Files         866      866              
  Lines       62689    62697       +8     
  Branches    11300    11302       +2     
==========================================
- Hits        33070    33067       -3     
- Misses      26977    26981       +4     
- Partials     2642     2649       +7     
Flag Coverage Δ
#production 40.73% <33.33%> (-0.01%) ⬇️
#tests 94.20% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@bansan85
Copy link
Contributor Author

bansan85 commented May 8, 2020

Is the limit applied now?

It's the same behavior than before (except it's applied when the window is closed) :

  • if 0 then no limitation,
  • if not 0 then limitation is applied,

It's just that if you uncheck the option, the limitation is forced to disable.

The label is too short now IMO, i.e. add " to be loaded" or the like.

Are you sure ? I like this short text. If it not bothing you, I would like a second opinion.

I guess disabling will result in 🤔:

grafik

No : unchecking the checkbox disables the NumeripUpDown but it does not force the value in NumericUpDown to 0. It's in case someone unchecks by mistake, it's easy reversable. But if you uncheck the box and close the window, the value is definitly 0.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

LGTM

RussKie
RussKie previously approved these changes Jun 16, 2020
@pmiossec
Copy link
Member

The label is too short now IMO, i.e. add " to be loaded" or the like.

Are you sure ? I like this short text. If it not bothing you, I would like a second opinion.

When this PR was discussed, I was pretty sure we were talking of the option placed in the View => Set advanced filter which is really similar.
In that case, either of the label was good for me.

But I just realized that it is another option that was discussed about here. And I must admit that I agree with @mstv and because we don't have a context here, the label must be very explicit for the user and the one chosen here is not clear enough.

Sorry to be late in the discussion...

@RussKie RussKie dismissed their stale review June 16, 2020 14:32

Per other feedback

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jun 16, 2020
@ghost ghost added the 💤 status: no recent activity The issue is stale label Jul 16, 2020
@ghost
Copy link

ghost commented Jul 16, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs.

@ghost ghost removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity 💤 status: no recent activity The issue is stale labels Jul 18, 2020
@bansan85
Copy link
Contributor Author

Sorry, I forget about this PR :/

I changed the text of the option to "Limit number of commits to be loaded".

Copy link
Member

@pmiossec pmiossec left a comment

Choose a reason for hiding this comment

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

Thank you. I forgot also 😅

@RussKie RussKie merged commit 719c55b into gitextensions:master Jul 20, 2020
@ghost ghost added this to the 4.0 milestone Jul 20, 2020
@bansan85 bansan85 deleted the 8083-ImproveLimitCommitsOption branch July 20, 2020 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve option: "Limit number of commits that will be loaded at startup"
4 participants