0

I have an event set up to fire when an item in a ListView is checked. The event calls a function which updates various controls inside my form. Among other things, I need to enable or disable buttons based on how many items are checked. This function is quite expensive.

Example:

private void listView_ItemCheck(object sender, ItemCheckEventArgs e)
{
    UpdateForm();
}

Now the problem arises when a user wants to check many items at once. This causes the application to be unresponsive for a little while.

So, I would like to call UpdateForm() once after all the items were checked, instead of every time a single item is checked.

EDIT:

Here's a part of UpdateForm():

private void UpdateForm()
{
    // Puts all files in the mod list in a new list
    List<string> modListFiles = new List<string>(lvFiles.Items.Count);
    foreach (ListViewItem lvi in lvFiles.Items)
    {
        modListFiles.Add(lvi.Text);
    }

    // Adds found files to the file list
    foreach (string file in Files)
    {
         lvFiles.Items.Add(new ListViewItem(file));
    }

    // Removes files from mod list that no longer exist
    List<string> deleteQue = new List<string>(lvFiles.Items.Count);
    foreach (string file in modListFiles)
    {
        // If a file in the list doesn't exist anymore, que it to delete
        if (!Files.Contains(file))
        {
            deleteQue.Add(file);
        }
    }

    // Remove queued files
    foreach (string file in deleteQue)
    {
        foreach (ListViewItem lvi in lvFiles.Items)
        {
            if (lvi.Text == file)
            {
                lvFiles.Items.Remove(lvi);
                break;
            }
        }
    }

    // Grays out mod list if a profile is installed
    if (InstalledProfile == null)
    {
        lvFiles.BackColor = SystemColors.Window;
        lvFiles.ForeColor = SystemColors.WindowText;
    }
    else
    {
        lvFiles.BackColor = SystemColors.Control;
        lvFiles.ForeColor = SystemColors.ControlDark;
    }

    // Fills out the game path if it exists
    if (Directory.Exists(GamePath))
    {
        txtGamePath.Text = GamePath;
    }
    else
    {
        txtGamePath.Text = "Game directory does not exist!";
    }

    // Makes sure that the cbxProfiles_SelectedIndexChanged doesn't run UpdateForm() again
    handleProfileChanged = false;

    // Adds profiles to the combobox
    foreach (string profile in Profiles)
        {
        if (!cbxProfiles.Items.Contains(profile))
        {
            cbxProfiles.Items.Add(profile);
        }
    }

    // Removes nonexistant profiles from the combobox
    foreach (string profile in cbxProfiles.Items)
    {
        if (!Profiles.Contains(profile))
        {
            cbxProfiles.Items.Remove(profile);
        }
    }

    if (InstalledProfile == null)
    {
        btnInstallUninstall.Text = "Install";
    }
    else
    {
        btnInstallUninstall.Text = "Uninstall";
    }

    if (Directory.Exists(GamePath) && lvFiles.CheckedItems.Count > 0)
    {
        btnInstallUninstall.Enabled = true;
    }
    else
    {
        btnInstallUninstall.Enabled = false;
    }
}

I've had to simplify some things so please excuse any errors I have probably made.

Some context: I'm trying to make a program that copies files from a set directory \mods to a user specified directory GamePath. It shows all files found in \mods, then allows the user to check some of them. Clicking btnInstall will copy these files to GamePath. After this so called installing, the copied files can be removed by clicking btnInstall again.

All of the properties I made (Profiles, GamePath) get and set their values using an XML file on disk. The main ListView is called lvFiles, sometimes called mod list or file list in comments.

