Markdown Markup Editor: MK2

Question

Following on from this question I have added some more functionality to Markdown Markup, and made it more WPF idiomatic.

It now supports saving data from any of the four boxes, and loading Markdown or CSS files.

Everything seems to work, so as always, any tips/pointers/critique is/are welcome.

So, first, the new MainWindow.xaml.cs:

/// <summary>
/// Interaction logic for MainWindow.xaml
/// </summary>
public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        DataContext = new MainWindowViewModel();
    }

    private MainWindowViewModel ViewModel => DataContext as MainWindowViewModel; 

    private void renderPreviewBrowser_Navigating(object sender, NavigatingCancelEventArgs e)
    {
        // This prevents links in the page from navigating, this also means we cannot call WebBrowser.Navigate for any browsers with this event.
        if (e.Uri != null)
        {
            e.Cancel = true;
        }
    }
}

Nice and succinct.

The new MainWindow.xaml:

<Window x:Class="Markdown_Markup.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:Markdown_Markup"
        mc:Ignorable="d"
        Title="MainWindow" Height="539" Width="749"
        d:DataContext="{d:DesignInstance local:MainWindowViewModel}">
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
            <RowDefinition Height="*"/>
        </Grid.RowDefinitions>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
            <ColumnDefinition Width="*"/>
        </Grid.ColumnDefinitions>
        <DockPanel Grid.ColumnSpan="3">
            <Menu DockPanel.Dock="Top">
                <MenuItem Header="_File">
                    <MenuItem Header="_Open">
                        <MenuItem Header="_Markdown" Command="{Binding OpenMarkdownCommand}"/>
                        <MenuItem Header="_CSS" Command="{Binding OpenCssCommand}"/>
                    </MenuItem>
                    <MenuItem Header="_Save">
                        <MenuItem Header="_Markdown" Command="{Binding SaveMarkdownCommand}" />
                        <MenuItem Header="_CSS" Command="{Binding SaveCssCommand}"/>
                        <MenuItem Header="_Generated HTML" Command="{Binding SaveGeneratedHtmlCommand}"/>
                        <MenuItem Header="_Rendered HTML" Command="{Binding SaveRenderedHtmlCommand}"/>
                    </MenuItem>
                </MenuItem>
            </Menu>
            <StackPanel></StackPanel>
        </DockPanel>
        <StatusBar Height="24" VerticalAlignment="Bottom" Grid.ColumnSpan="3" Grid.Row="2"/>
        <TextBox Margin="5,45,5,29" TextWrapping="Wrap" Grid.RowSpan="3" Text="{Binding MarkdownContent, UpdateSourceTrigger=PropertyChanged}"  AcceptsReturn="True" AcceptsTab="True"/>
        <TextBox Margin="5,28,5,5" TextWrapping="Wrap" Grid.Column="1" Grid.Row="1" IsReadOnly="True" Text="{Binding HtmlContent}"/>
        <TextBox Margin="5,26,5,29" TextWrapping="Wrap" Grid.Column="1" Grid.Row="2" IsReadOnly="True" Text="{Binding HtmlRenderContent}"/>
        <TextBox Margin="5,45,5,0" TextWrapping="Wrap" Grid.Column="1" Text="{Binding CssContent, UpdateSourceTrigger=PropertyChanged}" AcceptsReturn="True" AcceptsTab="True"/>
        <WebBrowser local:BrowserBehavior.Html="{Binding HtmlRenderContent}" Grid.Column="2" Margin="5,45,5,29" Grid.RowSpan="3" Navigating="renderPreviewBrowser_Navigating" />
        <Label Content="Markdown Content:" HorizontalAlignment="Left" Margin="5,19,0,0" VerticalAlignment="Top"/>
        <Label Content="Additional CSS:" Grid.Column="1" HorizontalAlignment="Left" Margin="5,19,0,0" VerticalAlignment="Top"/>
        <Label Content="Markdown HTML:" Grid.Column="1" HorizontalAlignment="Left" Margin="5,2,0,0" Grid.Row="1" VerticalAlignment="Top"/>
        <Label Content="Render HTML:" Grid.Column="1" HorizontalAlignment="Left" Margin="5,0,0,0" Grid.Row="2" VerticalAlignment="Top"/>
        <Label Content="HTML Preview:" Grid.Column="2" HorizontalAlignment="Left" Margin="5,19,0,0" VerticalAlignment="Top"/>
    </Grid>
