Implement IDisposable in IImageProcessor<TPixel> instances. #990
Conversation
Codecov Report
@@ Coverage Diff @@
## master #990 +/- ##
=========================================
- Coverage 89.72% 89.6% -0.12%
=========================================
Files 1097 1097
Lines 48573 48621 +48
Branches 3421 3425 +4
=========================================
- Hits 43581 43569 -12
- Misses 4289 4297 +8
- Partials 703 755 +52
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #990 +/- ##
==========================================
+ Coverage 89.72% 89.74% +0.02%
==========================================
Files 1097 1098 +1
Lines 48573 48690 +117
Branches 3421 3433 +12
==========================================
+ Hits 43581 43698 +117
+ Misses 4289 4288 -1
- Partials 703 704 +1
Continue to review full report at Codecov.
|
| { | ||
| this.destination = cloningImageProcessor.CloneAndApply(); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
If the processor is not a ICloningImageProcessor<TPixel> we are disposing and re-creating it unnecessarily.
There was a problem hiding this comment.
Good catch, will fix. 👍
There was a problem hiding this comment.
Maybe I'm just too tired but I cannot see a workaround here. The processors are created from different images.
There was a problem hiding this comment.
Indeed, haven't noticed that!
Makes it harder, I will also keep thinking tomorrow.
| /// Disposes the object and frees resources for the Garbage Collector. | ||
| /// </summary> | ||
| /// <param name="disposing">Whether to dispose managed and unmanaged objects.</param> | ||
| protected virtual void Dispose(bool disposing) |
There was a problem hiding this comment.
If we expect unmanaged objects (at least in theory) we should consider adding a finalizer.
In Image, I have distinnguished the dispose implementation by naming our method DisposeImpl. Still wondering if it's a good approach or we should rather Dispose(bool disposing), even if the bool parameter is dummy.
There was a problem hiding this comment.
I'd say stick with convention and use Dispose(bool disposing)
There was a problem hiding this comment.
I didn't use a finalizer since I didn't want to use (or condone) unmanaged object use.
| this.MutateApply(useBounds); | ||
| } | ||
|
|
||
| this.cloningProcessorImpl.Verify(p => p.CloneAndApply(), Times.Once()); |
There was a problem hiding this comment.
@JimBobSquarePants oops. We should expect this.cloningProcessorImpl.Verify(p => p.Apply(), Times.Once()); here instead. To avoid conflicts, I'll leave it to you.
|
We can skip the failing test and merge this as is for now. I may have an idea for unifying the regular and the cloning processor logic, and I can add it in a follow up PR. |
…s#990) * Implement IDisposable and ensure inheritance calls base * add ImageProcessingContextTests and move some other test classes * loosen up tests and leave TODO notes
Prerequisites
Description
Touches #967. Makes
IImageProcessor<TPixel>inheritIDIsposableand ensures all implementations are correctly disposed.Also ensures any overriding methods are calling their base implementation.