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

Fix leaking brushes #9566

Merged
merged 2 commits into from Sep 25, 2021
Merged

Fix leaking brushes #9566

merged 2 commits into from Sep 25, 2021

Conversation

NikolayXHD
Copy link
Member

@NikolayXHD NikolayXHD commented Sep 11, 2021

Fixes #9429

Proposed changes

  1. Return brush wrapper in situations when brush consumer is not certain whether it should dispose the brush.
    wrapper.Dispose method will not touch the underlying brush if it is system brush.
  2. Cache the native brushes returned by Win32 GetSysColorBrush override
  3. Render text input backround explicitly, because 2. somehow breaks default system rendering.

Test methodology

  • run GitExtensions
  • scroll revision grid back and forth, click on different revisions
  • observe GDI objects count using GDIView (thank you @pmiossec)

Test environment(s)

Windows 10

Merge strategy

  • Rebase merge

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

@ghost ghost assigned NikolayXHD Sep 11, 2021
@NikolayXHD NikolayXHD changed the base branch from master to release/3.5 September 11, 2021 02:32
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
Seem fine to me, I have only briefly run this.

GitExtUtils/GitUI/Theming/BrushContainer.cs Outdated Show resolved Hide resolved
@pmiossec
Copy link
Member

Return brush wrapper in situations when brush consumer is not certain whether it should dispose the brush.
wrapper.Dispose method will not touch the underlying brush if it is system brush.

Clever idea. I have checked with the portable version and from what I see with GdiView, it is working great and fix the issue.

Code seems good. I will give it another look later.

I was just wondering. On the project, do we prefer:

using(var foo = new Brush())
{}

or

using var foo = new Brush();

@gerhardol
Copy link
Member

I was just wondering. On the project, do we prefer:

using(var foo = new Brush())
{}

or

using var foo = new Brush();

The latter generally, but the case I checked here the first was still required (null check).

@RussKie
Copy link
Member

RussKie commented Sep 14, 2021

I've started to review, but got caught up with work. Will complete this week.

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.

👍 with some comments

GitExtUtils/GitUI/Theming/BrushContainer.cs Outdated Show resolved Hide resolved
GitExtUtils/GitUI/Theming/BrushContainer.cs Outdated Show resolved Hide resolved
GitExtUtils/GitUI/Theming/TabControlPaintContext.cs Outdated Show resolved Hide resolved
GitUI/Editor/Diff/DiffViewerLineNumberControl.cs Outdated Show resolved Hide resolved
GitUI/Theming/SystemBrushOverrides.cs Outdated Show resolved Hide resolved
@ghost ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Sep 15, 2021
@RussKie RussKie added this to the 3.5.4 milestone Sep 15, 2021
@RussKie
Copy link
Member

RussKie commented Sep 24, 2021

@NikolayXHD I've made few tweaks, please have a look.

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Sep 24, 2021
@NikolayXHD
Copy link
Member Author

NikolayXHD commented Sep 24, 2021

@NikolayXHD I've made few tweaks, please have a look.

Ok, meged the changes changes to respective commits, thank you.

@RussKie RussKie merged commit 78da08e into gitextensions:release/3.5 Sep 25, 2021
@RussKie
Copy link
Member

RussKie commented Sep 25, 2021

Thank you!

@gerhardol
Copy link
Member

Thanks!
Will this be merged to master too, even if themes are not possible yet with .NET5?
How should we remember this otherwise?

@NikolayXHD
Copy link
Member Author

Will this be merged to master too, even if themes are not possible yet with .NET5?

Sure thing :)

@NikolayXHD NikolayXHD mentioned this pull request Sep 26, 2021
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.

[NBug] An unexpected internal error occurred in the Win32 call: 80...
4 participants