views:

143

answers:

6

i am using .net win forms my mdi parent form contains the menu. If clicked the menu the form will be displayed. Up to now no problem. >

        UserForm uf = new UserForm();

        uf.Show();

        uf.MdiParent = this;

If i click the menu again another duplicate of the form created how to solve this issue?

A: 

You could always make the Form a Singleton:

public class MyForm : Form
{
    private MyForm _instance = null;
    private object _lock = new object();

    private MyForm() { }


    public static MyForm Instance
    {
        get
        {
            if (_instance == null)
            {
                lock (_lock)
                {
                    if (_instance == null)
                    _instance = new MyForm();
                }
            }
            return _instance;
        }
    }
}

Then your call would look something like:

MyForm.Instance.Show();
MyForm.Instance.MdiParent = this;
Justin Niessner
@Justin - fixed your code. You weren't returning anything and you hadn't added a get accessor.
GenericTypeTea
@GenericTypeTea - Heh. Got stuck in a fire drill and didn't get to edit the code. Thanks!
Justin Niessner
+3  A: 

You should create a singleton class for managing your form instances:

public class FormProvider
{
   public static UserForm UserForm
   {
       get
       {
          if (_userForm== null || _userForm.IsDisposed)
          {
            _userForm= new UserForm ();
          }
          return _userForm;
       }
   }
   private static UserForm _userForm;
}

NB, this is a very simple Singleton pattern. For the correct way to use the pattern, use this link.

You can then just access the form as follows:

FormProvider.UserForm.Show();
FormProvider.UserForm.MdiParent = this;

When FormProvider.UserForm is accessed for the FIRST time, it will be created. Any subsequent get on the FormProvider.UserForm property will return the form that was created on first access. This means that the form will only ever be created once.

GenericTypeTea
Adding a `lock` around the form instance is *not* adding any safety, only the illusion of it. A form is only permitted to be accessed from the thread that created it, and a thread *must* be an STA thread to create a form. Adding syncronization is arguably *more* dangerous than leaving it out, since it gives the impression to the user that you're actually doing the necessary work to make this multithread-compatible. In addition, this is *not* a scenario that calls for a Singleton. Finally, this will result in an error if the form is ever closed, as you'll access a disposed object.
Adam Robinson
Be extremely aware of memory leak when using a static form. Check out http://msdn.microsoft.com/en-us/library/ee658248.aspx to learn about handling events for a form.
Pierre-Alain Vigeant
@Adam - this will not cause an error if the form is closed. I've got 2/3 compact framework apps out there and there's never been an issue. Agree with you about the thread safety though, so I removed the snippet.
GenericTypeTea
@Generic: I'm not sure what the compact framework has to do with this particular scenario, but calling `Show` on a form (in winforms, as the question relates to) that has been closed will result in an `ObjectDisposedException`.
Adam Robinson
@Adam - Updated my snippet to take the ObjectDisposedException into consideration, thank you.
GenericTypeTea
@Generic: Forms (both in CF and Winforms) do not automatically dispose when closed when they're part of an MDI form (or when they're shown using `ShowDialog`). That's why it works for you, *and would work for this question*, but you'd be breaking SOC by assuming that the form is being shown as part of an MDI form collection in this class without any tangible reason to have confidence in that fact from the class's perspective.
Adam Robinson
A: 

Options:

Typically, disabling the button works fine, and makes more sense from the user's perspective. Singleton works if you need the button enabled for something else.

Singleton is probably not a good solution if the form could be closed and a new instance will later be required.

