wimh/SparkleShare — comment on commit 9d6cc36ad18b0938245e96c85cd0b976ada7cad5
File:  line 0
By wimh:

(is this a proper place to discuss this? - I can copy my text to issue 358)

The more I think about it, I think the best solution is to use locking. That will make sure nothing will go wrong. And the performance penalty would not be very big.

public List Repositories;

becomes

private readonly List repositories = new List ();

public List Repositories
{
get {
List result = new List ();
lock (repositories) {
result.AddRange(repositories);
}

return result;
}
}

this will make SparkleControllerBase responsible for proper locking of `repositories`, but if another class needs the list, it gets a copy.

All functions which edit the list, need a lock:

lock (repositories) {
repositories.Add (repo);
}

But also read access like a foreach need a lock, or it must use the property which creates a copy, that can be used without a lock.

So either this:

public void AddNoteToFolder (string folder_name, string revision, string note)
{
folder_name = folder_name.Replace ("%20", " ");
note = note.Replace ("%20", " ");

foreach (SparkleRepoBase repo in Repositories) {
if (repo.Name.Equals (folder_name))
repo.AddNote (revision, note);
}
}

or this:

public void AddNoteToFolder (string folder_name, string revision, string note)
{
folder_name = folder_name.Replace ("%20", " ");
note = note.Replace ("%20", " ");

lock (repositories) {
foreach (SparkleRepoBase repo in repositories) {
if (repo.Name.Equals (folder_name))
repo.AddNote (revision, note);
}
}
}

Because `RemoveRepository` modifies the list, it requires a lock around the whole loop:

// Removes a repository from the list of repositories and
// updates the statusicon menu
private void RemoveRepository (string folder_path)
{
string folder_name = Path.GetFileName (folder_path);

lock (repositories) {
for (int i = 0; i < repositories.Count; i++) {
SparkleRepoBase repo = repositories [i];

if (repo.Name.Equals (folder_name)) {
repo.Dispose ();
repositories.Remove (repo);
repo = null;
break;
}
}
}
}

Or it must create a copy, operate on the copy and replace it. (That requires two locks).