Is initializing a StackPanel in a converter and present it using a ContentPresenter is a good MVVM practice?

Question

so this is some kind of follow-up question from my question last time here How to Vertically “center” align the multi line text

At the last time there's this comment from the answerer :

Exactly that, plus that you should not manipulate UI elements in code, but instead make yourself familiar with MVVM

The answerer also have answered nicely using EllipseGeometry and Path. But, since I am not that familiar with Path, I am trying to find another way that more "fit" to my style.

And I found this another way to do it :

Converter Class

public class NoteOctaveConverter : IValueConverter
{
    private const double _dotDiameter = 3;
    private readonly Thickness _dotMargin = new Thickness(0.25);
    private SolidColorBrush _dotFill;
    private readonly HorizontalAlignment _dotHzAlign = HorizontalAlignment.Center;

    public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
    {
        int octave = (int)value;
        List<Ellipse> dots = new List<Ellipse>();
        StackPanel stack = new StackPanel();

        if ((octave > 0 && parameter as string == "TOP") || (octave < 0 && parameter as string == "BOT"))
        {
            _dotFill = Brushes.Black;
        }
        else
        {
            _dotFill = Brushes.Transparent;
        }

        for (int i = 0; i < Math.Abs(octave); i++)
        {
            dots.Add(new Ellipse());
            dots[i].Width = _dotDiameter;
            dots[i].Height = _dotDiameter;
            dots[i].Margin = _dotMargin;
            dots[i].Fill = _dotFill;
            dots[i].HorizontalAlignment = _dotHzAlign;

            stack.Children.Add(dots[i]);
        }

        return stack;
    }

    public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture)
    {
        throw new NotSupportedException();
    }
}

And this is how I present it to the view :

 <ContentPresenter Grid.Column="1" Grid.Row="2"
                      Content="{Binding Path=MusicalNotation.Octave, Converter={StaticResource NoteOctaveConverter}, ConverterParameter=BOT, Mode=OneWay}"
                      HorizontalAlignment="Center" VerticalAlignment="Top"/>

Idk if this what answerer means by "not manipulate UI Elements in code" (I'm new to MVVM), but, is this way* acceptable from the MVVM point of view since I want to try to use MVVM in a good way?

*)This way = Present a StackPanel from a Converter using ContentPresenter.

Additional Notes :

The reason I use the ContentPresenter way instead of Path is that I need to understand my work well, since I may have to present it to my lecturer in a presentation session (I don't want to present something new (risky), IF I have another possible method which is more comfortable to me).

If it turns out that using StackPanel in my way is a bad practice, please tell me what the reason is.

Thank you.

No, it is not good practice, whether you're using MVVM or not. An `IValueConverter` *provides a way to apply custom logic to a binding*, or in plain English, *converts one value to another value*. If your `Converter` class is doing any more than that, then it is invalid. — Sheridan, Jun 20 '14 at 16:06
That is really far from MVVM. See the DataTemplate approach by @EugenePodksal below — Federico Berasategui, Jun 20 '14 at 16:07

score 3 · Accepted Answer · edited May 23 '17 at 10:25

3

I'd recommend you to use the DataTemplate on your content presenter content

<ContentPresenter Content="{Binding X}"/>
  <COntentPresenter.ContentTemplate>
    <DataTemplate >
       <StackPanel>...</StackPanel>
    </DataTemplate>
  </COntentPresenter.ContentTemplate>
</ContentPresenter/>

You will obviously need some dynamic collection handling inside the StackPanel - to accomplish it you could read Bind Collection to StackPanel. Such a question is already answered.

About "Is it a good practice to return controls from the dataConverter":

I doubt. Because converters are not designed to return full-blown controls - it is the task of datatemplates. Well, they obviously can but are usually used for more simple task - some additional formatting, or value to color. When they create controls they make a violation of MVVM and "maximum of UI in XAML" principles. And in this case they do it for no advantage. Even worse - in place of clear XAML and viewmodel logic behind you have difficult to read conversion method.

edited May 23 '17 at 10:25

Community

1
1

answered Jun 20 '14 at 16:03

Eugene Podskal

10,270
5
31
53

So, is it also incorrect to make the `List` as well? (Since `Ellipse` is also an `UIElement`). – Moses Aprico Jun 20 '14 at 16:26
It is better to create some collection of items that define your ellipse. And use another DataTemplate for StackPanel items(see link in the answer) that will transform these items into Ellipses. – Eugene Podskal Jun 20 '14 at 16:29
Examine existing WPF MVVM tutorials like http://msdn.microsoft.com/en-us/magazine/dd419663.aspx. Nothing(UI) is created using the code. It is done through binding and DataTemplating. – Eugene Podskal Jun 20 '14 at 16:32
And do not become obsessed with the pattern. Patterns are just possible solutions, not a dogma. Consider the advantages and disadvantages over your approaches. Weigh everything. – Eugene Podskal Jun 20 '14 at 16:35
1

MVVM isn't ideal http://stackoverflow.com/questions/883895/what-are-the-problems-of-the-mvvm-pattern, but in many cases it will be more concise and clear than any other approach. – Eugene Podskal Jun 20 '14 at 16:43
I've posted an answer which contains the updated (worked well) code. Please check it out and correct me if I done anything which is MVVM-wise incorrect. If it's correct, please let me know as well. Thanks. – Moses Aprico Jun 20 '14 at 17:16

Moses Aprico · Answer 2 · 2014-06-20T17:20:56.400

Just in case anyone want to know the real answer, aside from the good fundamental answer in the accepted answer.

My NEW DotMockup Class (this one is an ellipse defining class to "mockup" the real ellipse)

public class DotMockup
{
    private SolidColorBrush _fill;
    private double _diameter;
    private Thickness _margin;

    public double Diameter
    {
        get { return _diameter; }
        set { _diameter = value; }
    }

    public SolidColorBrush Fill
    {
        get { return _fill; }
        set { _fill = value; }
    }

    public Thickness Margin
    {
        get { return _margin; }
        set { _margin = value; }
    }
}

My UPDATED Converter Class

public class NoteOctaveConverter : IValueConverter
{
    private const double _dotDiameter = 3;
    private readonly Thickness _dotMargin = new Thickness(0.25);
    private SolidColorBrush _dotFill;

    public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
    {
        int octave = (int)value;
        List<DotMockup> dots = new List<DotMockup>();

        if ((octave > 0 && parameter as string == "TOP") || (octave < 0 && parameter as string == "BOT"))
        {
            _dotFill = Brushes.Black;
        }
        else
        {
            _dotFill = Brushes.Transparent;
        }

        for (int i = 0; i < Math.Abs(octave); i++)
        {
            dots.Add(new DotMockup());
            dots[i].Diameter = _dotDiameter;
            dots[i].Margin = _dotMargin;
            dots[i].Fill = _dotFill;
        }

        return dots;
    }

    public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture)
    {
        throw new NotSupportedException();
    }

And finally, my UPDATED XAML part to display the data in a StackPanel

<ItemsControl Grid.Column="1" Grid.Row="2" 
                  ItemsSource="{Binding Path=MusicalNotation.Octave, Converter={StaticResource NoteOctaveConverter}, ConverterParameter=BOT, Mode=OneWay}">
        <ItemsControl.ItemsPanel>
            <ItemsPanelTemplate>
                <StackPanel IsItemsHost="True"/>
            </ItemsPanelTemplate>
        </ItemsControl.ItemsPanel>
        <ItemsControl.ItemTemplate>
            <DataTemplate>
                <Ellipse HorizontalAlignment="Center" VerticalAlignment="Top"
                         Margin="{Binding Margin}" Fill="{Binding Fill}" 
                         Width="{Binding Diameter}" Height="{Binding Diameter}"/>
            </DataTemplate>
        </ItemsControl.ItemTemplate>
    </ItemsControl>

Hope this will help. (The accepted answer will not changed though, since you need to understand it fundamentally first)

Eugene Podskal · Answer 3 · 2014-06-20T17:58:14.560

AFTER THE SOLUTION

It is quite good. But I have few suggestions:

1) Your converter is still too complex as for my taste. Ideally your viewmodel should contain those DotMockups like

