10 Replies - 454 Views - Last Post: 02 February 2020 - 04:53 AM Rate Topic: -----

#1 MarkPC   User is offline

  • New D.I.C Head

Reputation: 1
  • View blog
  • Posts: 11
  • Joined: 04-May 16

Action creates new instance of class but it shouldn't

Posted 29 January 2020 - 05:46 AM

Hi all,

I have a weird problem that I can't seem to figure out. I am working on an app in WPF that helps you keep your programs up to date. On a page of the app there's a ListView with information about the app and a link label to run the app. What happens when you click that link is specified with a LinkClickCommand (because the link should do different things on different pages). Here is my code:

notInstalledPortableApps = GetNotInstalledPortableApps(); // Returns a List<ProtableApp>    

foreach (PortableApp app in notInstalledPortableApps)
{
	app.DisplayedVersion = app.LatestVersion;

	app.LinkClickCommand = new LinkClickCommand(new Action(async () =>
	{
		await app.Download();
		await app.Install(true);
	}));
}

portableAppsListView.ItemsSource = notInstalledPortableApps;


public class LinkClickCommand : ICommand
{
	public event EventHandler CanExecuteChanged;
	private Action execute;

	public LinkClickCommand(Action execute)
	{
		this.execute = execute;
	}

	public bool CanExecute(object parameter)
	{
		return true;
	}

	public void Execute(object parameter)
	{
		execute?.Invoke();
	}
}


Everything but one thing works like expected. When the link is clicked the app is downloaded and installed like expected, but it doesn't report on the progress of the download and installation. That logic is in the app.Download() and app.Install() functions and works fine in other parts of the app, where there's no link used to call the method. It just changes two properties of the PortableApp instance (app.Status and app.Progress) and the ListView should update accordingly.

What I suspect is that somehow new instance of the PortableApp is created when the Action is called. The Action should invokes the download and install functions of the same instance that is in the list (notInstalledPortableApps, the ItemsSource of the ListView). I just don't know how to make this work. I feel like it's something small, but I can't figure it out.

Is This A Good Question/Topic? 0
  • +

Replies To: Action creates new instance of class but it shouldn't

#2 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 7244
  • View blog
  • Posts: 24,556
  • Joined: 05-May 12

Re: Action creates new instance of class but it shouldn't

Posted 29 January 2020 - 01:08 PM

Does PortableApp implement INotifyPropertyChanged correctly?
Was This Post Helpful? 0
  • +
  • -

#3 MarkPC   User is offline

  • New D.I.C Head

Reputation: 1
  • View blog
  • Posts: 11
  • Joined: 04-May 16

Re: Action creates new instance of class but it shouldn't

Posted 30 January 2020 - 05:15 AM

Yes, as I said, it works in other parts of the app, but just not in combination with the Action/LinkClickCommand.

This is the related code form the Portable Apps. I haven't implemented INotifyPropertyChanged, but this seems to work the same.
private int progress;
public int Progress
{
	get { return progress; }
	set
	{
		progress = value;
		onpropertychanged("Progress");
	}
}

private string status;
public string Status
{
	get { return status; }
	set
	{
		status = value;
		onpropertychanged("Status");
	}
}

protected void onpropertychanged(string name)
{
	PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(name));
}

Was This Post Helpful? 0
  • +
  • -

#4 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 7244
  • View blog
  • Posts: 24,556
  • Joined: 05-May 12

Re: Action creates new instance of class but it shouldn't

Posted 30 January 2020 - 07:53 AM

