Skip to content

Conversation

@weltkante
Copy link
Contributor

@weltkante weltkante commented Jun 5, 2020

Fixes #3309

Proposed changes

Loading an image from a stream passes ownership, do not dispose the stream

  • Fix regression in ImageEditor
  • Fix regression in ImageListImageEditor
  • Add test to ensure BitmapEditor doesn't regress

Customer Impact

Using ImageEditor or ImageListImageEditor through a property grid or designer was producing partially defunct images whose underlying stream was disposed, depending on the operation performed on the image this could load to errors.

Regression?

Yes

Risk

unlikely to have any risk, this restores behavior from before the regression

Before

Image loaded from ImageEditor or ImageListImageEditor could throw exceptions on various operations

After

Image loaded from ImageEditor or ImageListImageEditor should operate normally

Test methodology

Ensure unit tests perform an operation accessing the underlying stream.

Microsoft Reviewers: Open in CodeFlow

@weltkante weltkante requested a review from a team as a code owner June 5, 2020 20:07
@ghost ghost assigned weltkante Jun 5, 2020
@weltkante
Copy link
Contributor Author

weltkante commented Jun 5, 2020

ImageListImageEditor has the same regression, but writing a test is harder since the load method is private. I remember you had some utilities for this but I can't find them easily, don't know where to look ... found it :-)

@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #3404 into master will decrease coverage by 30.95110%.
The diff coverage is 60.00000%.

@@                 Coverage Diff                  @@
##              master       #3404          +/-   ##
====================================================
- Coverage   65.14931%   34.19821%   -30.95110%     
====================================================
  Files           1328         890         -438     
  Lines         489778      253832      -235946     
  Branches       40042       36793        -3249     
====================================================
- Hits          319087       86806      -232281     
+ Misses        165265      162258        -3007     
+ Partials        5426        4768         -658     
Flag Coverage Δ
#Debug 34.19821% <60.00000%> (-30.95110%) ⬇️
#production 34.19821% <60.00000%> (+0.01760%) ⬆️
#test ?

@RussKie
Copy link
Contributor

RussKie commented Jun 9, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RussKie RussKie merged commit 75f13d7 into dotnet:master Jun 9, 2020
@ghost ghost added this to the 5.0 Preview7 milestone Jun 9, 2020
@RussKie
Copy link
Contributor

RussKie commented Jun 9, 2020

Thank you

@ghost ghost locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An exception occurred when adding an image to the BackgroundImage property

2 participants