public IEnumerable<DotMockups> OctaveData { get {...}}

And your model should contain something like

public Int32 Octave {get {}} // It could be a method if you want

You should transfer creation of DotMockups into the viewmodel layer and calculation of octave to the model layer.

But, actually, if you don't want to make your viewmodel too complex, you could leave it as it is.

2) DotMockups members are too UI-like. If you leave the solution as it is than they are not ideal but passable - they must be visible only from your view layer.

But if your transfer them into the viewmodel they will become UI artifacts - so you will have to deal with them in UI-agnostic way. And here we've come to the moment when direct application of MVVM could bring a lot of problems. If your objects could vary greatly in their representation than adherence to the MVVM will lead to a lot of code duplication and invention of your own Brushes, Shapes and so on.

But if we will look at

private const double _dotDiameter = 3;
private readonly Thickness _dotMargin = new Thickness(0.25);

and

if ((octave > 0 && parameter as string == "TOP") || (octave < 0 && parameter as string == "BOT"))
        {
            _dotFill = Brushes.Black;
        }
        else
        {
            _dotFill = Brushes.Transparent;
        }

you have only two types of dots: Black and Transparent. So you could write them as:

public class DotViewModel { public Boolean IsFilled // I think that there are could be some more appropriate name in music. { get {...}; } }

P.S.: I won't continue further. First - this post isn't the answer to the question. Second - Sorry, but StackOverflow isn't some code review or write-code-for-me resource. Third - I have some things to do for myself. So I will graciously leave you to the intricacies of the MVVM and WPF. I hope that I have shown some ideas. Just work on them, read and improve. Good luck.

Just some questions to continue - how many types of DotMockups there are? At least how many possible colors? — Eugene Podskal, Jun 20 '14 at 17:43
3 Types. Black & Thickness & D3, Transparent & Thickness & D3, and Black & Thickness & D2 — Moses Aprico, Jun 20 '14 at 18:21
Actually there's only 1 dot color, which is black. the transparent one is to make the note always at the center of the viewer (described more in the link given in OP). Alright, that've been more than helping. I think that my MVVM implementation is already correct in some way (no fundamental error). Many thanks! — Moses Aprico, Jun 20 '14 at 18:28

Is initializing a StackPanel in a converter and present it using a ContentPresenter is a good MVVM practice?

3 Answers3