2

I'm trying to get a better feeling for how to maintain subscription to a class which may swap (Change Strategies). I'll try to keep this directed even if the examples are contrived.

Assume there is a class Skin

public class Skin
{
  //Raised when the form needs to turn on/off a blinking light
  public event BlinkEventHandler BlinkEvent;
  //The back color that forms should use
  public Color BackColor{ get; protected set; }
}

When the application starts, it will read a directory full of configuration files for the different Skin classes. The user can switch the current skin at any time.

My current job uses a very strange strategy (IMO) that looks like this:

/// <summary>
/// Some class that can see when the Skin Changes
/// </summary>
public class SkinManager
{
  //Raised when the Skin changes
  public event SkinChangedEventHandler SkinChangedEvent;
  private static Skin currentSkin;
  public static Skin CurrentSkin {get;}

  public SkinManager(){/* gets a skin into currentSkin */}
  public void ChangeSkin()
  {
    //... do something to change the skin
    if(SkinChangedEvent != null)
    {
      SkinChangedEvent(this, new SkinChangedEventArgs(/*args*/));
    }
  }
}

/// <summary>
/// Some form that follows the Skinning Strategy
/// </summary>
public class SkinnedForm : Form
{
  private Skin skin;
  public SkinnedForm()
  {
    skin = SkinManager.CurrentSkin;
    if(skin != null)
    {
      skin.BlinkEvent += OnBlink;
    }
    SkinManager.SkinChangedEvent += OnSkinChanged;
  }

  private void OnSkinChanged(object sender, SkinChangedEventArgs e)
  {
    //unregister if we have a current skin
    //the local was to ensure that the form unsubscribes
    //when skin changes
    if(skin != null)
    {
       skin.BlinkEvent -= OnBlink;
    }
    skin = SkinManager.CurrentSkin;
    if(skin != null)
    {
       skin.BlinkEvent += OnBlink;
    }
    SkinChanged();
  }

  private void SkinChanged(){ Invalidate(); }

  private void OnBlink(object sender, BlinkEventArgs e)
  {
     //... do something for blinking
  }
}

I cannot believe this to be a good implementation and would instead want to see something like this:

/// <summary>
/// Some class that can see when the Skin Changes
/// </summary>
public class SkinManager
{
  //Raised when the Skin changes
  public event SkinChangedEventHandler SkinChangedEvent;
  //Relays the event from Skin
  public event BlinkEventHander BlinkEvent;
  private static Skin currentSkin;
  public static Skin CurrentSkin {get;}

  public SkinManager()
  {
    //... gets a skin into currentSkin
    currentSkin.BlinkEvent += OnBlink;
  }

  /// <summary>
  /// Relays the event from Skin
  /// </summary>
  private void OnBlink(object sender, BlinkEventArgs e)
  {
     if(BlinkEvent != null)
     {
       BlinkEvent(this, e);
     }
  }
  public void ChangeSkin()
  {
    //... do something to change the skin
    if(SkinChangedEvent != null)
    {
      SkinChangedEvent(this, new SkinChangedEventArgs(/*args*/));
    }
  }
}

/// <summary>
/// Some form that follows the Skinning Strategy
/// </summary>
public class SkinnedForm : Form
{
  //Do not need the local anymore
  //private Skin skin;
  public SkinnedForm()
  {
    SkinManager.CurrentSkin.BlinkEvent += OnBlink;
    SkinManager.SkinChangedEvent += OnSkinChanged;
  }

  private void OnSkinChanged(object sender, SkinChangedEventArgs e)
  {
    //Only register with the manager, so no need to deal with
    //subscription maintenance, could just directly to go SkinChanged();
    SkinChanged();
  }

  private void SkinChanged() { Invalidate(); }

  private void OnBlink(object sender, BlinkEventArgs e)
  {
     //... do something for blinking
  }
}

I'm not sure if that's clear, but mainly there is a local variable that is used strictly to ensure that we unsubscribe from events before subscribing the events on the new class. I view it as: we implemented the strategy pattern for skinning (pick the skinning strategy you want to use and run with it), but each strategy implementation has events which we are directly subscribing to. When the strategy changes, we want our subscribers to watch the correct publisher so we use the locals. Again, I think this is a terrible methodology.

Is there a name for the transformation I proposed by using the manager to monitor all the events of the class it manages and pass them along so that the strategy can change and the subscribers continue to listen for the correct event notifications? The code provided was created on the fly as I formed the question so forgive any errors.

3 Answers 3

1

Generally, when you want to create a proxy (wrapper) for a class which fires events, you need to unsubcribe (detach) the previous instance, swap with a new one, and then subscribe (attach) to its events.

Let's say your skin interface looks like this:

interface ISkin
{
    void RenderButton(IContext ctx);
    event EventHandler Blink;
}

Then the part where you change it needs to look like this:

public void SetSkin(ISkin newSkin)
{
    // detach handlers from previous instance
    DetachHandlers();

    // swap the instance
    _skin = newSkin;

    // attach handlers to the new instance
    AttachHandlers();
}

void DetachHandlers()
{
    if (_skin != null)
       _skin.Blink -= OnBlink;
}

void AttachHandlers()
{
    if (_skin != null)
       _skin.Blink += OnBlink;
}

Complete proxy would look something like this:

interface IChangeableSkin : ISkin
{
    event EventHandler SkinChanged;
}

public class SkinProxy : IChangeableSkin 
{
    private ISkin _skin; // actual underlying skin

    public void SetSkin(ISkin newSkin)
    {
        if (newSkin == null)
           throw new ArgumentNullException("newSkin");

        if (newSkin == _skin)
           return; // nothing changed

        // detach handlers from previous instance
        DetachHandlers();

        // swap the instance
        _skin = newSkin;

        // attach handlers to the new instance
        AttachHandlers();

        // fire the skin changed event
        SkinChanged(this, EventArgs.Empty);
    }

    void DetachHandlers()
    {
        if (_skin != null)
           _skin.BlinkEvent -= OnBlink;
    }

    void AttachHandlers()
    {
        if (_skin != null)
           _skin.BlinkEvent += OnBlink;
    }

    void OnBlink(object sender, EventArgs e)
    {
        // just forward the event
        BlinkEvent(this, e);
    }

    // constructor
    public SkinProxy(ISkin initialSkin)
    {
        SetSkin(initialSkin);
    }


    #region ISkin members

    public void RenderButton(IContext ctx)
    {
        // just calls the underlying implementation
        _skin.RenderButton(ctx);
    }

    // this is fired inside OnBlink
    public event EventHandler BlinkEvent = delegate { }; 

    #endregion


    #region IChangeableSkin members

    public event EventHandler SkinChanged = delegate { }; 

    #region
}

Your Form should only hold a reference to an implementation of a IChangeableSkin.

1
  • Awesome, thanks. This is exactly what I was proposing, but wanted to make sure that I wasn't off base. So this is known as proxying then? For some reason, they call it Provider here, but when I looked up the pattern, it didn't seem to match.
    – MPavlak
    Jun 8, 2011 at 14:51
1

Kind of complicated and the burden of switching is placed by the subscriber. That's not so good.

When swapping Skins it is possible for the old Skin to remove its event-subscribers and probably to attach them to the new Skin as well.

But a neater pattern might be a Skin-holder that doesn't change and that exposes the events.

2
  • I agree that it is not a good situation with what you have in place. Is the proposed change different from a skin-holder that you are proposing? Is it OK to have the manager also as the holder?
    – MPavlak
    Jun 8, 2011 at 14:19
  • Yes, I think your Manager does about the same. Jun 8, 2011 at 14:23
0

SkinnedForm could have a property of type ISkin -

public class SkinnedForm : Form
{
  private ISkin _Skin;
  ...
}

Expose this through a public property, and set it at any point. This way, SkinnedForm is never concerned how ISkin works, or the event model it contains. When you pass in a new Skin class reference, the new OnBlink event will automatically take over. Classes implementing ISkin should contain the logic for OnBlink.

You then have a manager class (not too far from what you've specified) which can get a reference to the new skin, and the concerned SkinnedForm. The only job of the manager is to update the ISkin property on SkinnedForm.

3
  • Wow, I did not know that keeping an Interface local would manage the subscriptions for me. To verify, are you saying that... SkinnedForm(){_Skin = SkinManager.CurrentSkin; _Skin.BlinkEvent += OnBlink; GetDifferentSkin();} GetDifferentSkin(){_Skin = SomeOtherSkin;} After calling GetDifferentSkin, the events are still connected?
    – MPavlak
    Jun 8, 2011 at 14:15
  • Hi, not quite. I'm saying that at any one point, SkinnedForm should have a reference to a single Skin object, which has a single BlinkEvent implementation. If you swap out the whole Skin object, then there is no need to unregister/re-register new events. Especially not in the SkinnedForm object, it shouldnt care about how the Skin events are implemented, it should only know that it has a skin and can call methods / raise events on that skin.
    – christofr
    Jun 8, 2011 at 14:25
  • Phew, thought I was missing a huge part of the language there for a sec. I did some testing and found that indeed, it does not do the auto swapping. I believe your suggest is similar to what Groo has proposed. Thanks :D
    – MPavlak
    Jun 8, 2011 at 14:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.