Doh! I forgot all about lambda captures in relation to loops. Try making an explicit copy. (See Eric Lippert's Closing over the loop variable considered harmful, part one) Try:
foreach (PortableApp iterApp in notInstalledPortableApps)
{
    PortableApp app = iterApp;
    app.DisplayedVersion = app.LatestVersion;

    app.LinkClickCommand = new LinkClickCommand(new Action(async () =>
    {
        await app.Download();
        await app.Install(true);
    }));
}



As noted in the update at the top of Eric's blog, C# 5.0 made a breaking change. The compiler is supposed to make a copy automatically for you so that you don't need to make a copy. Are you perhaps using an older version of C#?

Anyway, Action() shouldn't be making a new instance. It's the C# 5.0 compiler that just makes a copy of the reference. Is your PowerApp a struct, rather then a class, and so you are running into this issue where the copy of a struct is effectively a new instance?
Was This Post Helpful? 0
  • +
  • -

#5 MarkPC   User is offline

  • New D.I.C Head

Reputation: 1
  • View blog
  • Posts: 11
  • Joined: 04-May 16

Re: Action creates new instance of class but it shouldn't

Posted 31 January 2020 - 06:29 AM

Thanks, that is helpful. But unfortunately making a explicit copy did not work, the progress and status of the ListViewItem still don't change after clicking the link. I still suspect it doesn't change the same object/instance that is in the notInstalledPortableApps List. PortableApp is just a regular class, not a struct.

I am using the latest minor version of C# (7.2) with Visual Studio 2019.
Was This Post Helpful? 0
  • +
  • -

#6 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 7244
  • View blog
  • Posts: 24,556
  • Joined: 05-May 12

Re: Action creates new instance of class but it shouldn't

Posted 01 February 2020 - 12:10 PM

At this point, it's time to try either prove or disprove your theory about a new instance being created (or not).

A brute force way to try to prove or disprove it would be to assign a unique ID to each instance of your PowerApp object. Something like this:

class PowerApp
{
    static int _IdCounter = 0;

    public int ObjectId { get; private set; }

    public PowerApp( ... )
    {
        ObjectId = Interlocked.Increment(ref _IdCounter);
        :
    }
}



and then in the place were you set things up, you build up a list of those Id's that you setup lambda's for. And have an assert to make sure that the enumerator is giving you the same object, as as the lambda that is running is actually within that registered list. Something like:

List<int> _registeredIds = new List<int>();

:

_registeredIds.Clear();
var notInstalledPortableApps = List<PortableApp>();
foreach(var app in GetNotInstalledPortableApps())
{
    _registeredIds.Add(app.Id);
    notInstalledPortableApps.Add(app);
}

foreach (PortableApp app in notInstalledPortableApps)
{
    Debug.Assert(_registeredIds.Contains(app.Id));

    app.DisplayedVersion = app.LatestVersion;

    app.LinkClickCommand = new LinkClickCommand(new Action(async () =>
    {
        Debug.Assert(_registeredIds.Contains(app.Id));

        await app.Download();
        await app.Install(true);
    }));
}

portableAppsListView.ItemsSource = notInstalledPortableApps;


Was This Post Helpful? 0
  • +
  • -

#7 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 7244
  • View blog
  • Posts: 24,556
  • Joined: 05-May 12

Re: Action creates new instance of class but it shouldn't

Posted 01 February 2020 - 12:44 PM

As an aside, my quick and dirty test seems to show that I'm not getting new instances...
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

class PortableApp
{
    static int _counter = 0;
    public int Id { get; }

    public PortableApp() => Id = Interlocked.Increment(ref _counter);

    public async Task ShowIdAsync() => await Task.Run(() => Console.WriteLine(Id));
}

class Program
{
    public static async Task Main()
    {
        var apps = Enumerable.Range(1, 20)
                             .Select(_ => new PortableApp())
                             .ToList();

        Console.WriteLine("Registering...");
        var actions = new List<Action>();
        foreach (var app in apps)
        {
            await app.ShowIdAsync();

            actions.Add(new Action(async () => await app.ShowIdAsync()));
        }

        Console.WriteLine("Invoking actions...");
        foreach (var action in actions)
            action();

        await Task.Delay(5000);
    }
}



Outputs:
Registering...
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
Invoking actions...
1
3
2
4
7
6
9
10
11
5
8
14
12
20
19
18
13
17
16
15


Was This Post Helpful? 0
  • +
  • -

#8 MarkPC   User is offline

  • New D.I.C Head

Reputation: 1
  • View blog
  • Posts: 11
  • Joined: 04-May 16

Re: Action creates new instance of class but it shouldn't

Posted 01 February 2020 - 03:59 PM

Testing with unique IDs is a good one. I tested it myself with a simple counter on the constructor of PortableApp (like yours) and a message box when I click the link.
I get the same IDs as well, so that is not the problem. But why doesn't displaying the status and progress on the ListViewItem work then?
Was This Post Helpful? 0
  • +
  • -

#9 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 7244
  • View blog
  • Posts: 24,556
  • Joined: 05-May 12

Re: Action creates new instance of class but it shouldn't

Posted 01 February 2020 - 04:54 PM

Set breakpoints on the Download() and Install() methods. Are they called? Do they run to completion without throwing exceptions?

Set breakpoints on the get and set for those properties. Are they called while Download() and Setup() are executing?
Was This Post Helpful? 0
  • +
  • -

#10 Skydiver   User is offline

  • Code herder
  • member icon

Reputation: 7244
  • View blog
  • Posts: 24,556
  • Joined: 05-May 12

Re: Action creates new instance of class but it shouldn't

Posted 01 February 2020 - 05:02 PM

Also, try changing this:
portableAppsListView.ItemsSource = notInstalledPortableApps;


to:
portableAppsListView.ItemsSource = new ObservableCollection<PortableApp>(notInstalledPortableApps);


Was This Post Helpful? 0
  • +
  • -

#11 MarkPC   User is offline

  • New D.I.C Head

Reputation: 1
  • View blog
  • Posts: 11
  • Joined: 04-May 16

Re: Action creates new instance of class but it shouldn't

Posted 02 February 2020 - 04:53 AM

The download and the install methods get called and run without exceptions. The properties also get called and changed like expected. Changing the ItemsSource to an ObservableCollection did not change anything.

Turns out that I actually had the event and method for the INotifyPropertyChanged interface, but I just forgot to actually implement the interface itself. By implementing it on the PortableApp class and not changing anything else, it works like excepted. Should have checked that sooner... Thanks for the help, I learnt some useful debugging tips from it!
Was This Post Helpful? 1
  • +
  • -

Page 1 of 1