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

Correct padding increase #8819

Merged
merged 1 commit into from Feb 6, 2021
Merged

Correct padding increase #8819

merged 1 commit into from Feb 6, 2021

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Feb 4, 2021

#8557 introduced new margins and borders, however those margins and borders are not correctly calculated in scaled environments, i.e. scale factor >100%.

@ivangrek in #8732 has provided probably a more "correct" fix, however this is a smaller and more targeted fix.

Resolves #8698
Closes #8732

Screenshots

Before

image

After

image

Test methodology

  • manual

image


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

gitextensions#8557 introduced new margins and borders, however those margins
and borders are not correctly calculated in scaled environments, i.e.
scale factor >100%.

gitextensions#8732 is probably a more "correct" fix, however this is a smaller more
targeted fix.

Resolves gitextensions#8698
Closes gitextensions#8732
@ghost ghost assigned RussKie Feb 4, 2021
@ghost
Copy link

ghost commented Feb 4, 2021

Why does it work? 😄

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #8819 (ed9e005) into master (e3d1bee) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8819      +/-   ##
==========================================
+ Coverage   56.10%   56.13%   +0.03%     
==========================================
  Files         919      919              
  Lines       65524    65529       +5     
  Branches    11997    11997              
==========================================
+ Hits        36759    36784      +25     
+ Misses      25770    25754      -16     
+ Partials     2995     2991       -4     
Flag Coverage Δ
production 43.29% <100.00%> (+0.04%) ⬆️
tests 94.45% <ø> (ø)

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

@RussKie
Copy link
Member Author

RussKie commented Feb 4, 2021

Why does it work? 😄

As described in #8698 (comment) something in the layout engine triggers multiple padding scaling:

There's definitely something in our code that scales paddings 4(!) times: 1 -> 2 -> 3 -> 4 -> 6.

this.MainSplitContainer.Panel1.ResumeLayout(false); // 1 -> 2px
...
this.MainSplitContainer.ResumeLayout(false); // 2 -> 3px
...
this.toolPanel.ResumeLayout(false); // 3 -> 4px
...
this.ResumeLayout(false); // 4 -> 6px

In this change I force the padding to 1 after all the layout is done, thus making it expected.

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.

+1
I would prefer #8732 but can live with this fix

@RussKie
Copy link
Member Author

RussKie commented Feb 5, 2021

I'd like to reduce amount of changes until after we migrate to .NET 5. I expect some thing may actually change, and some things may require an update.

@RussKie
Copy link
Member Author

RussKie commented Feb 6, 2021

We can revisit the implementation after the migration.

@RussKie RussKie merged commit e40b0fc into gitextensions:master Feb 6, 2021
@RussKie RussKie deleted the fix_8732 branch February 6, 2021 01:47
@ghost ghost added this to the 3.6 milestone Feb 6, 2021
@mdonatas
Copy link
Contributor

mdonatas commented Feb 9, 2021

image

this seems like the popular case of "fix one, break another" :)
there's now fat padding around main controls

@ghost
Copy link

ghost commented Feb 9, 2021

there's now fat padding around main controls

This is conceived, but I agree, we can reduce by 2 pixels to 8 pixels like in VS.

@mdonatas
Copy link
Contributor

mdonatas commented Feb 9, 2021

to 8px? why not 0px? I'm running an older build where IMO it looks the best.
Even in VS you don't get a thick padding on all sides and what's more important in VS say Solution Explorer is docked (you can drag it out) so you kinda need to show that visually. I've just checked Slack, Notepad++, Windows Terminal, Windows Explorer and in there controls go to the sides of the window - no wasted space.

image

Also in VS when you don't dock, there is no wasteful padding
image

@ghost
Copy link

ghost commented Feb 9, 2021

image

@ghost
Copy link

ghost commented Feb 9, 2021

to 8px? why not 0px? I'm running an older build where IMO it looks the best.

In this case, it is necessary to delete everywhere. And use one line as separator and use color for selection.

@mstv
Copy link
Member

mstv commented Feb 9, 2021

(This issue deals with the DPI scaling.)

We have started a very similar discussion in #8711.

MSVS vertical tab
There is no additional border left to the vertical tabs in MSVS. The bars to the left belong to the tabs (maybe in analogy to the Windows task bar).
taskbar
Perhaps MSVS is somewhat too special to be taken as the example UI.

mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Feb 18, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Mar 16, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Mar 16, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Mar 19, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Apr 2, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Oct 15, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Oct 16, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Oct 25, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Oct 27, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Oct 29, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Oct 30, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Nov 23, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Dec 23, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Dec 23, 2021
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Feb 24, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Feb 28, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Apr 7, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Aug 24, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Sep 27, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Sep 29, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Nov 7, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Nov 10, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Nov 12, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Nov 17, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Nov 20, 2022
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Jan 1, 2023
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Jan 17, 2023
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Feb 10, 2023
mdonatas added a commit to mdonatas/gitextensions that referenced this pull request Mar 23, 2023
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.

Incorrect borders when app run in >100% scaling factor
4 participants