</Window>

This is a bit larger than last time.

The BrowserBehavior.cs:

/// <summary>
/// Represents a behavior to control WebBrowser binding to an HTML string.
/// </summary>
/// <remarks>
/// Adopted from: http://stackoverflow.com/a/4204350/4564272
/// </remarks>
public class BrowserBehavior
{
    public static readonly DependencyProperty HtmlProperty = DependencyProperty.RegisterAttached(
            "Html",
            typeof(string),
            typeof(BrowserBehavior),
            new FrameworkPropertyMetadata(OnHtmlChanged));

    [AttachedPropertyBrowsableForType(typeof(WebBrowser))]
    public static string GetHtml(WebBrowser d) => (string)d.GetValue(HtmlProperty);

    public static void SetHtml(WebBrowser d, string value)
    {
        d.SetValue(HtmlProperty, value);
    }

    static void OnHtmlChanged(DependencyObject dependencyObject, DependencyPropertyChangedEventArgs e)
    {
        var webBrowser = dependencyObject as WebBrowser;
        webBrowser?.NavigateToString(e.NewValue as string ?? "&nbsp;");
    }
}

It's slightly modified from the Stack Overflow question mentioned in the XML comment.

A DelegateCommand (given to me by Mat's Mug, which I made a couple changes to):

public class DelegateCommand : ICommand
{
    private readonly Predicate<object> _canExecute;
    private readonly Action<object> _execute;

    public DelegateCommand(Action<object> execute, Predicate<object> canExecute = null)
    {
        _canExecute = canExecute;
        _execute = execute;
    }

    public bool CanExecute(object parameter) => _canExecute == null || _canExecute.Invoke(parameter);

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

    public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    }      
}

Lastly, and this is the more fun of the files, the MainWindowViewModel.cs:

public class MainWindowViewModel : INotifyPropertyChanged
{
    private Markdown _markdown;

    private string _markdownContent;
    private string _cssContent;
    private string _htmlContent;
    private string _htmlRenderContent;

    public MainWindowViewModel()
    {
        _markdown = new Markdown();
        SaveMarkdownCommand = new DelegateCommand(SaveMarkdown, CanSaveMarkdown);
        SaveCssCommand = new DelegateCommand(SaveCss, CanSaveCss);
        SaveGeneratedHtmlCommand = new DelegateCommand(SaveGeneratedHtml, CanSaveGeneratedHtml);
        SaveRenderedHtmlCommand = new DelegateCommand(SaveRenderedHtml, CanSaveRenderedHtml);
        OpenMarkdownCommand = new DelegateCommand(OpenMarkdown, CanOpenMarkdown);
        OpenCssCommand = new DelegateCommand(OpenCss, CanOpenCss);
    }

    public ICommand SaveMarkdownCommand { get; }
    public ICommand SaveCssCommand { get; }
    public ICommand SaveGeneratedHtmlCommand { get; }
    public ICommand SaveRenderedHtmlCommand { get; }
    public ICommand OpenMarkdownCommand { get; }
    public ICommand OpenCssCommand { get; }

    public string MarkdownContent
    {
        get { return _markdownContent; }
        set
        {
            _markdownContent = value;
            OnPropertyChanged(new PropertyChangedEventArgs(nameof(MarkdownContent)));
            UpdateHtml();
        }
    }

    public string CssContent
    {
        get { return _cssContent; }
        set
        {
            _cssContent = value;
            OnPropertyChanged(new PropertyChangedEventArgs(nameof(CssContent)));
            UpdateHtml();
        }
    }

    public void UpdateHtml()
    {
        var html = _markdown.Transform(MarkdownContent);

        HtmlContent = html;

        html = $"<html>\r\n\t<head>\r\n\t\t<style>\r\n\t\t\t{CssContent}\r\n\t\t</style>\r\n\t</head>\r\n\t<body>\r\n\t\t{html}\r\n\t</body>\r\n</html>";

        HtmlRenderContent = html;
    }