Jay
@Jay: I would leave the button enabled, as that (in most applications I've seen) will either show the form or bring it to the front if it's already open.
Adam Robinson
+2  A: 

In contrast to the existing answers here, I would not recommend using a Singleton for this. The Singleton pattern is woefully overused, and is generally a "code smell" that indicates that something's gone wrong with your overall design. Singletons are generally put in the same "bucket" as global variables: you'd better have a really strong case for using it.

The simplest solution is to make an instance variable on your main form that represents the form in question, then use that to show it.

public class MainMdiForm : Form
{
    ...

    UserForm userForm;

    ...

    private void ShowUserForm()
    {
        if(userForm == null || userForm.IsDisposed)
        {
            userForm = new UserForm();
            userForm.MdiParent = this;
        }

        userForm.Show();
        userForm.BringToFront();
    }
}
Adam Robinson
+1 for not using Singleton
Mark Heath
@Adam - the singleton pattern has it's incorrect uses. I, however, believe this is one of the particular instances where a singleton is used correctly. What you'e doing is virtually the same as what's happening within the singleton anyway, only you'll end up with messier code.
GenericTypeTea
@Generic: Only creating a single instance is *not* a synonym for an object being a singleton, nor is it an indication that a Singleton pattern is appropriate.
Adam Robinson
@Adam - OK, so take the word singleton out of the equation. My example is still much easier to manage and is the same as what you've provided, only my form variable is static and yours is contained within another form. Both our examples are perfectly valid, I just happen to think mine's cleaner.
GenericTypeTea
@Generic: While I wouldn't agree that yours is easier to manage (you now have another type that you have to manage whose logic--which is specific to a single other type--is in another location), it's certainly valid in the sense that it's functional and will solve the problem. I just argue that it's not the "right" way, for whatever that's worth.
Adam Robinson
@Adam - I want to understand your reasoning against it (I'm on SO to learn like a lot of others, I only provide answers when I think I know the correct answer). If I shouldn't be recommending this method, in factual terms, why not?
GenericTypeTea
@Generic: I've found these to be pretty compelling arguments against their proliferation: http://blogs.msdn.com/b/scottdensmore/archive/2004/05/25/140827.aspx http://scottdensmore.typepad.com/blog/2009/08/singletons-are-evil-part-2.html
Adam Robinson
@Adam - thanks very much. They are a good read. The author (on the 2nd link) does say himself that my example is 'Great!'... 'other than this is quite contrived and nothing in the real world is this easy'. Well, I disagree :P, my real world winforms apps are that easy, heh. But I do understand the points made. The example is only bad if the use is more complex then the OP is letting on. I'm starting to lean more toward your example now to be honest for anything but simple apps.
GenericTypeTea
@Generic: Yeah, that's kinda what I'm getting at. In most cases, the pattern is just a "bandaid" for a bad design. In most *other* cases (like here), it's generally just a contrived solution; in a simple application where this is being done in one spot, you don't *gain* anything by moving the logic to another class (you aren't making the code much, if any, more meaningful, and you aren't reusing the code anywhere else).
Adam Robinson
@Adam Robinson: I fixed a typo in your code (it had `pivate` instead of `private`)
R. Bemrose
@Adam - Thanks for opening my eyes. Time to update the coding standards!
GenericTypeTea
A: 

You could just examine the MdiChildren property of your host form to determine if an instance of your UserForm exists in it.

UserForm myForm = null;
foreach (Form existingForm in this.MdiChildren)
{
    myForm = existingForm as UserForm;
    if (myForm != null)
        break;
}

if (myForm == null)
{
    myForm = new UserForm();
    myForm.MdiParent = this;

    myForm.Show();
}
else
    myForm.Activate();

This will create a new instance of your UserForm is it doesn't already exist, and it will switch to the created instance if it does exist.

Nathen Silver
+3  A: 

The cleanest way is to simply track the lifetime of the form instance. Do so by subscribing the FormClosed event. For example:

    private UserForm userFormInstance;

    private void showUserForm_Click(object sender, EventArgs e) {
        if (userFormInstance != null) {
            userFormInstance.WindowState = FormWindowState.Normal;
            userFormInstance.Focus();
        }
        else {
            userFormInstance = new UserForm();
            userFormInstance.MdiParent = this;
            userFormInstance.FormClosed += (o, ea) => userFormInstance = null;
            userFormInstance.Show();
        }
    }
Hans Passant