WPF beyond my understanding



  • K, so, we have some interns, and they're quite good. We have them learning WPF and dealing with XML. I taught them basic LINQ and explained what an ItemsControl is for. Then they found something weird. They have code that works pretty much like this:

    private IEnumerable<InputType> defaults = new List<InputType>
    {
      new TextBoxInput("Blah", "Bleh"),
      new ComboBoxInput("Blah", users),
    }
    
    private IEnumerable<InputType> GetTheThings(XDocument doc)
    {
      var inputs = doc.GetInputElement(); // They do something better than this, I'm paraphrasing
      if(inputs.Any())
        thingOverThereViewModel.Inputs = inputs.Select(x => MakeAThing(x.Name, x.Element("Blah").Value, x.Element("Bleh").Value));
      else
        thingOverThereViewModel.Inputs = defaults;
    }
    
    private InputType MakeAThing(string name, string whatever, string thisToo)
    {
      if(whatever == "Bleh")
        return new ComboBoxInput(name, thisToo);
      return new TextBoxInput(name);
    }
    

    Now, the viewmodel's Inputs is bound to an itemscontrol, and when they cant find any things, the defaults work, and the values update. The actual things constructed get made, but a separate copy gets updated.

    I had them tack on a .ToList() after the .Select() thinking, "This will never work, and makes no sense, but let's try it."

    IT WORKED.

    WHY!?

    I get that LINQ is lazy, and I normally know how to deal with it. But this is bizarre!



  • I don't understand what's the problem.



  • Yeah, when in doubt, ToList() everything.



  • How is GetTheThings used?

    Is there some sort of temporal issue?



  • So, the only way you'd get this behavior that I can think of is:

    When you bind the ItemsSource to the IEnumerable, it iterates over each element, and then iterates again to assign values to the bindings. Whereas forcing it into a list means you just iterate over the same structure, rather than calling a constructor when you iterate.

    But it seems stupid beyond belief that they'd iterate twice.

    @Maciejasjmj said:

    Yeah, when in doubt, ToList() everything.

    Not without being sure it's a good idea. I mean, it's worth trying sure, but it's also massively overused, and causes performance issues if you spam it.



  • @Magus said:

    performance issues

    Well duh. But before you optimize for performance you generally want your shit to work.



  • Which is why I always carefully consider what code to write, rather than guessing '.ToList()'.



  • If it's a correct guess like 90 percent of the time (and make it 99 if you have Select around), I'd say it's a good first guess.


  • ♿ (Parody)

    Sounds like good advice. My Windows troubleshooting rule of thumb is now:

    1. Reboot!
    2. .ToList()

    Good to know.



  • Ok, first of all, the code makes no sense because GetTheThings has no return or yield? Am I crazy?

    Secondly, List implements IList, which implements IEnumerable, so I'm not sure why you thought it wouldn't work. All you're doing is defeating the lazy evaluation by telling LINQ to evaluate it right there right then.



  • My guess is "because he thought it doesn't matter since whatever is bound to Inputs should enumerate the IEnumerable anyway".



  • Because each time you iterate over the select, you're generating a new list. Because the anonymous method is returning new objects each time.

    Items is not a real list, it's an IEnumerator, so every time you do a foreach on it, you get a new list of items.

    You're iterating twice likely because the first iteration is to establish the entities the items control is building, the second iteration is to apply the datacontexts to the child views.



  • But even if I'm wrong in the previous post, it doesn't matter.

    @Magus said:

    Not without being sure it's a good idea.

    No, what you avoid is you don't return new for a select, and if you do, you evaluate it before you store it somewhere.

    If you only want to initialize once, you collapse the select when you want it initialized.

    Don't ever assume how an external party is going to behave with your IEnumerable.



  • @xaade said:

    You're iterating twice likely because the first iteration is to establish the entities the items control is building, the second iteration is to apply the datacontexts to the child views.

    That's exactly the conclusion I came to. And it makes no sense. I would have expected them to build a list or array or something, because having an accidental generator isn't all that hard, and I can't think of when binding to a generator would not be a bug. There should be something telling you what's wrong at the very least.

    @blakeyrat said:

    Ok, first of all, the code makes no sense because GetTheThings has no return or yield? Am I crazy?

    I wrote this all just a bit ago. It's essentially pseudocode to demonstrate approximately what the situation was.

    @blakeyrat said:

    I'm not sure why you thought it wouldn't work.

    At the time, I hadn't come to terms with the fact that it's a generator.



  • @Magus said:

    I would have expected them to build a list or array or something

    I don't see why they'd want to wrap the list you provided, in a list, just for the sake of iteration.

    The view isn't going to directly create or delete view objects, it's going to call the viewmodel to do that, because it has no way to update the original list.

    So the safest thing to do is just use the list it has been given.

    Plus, if you wrap an Observable collection, you lose the notifications, which means you lose the datacontext binding for that list. Unless you're going to wrap it in another list that forwards those events.

    In the end, there's no advantage for WPF to wrap in its own list.

    The UI objects will have their own list, but they are going to reference the data context for updates every single time.

    Once the UI objects exist, they can bind directly to the child object, but until that point they'll have to iterate the original list.

    EDIT:

    I'm pretty sure WPF and databinding came before yield return was used for LINQ.
    So, it's likely they never needed to consider this possibility.



  • @xaade said:

    Plus, if you wrap an Observable collection, you lose the notifications, which means you lose the datacontext binding for that list. Unless you're going to wrap it in another list that forwards those events.

    Now, see, in all of my code, I bind to List or ObservableCollection in almost every case. Even IEnumerable, sometimes. But this is some interns trying to parse XML into UI elements. It isn't that strange to see a generator this close to the xml, and elements aren't going to be added or removed. Really, the whole object should have been a list, and that would have made things simpler, but it isn't even slightly intuitive that this would happen.

    @xaade said:

    I'm pretty sure WPF and databinding came before yield return was used for LINQ.So, it's likely they never needed to consider this possibility.

    Now this makes sense and is probably exactly the reason for this. All I know is, I'd like something to tell me if something like this is happening. Maybe I can write a code analysis reference thingy. Those exist now...



  • @Magus said:

    I bind to List or ObservableCollection in almost every case. Even IEnumerable, sometimes. But this is some interns trying to parse XML into UI elements. It isn't that strange to see a generator this close to the xml, and elements aren't going to be added or removed.

    Solved problem.
    ObservableCollection<T> Constructor (IEnumerable<T>)



  • Why are you linking me the reference for a class you bind to when you need to add/remove elements? That isn't useful at all if the goal is to set a list of elements once. ToList works for that, and it's solved already. But what I want is for something to show a warning if you make a binding to an IEnumerable that acts as a generator. And I have no doubt I can do that, since it's built into 2015.



  • Oh, I missed that the XML document was read-only.



  • It's basically, read some XML, generate a UI to fill in some values, write some XML, which generates TFS work items. It's a good project for interns imo.



  • @Magus said:

    Really, the whole object should have been a list

    Gee, if only there was some function that would turn such an object to list when you need it to be so...

    @Magus said:

    And I have no doubt you can do that, since it's built into 2015

    Then what exactly do you want? A warning that you're already getting? For Microsoft to backport the features from 2015 to whichever version you have? To change the framework because your interns don't realize you don't get a list on Select? To forbid binding to generators because you don't want it, so that obviously means there's no possible use case for that?

    Look, if you want a stable, collapsed collection, there's a method for that.



  • @Maciejasjmj said:

    an object to list when you need it to be so...

    I have that, have said I have that, and am using it. It's that confusing to you?

    If I was getting a warning, I wouldn't be complaining. Instead, you just get a binding that doesn't work. This is in 2015. So I'm writing a warning in Roslyn. Do you have some kind of problem with that?



  • Your guys' dumb anime avatars are too similar. Couldn't one of you be, like, a festering corpse or something?



  • Come on, we're totally facing different directions!



  • @blakeyrat said:

    Couldn't one of you be, like, a festering corpse or something?

    Not planning on dying anytime soon, so sorry, not it.


    Filed under: also I'm totally the smarter guy



  • Anyway, Roslyn is slightly annoying. You don't really have any connections with the normal reflection type system, and have to cast a lot. It'll take some getting used to.



  • @Magus said:

    So I'm writing a warning in Roslyn.

    Well since it still doesn't seem to be a thing that requires a warning to me, it's nice you can write one yourself.

    There's no problem with any of those steps in principle - the problem is when you combine them together in a way they weren't intended to. And now you can even rig the compiler to tell you when you use this feature, because in your case it's always a mistake.

    So what's there to complain about? You have a bug there. Fix it instead of expecting the compiler to do it for you.



  • The point is, it's not obvious that it's a bug. I wouldn't have expected it. Like resharper's null warnings, it might not always indicate a bug, but I'd rather see a green squiggle than nothing, and a green squiggle with a suggested fix and a description is even better.



  • Ok, TIL

    It works here???

    var result =
      from d in data
      group d by new { d.PID, d.Desc } into pg
      select new { Process = pg.Key, Items = pg };
    
    TopLevelListBox.DataContext = result;
    

    Maybe groupby evaluates the query?
    https://msdn.microsoft.com/en-us/library/vstudio/bb534304(v=vs.100).aspx

    This method is implemented by using deferred execution.

    IDK?!?!



  • No, you just aren't using a binding from what I can tell. Which is fine, if you don't want to be able to change the values of the types you new up. It's only an issue if you use a proper binding (such as <ItemsControl ItemsSource="{Binding Result}"/>). In essence, you're testing something completely unrelated, which obviously works exactly as expected.


Log in to reply