GMapPolygon, GMapRoute: Fill Brush, Stroke Pen are Disposed

Topics: Bugs, Windows Forms
May 7, 2013 at 6:50 PM
Hi radioman!

It took me a while to track this one down, I but I think I've gotten to the bottom of it.

My application is providing its own Brush instance that is used by all GMapPolygon instances. But during the GMapPolygon.Dispose() method, my Brush is being disposed, which causes an exception when another polygon tries to use it later.

For example, suppose that I use System.Drawing.Brushes.Blue as a polygon fill... the current code will call Dispose on this system resource which should never be disposed.



I think the solution is pretty straightforward though. First, the GMapPolygon/GMapRoute should not dispose their Brush/Pen. Instead, they can provide default brush/pen instances that are static and do not need to be explicitly disposed.

For example, the Fill/Stroke properties could be written like this:

GMapPolygon:
      /// <summary>
      /// specifies how the outline is painted
      /// </summary>
      [NonSerialized]
#if !PocketPC
      private static readonly Pen DefaultStroke = new Pen(Color.FromArgb(155, Color.MidnightBlue));
#else
      private static readonly Pen DefaultStroke = new Pen(Color.MidnightBlue);
#endif
      public Pen Stroke = DefaultStroke;

      /// <summary>
      /// background color
      /// </summary>
      [NonSerialized]
#if !PocketPC
      private static readonly Brush DefaultFill = new SolidBrush(Color.FromArgb(155, Color.AliceBlue));
#else
      private static readonly Brush DefaultFill = new System.Drawing.SolidBrush(Color.AliceBlue);
#endif
      public Brush Fill = DefaultFill;
GMapRoute:
/// <summary>
        /// specifies how the outline is painted
        /// </summary>
        [NonSerialized]
#if !PocketPC
        private static readonly Pen DefaultStroke = new Pen(Color.FromArgb(144, Color.MidnightBlue));
#else
        private static readonly Pen DefaultStroke = new Pen(Color.MidnightBlue);
#endif
        public Pen Stroke = DefaultStroke;
And finally, remove from GMapPolygon.Dispose():
Stroke.Dispose();
Fill.Dispose();
And remove from GMapRoute.Dispose():
Stroke.Dispose();
This would allow an application to provide its own brushes and pens, and then manage the lifetime of its own objects. Additionally, GMap becomes more lightweight because, by default, it only has 2 Pens and 1 Brush that are shared among all of the GMapPolygon and GMapRoute instances.

I have tested these changes, and they work. Would you like me to create a fork?
Coordinator
May 7, 2013 at 7:39 PM
sounds like a plan, i'll test it later
Jun 28, 2013 at 12:13 PM
Hi radioman,

I am sorry if I misunderstood. Would you like to me to create a fork for these changes? Or are you able to make these changes?

Thanks!
Coordinator
Jun 28, 2013 at 2:05 PM
..but what if i want polygons with different colors ;/
Jun 28, 2013 at 2:16 PM
Edited Jun 28, 2013 at 2:17 PM
// Different colors under new system
polygon1.Fill = new SolidBrush(Color.Black); //Application created this brush, application should dispose it
polygon2.Fill = new SolidBrush(Color.Blue); //Application created this brush, application should dispose it
polygon3.Fill = new SolidBrush(Color.Yellow); //Application created this brush, application should dispose it

//Because under the current system
polygon4.Fill = new SolidBrush(Color.Red);
polygon5.Fill = polygon4.Fill; //polygon5 will be broken after polygon4 is disposed
If you would like the polygon to dispose the brush, then maybe the polygon should clone the brush and dispose the clone?
Coordinator
Jun 28, 2013 at 3:13 PM
ok, done
Jun 28, 2013 at 4:05 PM
You da man!

Thanks!
Feb 4, 2014 at 1:09 PM
Did this change affect the GMapPolygon.Stroke.Brush property? I'm having issues where all my Polygons have the same brush colour as the last one added to the overlay.
I've just set them to black for now so its not a big issue.
Jun 19, 2014 at 11:41 AM
Edited Jun 19, 2014 at 11:53 AM
Concerning Ruttager's problem, I had the same.

Although to me it applies to the last added route in GmapRoute. I debugged and whenever I change the stroke the defaultstroke gets modified despite being readonly. In fact all instances of Gmaproute stroke property point to the defaultstroke, hence they all will be the same. I'm sure same issue is for polygons.

I changed the declaration of the stroke in GmapRoute.cs

Old:
Public Pen Stroke = DefaultStroke;
New:
Public Pen Stroke = (Pen)DefaultStroke.Clone();
Now color for each route can be changed.

Probably fix applies for GmapRoute, GmapPolygon and GmapTooltip.
Also probably the fill, font, format and foreground property have the same issue.

Edit:
Alright now I understand more the problem of the topic starter. Now you should create a new brush in your calling object. I don't find it pretty however because in my project I was only changing properties inside the Brush object which now fail after this change because in fact you are modifying the static default property. You think you are dealing with a property that has an own instance because it is not static, but in fact you are modifying a variable that is pointing to a static variable. I would suggest to use my solution. Makes it a bit less lightweight although I'm not too worried about that.
Jun 19, 2014 at 1:42 PM
Hi JP!

I like your solution because it would still allow an application to use its own brushes/pens, or allows the application to directly modify GMap's clones. So the degree of lightweightedness, while probably only important to mobile, is still entirely up to the implementing application.

Your solution works for me so long as we let Garbage Collection take care of the clean-up (i.e. GMap does not ever explicitly dispose the pens/brushes).


Perhaps radioman will chime in here...
Jun 19, 2014 at 3:08 PM
Thanks mattgerg,

You could restore some of the lightweightness by adding a getter and setter which only clones or instantiates when the variable is still null. Then this cloning won't be done when you create a brush in your implementation, but will be done when you access the getter. Something like this:
private Pen _stroke = null;
Public Pen Stroke
{
  get
  {
    if (_stroke == null)
      _stroke = (pen)DefaultStroke.Clone();
    return _stroke;
  }
  set
  {
    _stroke = value;
  }
}