Nimo Beeren
  • 93
  • 2
  • 8
  • 1
    First I would suggest you look into using async/await to prevent your gui from locking up. – John Koerner Dec 18 '14 at 17:47
  • 1
    Maybe use an 'Apply' button instead of the ItemCheck event to call the UpdateForm() method. You might also use "OK" and "Cancel" buttons if you want it to be more like a desktop app. – gmlacrosse Dec 18 '14 at 17:48
  • 1
    It sounds like you need a separate push button for the user to click when they have finished selecting items. – Steve Wellens Dec 18 '14 at 17:48
  • Look at my answer from yesterday on how to properly thread and prevent GUI lock ups: http://stackoverflow.com/questions/27519769/how-to-update-a-label-each-time-in-for-loop/27530479#27530479 – Icemanind Dec 18 '14 at 17:49
  • "This function is quite expensive." Is it only updating the UI, or is it also accessing a database or disk files or something? I find it hard to believe that it can be slowing things down if it is only updating the UI. – RenniePet Dec 18 '14 at 18:18
  • Sounds like properly threading it would be a good idea. Barring that, one **hack** is to use a Timer. Set the Interval to something like one second, then **restart** it each time something gets checked. When the Timer fires, nothing has been checked in the last second (don't forget to turn it off). – Idle_Mind Dec 18 '14 at 18:22
  • To get a good answer, you need to show more code. _Why_ is `UpdateForm()` expensive? It shouldn't be. If it really has to be, then you should run it in a background thread, in a cancellable/restartable way (i.e. so that as the UI selection changes, it can adapt to the changes). Without showing that code though, no one can suggest ways to improve its performance and/or to properly put it in a background/async task. – Peter Duniho Dec 18 '14 at 18:34
  • @Spioner: I would suggest that writing a file every time the user changes a selection may not be the best design. Why do you feel that you need to do it that way, instead of e.g. writing the file in response to some other user action? What is so important about this file that it has to be written immediately? – Peter Duniho Dec 18 '14 at 18:36
  • @RenniePet Actually, it reads data from an XML file and uses this to determine wether or not buttons should be enabled. It also looks in a directory and adds each found file as an item to the ListView. – Nimo Beeren Dec 18 '14 at 18:41
  • @peter The program has to remember what items were checked, so it can check them again on startup. I guess I could write the file on exit of the application, but if the user ends the process or the power goes out some work could be lost. I will add some parts of `UpdateForm()` to explain. – Nimo Beeren Dec 18 '14 at 18:47
  • 1
    @Spioner: I understand the desire to persist user state. But even if you don't want to wait until exit, there are almost certainly more suitable times to do that I/O than every single time the user interacts with the UI. In any case, it doesn't make sense to cripple the normal UI scenarios just to hedge against unlikely scenarios where some lost work is to be expected anyway. – Peter Duniho Dec 18 '14 at 18:50

1 Answers1

0

I've managed to speed the process of checking files up considerably by not calling UpdateForm(). Instead, I made a function UpdateButtons() that only enables/disables buttons.

This way, we do not call UpdateForm() until either the form is activated or the process exits.

Although the current code is far from perfect, all of your help was very useful and is greatly appreciated. I will probably think about the updating mechanism a bit more and apply some good threading later on.

Thank you all!

Here is the code if you want to see it:

    private void UpdateButtons()
    {
        #region btnOpenPath

        if (Directory.Exists(GamePath))
        {
            btnOpenPath.Enabled = true;
        }
        else
        {
            btnOpenPath.Enabled = false;
        }

        #endregion

        #region btnInstallUninstall

        if (InstalledProfile == null)
        {
            btnInstallUninstall.Text = "Install";
        }
        else
        {
            btnInstallUninstall.Text = "Uninstall";
        }

        if (Directory.Exists(GamePath) && lvFiles.CheckedItems.Count > 0)
        {
            btnInstallUninstall.Enabled = true;
        }
        else
        {
            btnInstallUninstall.Enabled = false;
        }

        #endregion

        #region btnDelete, btnCheckAll, btnUncheckAll

        if (InstalledProfile == null)
        {
            btnDelete.Enabled = true;
            btnCheckAll.Enabled = true;
            btnUncheckAll.Enabled = true;
        }
        else
        {
            btnDelete.Enabled = false;
            btnCheckAll.Enabled = false;
            btnUncheckAll.Enabled = false;
        }

        #endregion
    }
Nimo Beeren
  • 93
  • 2
  • 8