    public string HtmlContent
    {
        get { return _htmlContent; }
        set
        {
            _htmlContent = value;
            OnPropertyChanged(new PropertyChangedEventArgs(nameof(HtmlContent)));
        }
    }

    public string HtmlRenderContent
    {
        get { return _htmlRenderContent; }
        set
        {
            _htmlRenderContent = value;
            OnPropertyChanged(new PropertyChangedEventArgs(nameof(HtmlRenderContent)));
        }
    }

    public bool CanSaveMarkdown(object parameter) => !string.IsNullOrWhiteSpace(MarkdownContent);

    public void SaveMarkdown(object parameter)
    {
        var dialog = new SaveFileDialog();
        dialog.AddExtension = true;
        dialog.Filter = "Markdown Files|*.md|All Files|*.*";
        var result = dialog.ShowDialog();

        if (result.Value)
        {
            using (var sw = new StreamWriter(dialog.FileName))
            {
                sw.WriteLine(MarkdownContent);
            }
        }
    }

    public bool CanSaveCss(object parameter) => !string.IsNullOrWhiteSpace(CssContent);

    public void SaveCss(object parameter)
    {
        var dialog = new SaveFileDialog();
        dialog.AddExtension = true;
        dialog.Filter = "CSS Files|*.css|All Files|*.*";
        var result = dialog.ShowDialog();

        if (result.Value)
        {
            using (var sw = new StreamWriter(dialog.FileName))
            {
                sw.WriteLine(CssContent);
            }
        }
    }

    public bool CanSaveGeneratedHtml(object parameter) => !string.IsNullOrWhiteSpace(HtmlContent);

    public void SaveGeneratedHtml(object parameter)
    {
        var dialog = new SaveFileDialog();
        dialog.AddExtension = true;
        dialog.Filter = "HTML Files|*.html|All Files|*.*";
        var result = dialog.ShowDialog();

        if (result.Value)
        {
            using (var sw = new StreamWriter(dialog.FileName))
            {
                sw.WriteLine(HtmlContent);
            }
        }
    }

    public bool CanSaveRenderedHtml(object parameter) => !string.IsNullOrWhiteSpace(HtmlRenderContent);

    public void SaveRenderedHtml(object parameter)
    {
        var dialog = new SaveFileDialog();
        dialog.AddExtension = true;
        dialog.Filter = "HTML Files|*.html|All Files|*.*";
        var result = dialog.ShowDialog();

        if (result.Value)
        {
            using (var sw = new StreamWriter(dialog.FileName))
            {
                sw.WriteLine(HtmlRenderContent);
            }
        }
    }

    public bool CanOpenMarkdown(object parameter) => true;

    public void OpenMarkdown(object parameter)
    {
        var dialog = new OpenFileDialog();
        dialog.AddExtension = true;
        dialog.Filter = "Markdown Files|*.md|All Files|*.*";
        var result = dialog.ShowDialog();

        if (result.Value)
        {
            using (var sr = new StreamReader(dialog.FileName))
            {
                MarkdownContent = sr.ReadToEnd();
            }
        }
    }

    public bool CanOpenCss(object parameter) => true;

    public void OpenCss(object parameter)
    {
        var dialog = new OpenFileDialog();
        dialog.AddExtension = true;
        dialog.Filter = "CSS Files|*.css|All Files|*.*";
        var result = dialog.ShowDialog();

        if (result.Value)
        {
            using (var sr = new StreamReader(dialog.FileName))
            {
                CssContent = sr.ReadToEnd();
            }
        }
    }

    public void OnPropertyChanged(PropertyChangedEventArgs e)
    {
        var handler = PropertyChanged;
        handler?.Invoke(this, e);
    }

    public event PropertyChangedEventHandler PropertyChanged;
}

All comments and critique are welcome.

It's also on GitHub now.

Answer 1

OnPropertyChanged()

This method to raise the PropertyChanged event needs only to be called if the value is changed which isn't verified by the setters of your properties yet. To fix this issue and prevent unneeded work to be done a simple if condition is needed like so

public string MarkdownContent
{
    get { return _markdownContent; }
    set
    {
        if (_markdownContent == value) { return; }

        _markdownContent = value;
        OnPropertyChanged(new PropertyChangedEventArgs(nameof(MarkdownContent)));
        UpdateHtml();
    }
}

