-
-
Notifications
You must be signed in to change notification settings - Fork 887
Fixed Issue#1628 #1629
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
Fixed Issue#1628 #1629
Changes from 16 commits
7900b43
d48b152
7029b2f
1c45c1a
095ce41
acf9d85
8ec1013
ff4b269
cbca565
127e9dd
3f8bd3d
009e935
f0f0c08
a71ce19
f561053
34706fb
1c8dcef
e787ffa
d54ff0e
5704403
65808ae
5d2884e
afee881
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ namespace SixLabors.ImageSharp | |
| /// </summary> | ||
| public abstract partial class Image : IImage, IConfigurationProvider | ||
| { | ||
| private bool isDisposed; | ||
|
|
||
| private Size size; | ||
| private readonly Configuration configuration; | ||
|
|
||
|
|
@@ -80,8 +82,14 @@ internal Image( | |
| /// <inheritdoc /> | ||
| public void Dispose() | ||
| { | ||
| this.Dispose(true); | ||
| GC.SuppressFinalize(this); | ||
| if (this.isDisposed) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| this.DisposeManaged(); | ||
|
|
||
| this.isDisposed = true; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -140,15 +148,20 @@ public abstract Image<TPixel2> CloneAs<TPixel2>(Configuration configuration) | |
| protected void UpdateSize(Size size) => this.size = size; | ||
|
|
||
| /// <summary> | ||
| /// Disposes the object and frees resources for the Garbage Collector. | ||
| /// Internal routine for freeing managed resources called from <see cref="Dispose"/> | ||
| /// </summary> | ||
| /// <param name="disposing">Whether to dispose of managed and unmanaged objects.</param> | ||
| protected abstract void Dispose(bool disposing); | ||
| protected abstract void DisposeManaged(); | ||
|
||
|
|
||
| /// <summary> | ||
| /// Throws <see cref="ObjectDisposedException"/> if the image is disposed. | ||
| /// </summary> | ||
| internal abstract void EnsureNotDisposed(); | ||
| internal void EnsureNotDisposed() | ||
| { | ||
| if (this.isDisposed) | ||
| { | ||
| throw new ObjectDisposedException("Trying to execute an operation on a disposed image."); | ||
|
||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Accepts a <see cref="IImageVisitor"/>. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,10 @@ namespace SixLabors.ImageSharp | |
| /// Encapsulates a pixel-agnostic collection of <see cref="ImageFrame"/> instances | ||
| /// that make up an <see cref="Image"/>. | ||
| /// </summary> | ||
| public abstract class ImageFrameCollection : IEnumerable<ImageFrame> | ||
| public abstract class ImageFrameCollection : IDisposable, IEnumerable<ImageFrame> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow. I hadn't realized this wasn't already implementing the type. Good catch! |
||
| { | ||
| private bool isDisposed; | ||
|
|
||
| /// <summary> | ||
| /// Gets the number of frames. | ||
| /// </summary> | ||
|
|
@@ -21,7 +23,15 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame> | |
| /// <summary> | ||
| /// Gets the root frame. | ||
| /// </summary> | ||
| public ImageFrame RootFrame => this.NonGenericRootFrame; | ||
| public ImageFrame RootFrame | ||
| { | ||
| get | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericRootFrame; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the root frame. (Implements <see cref="RootFrame"/>.) | ||
|
|
@@ -36,7 +46,15 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame> | |
| /// </value> | ||
| /// <param name="index">The index.</param> | ||
| /// <returns>The <see cref="ImageFrame"/> at the specified index.</returns> | ||
| public ImageFrame this[int index] => this.NonGenericGetFrame(index); | ||
| public ImageFrame this[int index] | ||
| { | ||
| get | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericGetFrame(index); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Determines the index of a specific <paramref name="frame"/> in the <seealso cref="ImageFrameCollection"/>. | ||
|
|
@@ -52,14 +70,24 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame> | |
| /// <param name="source">The <seealso cref="ImageFrame"/> to clone and insert into the <seealso cref="ImageFrameCollection"/>.</param> | ||
| /// <exception cref="ArgumentException">Frame must have the same dimensions as the image.</exception> | ||
| /// <returns>The cloned <see cref="ImageFrame"/>.</returns> | ||
| public ImageFrame InsertFrame(int index, ImageFrame source) => this.NonGenericInsertFrame(index, source); | ||
| public ImageFrame InsertFrame(int index, ImageFrame source) | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericInsertFrame(index, source); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Clones the <paramref name="source"/> frame and appends the clone to the end of the collection. | ||
| /// </summary> | ||
| /// <param name="source">The raw pixel data to generate the <seealso cref="ImageFrame{TPixel}"/> from.</param> | ||
| /// <returns>The cloned <see cref="ImageFrame{TPixel}"/>.</returns> | ||
| public ImageFrame AddFrame(ImageFrame source) => this.NonGenericAddFrame(source); | ||
| public ImageFrame AddFrame(ImageFrame source) | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericAddFrame(source); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Removes the frame at the specified index and frees all freeable resources associated with it. | ||
|
|
@@ -91,23 +119,38 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame> | |
| /// <param name="index">The zero-based index of the frame to export.</param> | ||
| /// <exception cref="InvalidOperationException">Cannot remove last frame.</exception> | ||
| /// <returns>The new <see cref="Image{TPixel}"/> with the specified frame.</returns> | ||
| public Image ExportFrame(int index) => this.NonGenericExportFrame(index); | ||
| public Image ExportFrame(int index) | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericExportFrame(index); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates an <see cref="Image{T}"/> with only the frame at the specified index | ||
| /// with the same metadata as the original image. | ||
| /// </summary> | ||
| /// <param name="index">The zero-based index of the frame to clone.</param> | ||
| /// <returns>The new <see cref="Image{TPixel}"/> with the specified frame.</returns> | ||
| public Image CloneFrame(int index) => this.NonGenericCloneFrame(index); | ||
| public Image CloneFrame(int index) | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericCloneFrame(index); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a new <seealso cref="ImageFrame{TPixel}" /> and appends it to the end of the collection. | ||
| /// </summary> | ||
| /// <returns> | ||
| /// The new <see cref="ImageFrame{TPixel}" />. | ||
| /// </returns> | ||
| public ImageFrame CreateFrame() => this.NonGenericCreateFrame(); | ||
| public ImageFrame CreateFrame() | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericCreateFrame(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a new <seealso cref="ImageFrame{TPixel}" /> and appends it to the end of the collection. | ||
|
|
@@ -116,14 +159,53 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame> | |
| /// <returns> | ||
| /// The new <see cref="ImageFrame{TPixel}" />. | ||
| /// </returns> | ||
| public ImageFrame CreateFrame(Color backgroundColor) => this.NonGenericCreateFrame(backgroundColor); | ||
| public ImageFrame CreateFrame(Color backgroundColor) | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericCreateFrame(backgroundColor); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void Dispose() | ||
| { | ||
| if (this.isDisposed) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| this.DisposeManaged(); | ||
|
|
||
| this.isDisposed = true; | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public IEnumerator<ImageFrame> GetEnumerator() => this.NonGenericGetEnumerator(); | ||
| public IEnumerator<ImageFrame> GetEnumerator() | ||
| { | ||
| this.EnsureNotDisposed(); | ||
|
|
||
| return this.NonGenericGetEnumerator(); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator(); | ||
|
|
||
| /// <summary> | ||
| /// Throws <see cref="ObjectDisposedException"/> if the image frame is disposed. | ||
| /// </summary> | ||
| protected void EnsureNotDisposed() | ||
| { | ||
| if(this.isDisposed) | ||
| { | ||
| throw new ObjectDisposedException("Trying to execute an operation on a disposed image frame."); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Internal routine for freeing managed resources called from <see cref="Dispose"/> | ||
| /// </summary> | ||
| protected abstract void DisposeManaged(); | ||
|
|
||
| /// <summary> | ||
| /// Implements <see cref="GetEnumerator"/>. | ||
| /// </summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be removing the finalizer suppression in the base class.
https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#the-dispose-method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know
SuppressFinalizedo nothing if type has no finalizer, thanks!