Dispose stream after creating icon?

[hardcodet]

FYI @punker76 @Lakritzator

I've had this PR from a fellow developer dangling around for quite a while:

I didn't dispose that stream because the Icon now owns it, which should take care about actually cleaning it up. I obviously can't guarantee that, but by closing it myself right after creating the Icon instance, I might mess things up for the Icon.

OTOH, I was reported potential memory leaks when using the NotifyIcon to do animations. What's your 0.02$ on this?

[Lakritzator]

For bitmap it's bad practice to dispose the stream, I'm sure it's the same for icons.

The workaround for Bitmap is do something like:

using (stream)
using (var bitmap= new Bitmap(stream)) {
 return new Bitmap(bitmap);
}

I'm quite sure it's similar for icons! Also using icon with a stream will AFAIK not support some of the newer features.

I'm sure I have code for that, will report back.

[punker76]

@Lakritzator But using does the same, it calls the Dispose of the stream. Isn't it?

[Lakritzator]

Btw: This actually also touches the following PR, it seems someone invested a lot of time :grin: https://bitbucket.org/hardcodet/notifyicon-wpf/pull-requests/11/added-different-ways-to-set-the-icon/diff

Which came from: https://bitbucket.org/hardcodet/notifyicon-wpf/pull-requests/9/changed-imagesource-to-bitmapsource/diff

It's an overlap, as the stream "issue" is in both.

[Lakritzator]

@Lakritzator But using does the same, it calls the Dispose of the stream. Isn't it?

Yes, it's not about the dispose, the point in the example code is that I create a new bitmap of the bitmap which was created from the stream, stupid hack but only than one can dispose the stream.

[hardcodet]

@Lakritzator The snippet there looks dangerous. Despite the wrapping, you return a new Bitmap that will operate on a disposed object (the wrapped Bitmap). If that works, it still looks like a timebomb that rather coincidentally works... Thoughts?

[Lakritzator]

I believe that this is not a simple question, which it somehow never is. :cry:

In this example every time this is called, a stream is opened for something which is already there. When using this from animations, I could imagine caching it somehow. Also when using icons there is this size issue, as the render size should depend on the DPI settings. I believe there is room for improvements when we look at the icon handling.

Maybe we should focus on this "dispose" issue, see if we can solve this and than create something where we discuss what functionality we want to provide for the icons... like animations, resizing and different formats?

[hardcodet]

Btw: This actually also touches the following PR, it seems someone invested a lot of time 😁 https://bitbucket.org/hardcodet/notifyicon-wpf/pull-requests/11/added-different-ways-to-set-the-icon/diff

cough :)

Yes, let's revisit this one.

[hardcodet]

In this example every time this is called, a stream is opened for something which is already there. When using this from animations, I could imagine caching it somehow.

Yup, I was thinking about that, too. I guess we can assume that once an Icon is set, it's safe for us to close the stream we obtained for the previous one.

[Lakritzator]

@Lakritzator The snippet there looks dangerous. Despite the wrapping, you return a new Bitmap that will operate on a disposed object (the wrapped Bitmap). If that works, it still looks like a timebomb that rather coincidentally works... Thoughts?

AFAIK it's not, it internally really clones the first bitmap, Win32 behaviour... But we don't need this solution, let me think about it.

[Lakritzator]

Yes, let's revisit this one. Now with @punker76 having way more WPF experience than I had at that time, and even a gazillion times more experience with icons, I'm guessing we can now design a much better solution.