The implementation of the OnPropertyChanged() can be improved by just using the ? null-conditional operator which is clearly stated in the New Features in c# 6

We expect that a very common use of this pattern will be for triggering of events:

 PropertyChanged?.Invoke(this, args);

This is an easy and thread-safe way to check for null before you trigger an event. The reason it’s thread-safe is that the feature evaluates the left-hand side only once, and keeps it in a temporary variable.

OpenMarkDown() and OpenCss()

You have duplicated code here and an unused method parameter. By introducing a string GetLoadFilename(string filter) (not sure about the method name) this can be prevented like so

private string GetLoadFilename(string filter)
{
    var dialog = new OpenFileDialog();
    dialog.AddExtension = true;
    dialog.Filter = filter;
    var result = dialog.ShowDialog();
    
    return result.Value ? dialog.FileName : string.Empty;
}

and now for instance OpenCss() would look like so if we also extract the actual reading of the file to a string ReadFile(string) method

public void OpenCss(object parameter)
{
    var fileName = GetLoadFilename("CSS Files|*.css|All Files|*.*");
    if (fileName.Length == 0) { return; }

    CssContent = ReadFile(fileName)

}

private string ReadFile(string fileName)
{
    using (var sr = new StreamReader(dialog.FileName))
    {
        return sr.ReadToEnd();
    }
}  

Almost the same refactoring should be applied to the SaveMarkdown(), SaveCss(), SaveGeneratedHtml() and SaveRenderedHtml() by introducing the methods string GetSaveFilename(string) and void SaveFile(string).

Answer 2

Some feedback on the XAML:

• Good that you set d:DataContext as it saves some pain when bindings are ~compiletime checked (if the designer has the view open)
• <RowDefinition Height="*"/> *is the default but I don't mind being explicit about it.
• <DockPanel Grid.ColumnSpan="3"> usually you want to avoid ColumnSpan. Nesting panels simplifies layout and makes it easier to move things around. Another benefit is that nested panels can be collapsed in the XAML-editor.

Say your layout looks like this:

Then nesting grids like this creates a layout that is easy to reason about:

<Grid>
    <Grid.RowDefinitions>
        <RowDefinition Height="Auto" />
        <RowDefinition />
    </Grid.RowDefinitions>
    <Rectangle Grid.Row="0"
               Height="50"
               Fill="Red" />
    <Grid Grid.Row="1">
        <Grid.ColumnDefinitions>
            <ColumnDefinition />
            <ColumnDefinition Width="Auto" />
        </Grid.ColumnDefinitions>
        <Rectangle Grid.Column="0"
                   Fill="Yellow" />
        <Rectangle Grid.Column="1"
                   Width="50"
                   Fill="Blue" />
    </Grid>
</Grid>

• <DockPanel><Menu>...</Menu><StackPanel></StackPanel></DockPanel> there are a bunch of issues here. You add a panel with just a single child, not counting the empty StackPanel.
  • Panels come with a cost, they add to the visual tree hurting startup time and memory usage.
  • It adds noise to the XAML making it harder to figure out what it does.
• StatusBar and a couple of others are missing Grid.Column. Try to be consistent with the order of attributes. A common convention is x:Name first, then layout, then size, from memory. Pick a convention and be consistent. I can highly recommend Xaml Styler

• Order the elements in the XAML by Grid.Row and Grid.Column sample:

  <Rectangle Grid.Row="0" Grid.Column="0" Fill="Red" />
  <Rectangle Grid.Row="0" Grid.Column="1" Fill="Gray" />
  <Rectangle Grid.Row="1" Grid.Column="0" Fill="Yellow" />
  <Rectangle Grid.Row="1" Grid.Column="1" Fill="Blue" /> 
  

I usually do attribute per row for clean diffs but wanted a table look here.

• <TextBox Margin="5,45,5,29" margins like these look really suspicious. Use margins for adding margins around elements and not for positioning. Use panels for positioning.
• A note about panels: I estimate that I use Grid as panel 93% of the time.
• Label label is quite a bit more heavyweight than TextBlock unless you have a reason use TextBlock. First google hit on the subject