Shift selection

Question

Basically I have done a code for a project which works with a shift and ctrl selection. The program consists of a chart with specified points, which the user is able to select with the different key controls in order to move them. The highlightedIndex is a list of int. I am also working with WPF and later on I have to bind this with a DataGrid, so I do not know if this is the right approach or is any easier way to get this done with the Binding from WPF. I want to be able to select the points with Ctrl or Shift as Windows works, it should look like this.

public partial class MainWindow : Window
{
    public ChartValues<ObservablePoint> Chartvalues { get; set; }
    public SeriesCollection SeriesCollection { get; set; }
    public CartesianMapper<ObservablePoint> Mapper { get; set; }
    public Brush DangerBrush { get; set; }
    private int movingPointIdx = -1; //-1: no point moving
    private const int roundedNumber = 2;
    private const double min_offset = 0.01;
    private List<int> highlightedIndex = new List<int>();
    private int lastClickedPointIdx = -1;
    int firstvaluepoint = 0;

    public MainWindow()
    {
        InitializeComponent();
        Chartvalues = new ChartValues<ObservablePoint>();
        InitPoints();

        Mapper = Mappers.Xy<ObservablePoint>()
            .X(item => item.X)
            .Y(item => item.Y)
            .Stroke((item, index) => highlightedIndex.Contains(index) ? DangerBrush : null);

        DangerBrush = new SolidColorBrush(Color.FromRgb(253, 23, 23));

        var lineSeries = new LineSeries
        {
            Title = "Line 1",
            Values = Chartvalues,
            StrokeThickness = 4,
            Fill = Brushes.Transparent,
            PointGeometrySize = 15,
            LineSmoothness = 0.2,
            Configuration = Mapper,
            DataLabels = false,
        };
        SeriesCollection = new SeriesCollection { lineSeries };
        DataContext = this;
    }

    private void InitPoints() //plot the values of the chart
    {
        Chartvalues.Clear();
        double[] xvals = { 0, 2, 3, 4, 5, 6, 7, 8, 9 };
        double[] yvals = { 0, 1, 2, 3, 1, 2, 3, 1, 2 };

        for (int i = 0; i < xvals.Length; i++)
        {
            Chartvalues.Add(new ObservablePoint { X = xvals[i], Y = yvals[i] });
        }
    }

    private void ChartOnDataClick(object sender, ChartPoint p)
    {
        Chartvalues = (ChartValues<ObservablePoint>)SeriesCollection[0].Values;

        foreach (ObservablePoint val in Chartvalues)
        {
            if (val.X == p.X && val.Y == p.Y) //this is the clicked point
            {
                movingPointIdx = Chartvalues.IndexOf(val); //get index of currently clicked point

                Chartvalues[movingPointIdx].X = Chartvalues[movingPointIdx].X; //highlights selected point

                KeyPressed();
            }
        }
    }

    private void KeyPressed()
    {
        bool shiftclick = false;
        int firstselectedpoint = 0;
        if (Keyboard.IsKeyDown(Key.LeftCtrl) || Keyboard.IsKeyDown(Key.RightCtrl)) //Ctrl
        {
            if (highlightedIndex.Contains(movingPointIdx)) //remove point if already is in the highlightedIndex list
            {
                highlightedIndex.Remove(movingPointIdx);
                firstselectedpoint = movingPointIdx;
                shiftclick = false;
                return;
            }
            else //otherwise add it to the highlightedIndex list
            {
                highlightedIndex.Add(movingPointIdx);
                highlightedIndex.Sort();
                firstselectedpoint = movingPointIdx;
                shiftclick = false;
            }
        }
        else if (Keyboard.IsKeyDown(Key.LeftShift) || Keyboard.IsKeyDown(Key.RightShift)) //Shift
        {
            if (!shiftclick)
            {
                highlightedIndex.Clear();
                shiftclick = true;
            }            
            if (lastClickedPointIdx > -1) //highlights all points 
            {                            
                if (firstvaluepoint > firstselectedpoint)
                {
                    for (int i = firstselectedpoint; i <= firstvaluepoint; i++)
                    {
                        highlightedIndex.Add(i);
                    }
                }
                else
                {
                    for (int i = firstvaluepoint; i <= firstselectedpoint; i++)
                    {
                        highlightedIndex.Add(i);
                    }
                }                                                        
            }
        }
        else //no modifier key pressed -> only add the current point to the list
        {
            highlightedIndex.Clear();
            highlightedIndex.Add(movingPointIdx);
        }
        lastClickedPointIdx = movingPointIdx;
        if (!shiftclick)
            firstvaluepoint = firstselectedpoint; 
    }

    private void ChartOnMouseMove(object sender, MouseEventArgs e)
    {
        if (e.LeftButton != MouseButtonState.Pressed)
            return;

        if (movingPointIdx == -1)
            return;

        var newPoint = diagram.ConvertToChartValues(e.GetPosition(diagram));
        Chartvalues[movingPointIdx].X = Math.Round(newPoint.X, roundedNumber);
        Chartvalues[movingPointIdx].Y = Math.Round(newPoint.Y, roundedNumber);

        CheckForOnePointBoundaries(Chartvalues[movingPointIdx].X);
    }

    private void CheckForOnePointBoundaries(double xBoundary)//checks if the point has reacher his boundary (next or last point)
    {
        double leftBoundary = 0;
        double rightBoundary = Chartvalues[Chartvalues.Count - 1].X;

        if (movingPointIdx > 0)
            leftBoundary = Chartvalues[movingPointIdx - 1].X + min_offset;

        if (movingPointIdx < Chartvalues.Count - 1)
            rightBoundary = Chartvalues[movingPointIdx + 1].X - min_offset;

        if (xBoundary < leftBoundary)
        {
            Chartvalues[movingPointIdx].X = leftBoundary;
        }

        else if (xBoundary > rightBoundary)
        {
            Chartvalues[movingPointIdx].X = rightBoundary;
        }
    }

    private void ChartOnMouseLeftButtonUp(object sender, MouseButtonEventArgs e) => movingPointIdx = -1; // deactivate point moving

    private void AddPoints(object sender, MouseButtonEventArgs e)
    {
        if (e.ChangedButton == MouseButton.Left)
        {
            var newPoint = diagram.ConvertToChartValues(e.GetPosition(diagram));
            var addedPoint = new ObservablePoint(Math.Round(newPoint.X, roundedNumber), Math.Round(newPoint.Y, roundedNumber));
            Chartvalues.Add(addedPoint);

            SortChartValues();
            highlightedIndex.Clear();

            for (int i = 0; i < Chartvalues.Count; i++) 
            {
                if (addedPoint.X == Chartvalues[i].X) // adds the clicked point to the list
                {
                    highlightedIndex.Add(i);
                    i = Chartvalues.Count;
                }
            }
        }
    }

    private void SortChartValues()
    {
        List<Point> list = new List<Point>();

        foreach (ObservablePoint val in Chartvalues)
        {
            list.Add(new Point(val.X, val.Y));
        }
        list = list.OrderBy(p => p.X).ToList();

        Chartvalues.Clear();

        for (int i = 0; i < list.Count; i++)
        {
            Chartvalues.Add(new ObservablePoint { X = list[i].X, Y = list[i].Y });
        }
    }

    private void RemovePoints(object sender, RoutedEventArgs e)
    {
        if (movingPointIdx == -1)
            return;

        if (Chartvalues.Count > 1)
        {
            Chartvalues.RemoveAt(movingPointIdx);
            highlightedIndex.RemoveAt(movingPointIdx);
            movingPointIdx = -1;
        }
    }
}

XAML

<Grid>
    <lvc:CartesianChart  Margin="10,35,27,5" x:Name="diagram" LegendLocation="Top" Series="{Binding SeriesCollection}" DataClick="ChartOnDataClick" DisableAnimations="True"/>
</Grid>

Where is highlightedIndex defined? It looks like this is all a method part of a class. Please share the rest of the class as well, preferably with a small usage example. — Mast
– Mast ♦, Commented Jun 3, 2020 at 17:32
@Mast iÍ have already upload the whole class, what do you mean by usage example? Like a screenshot? — Juan Rendon LS
– Juan Rendon LS, Commented Jun 3, 2020 at 18:09
@Mast the highlightedIndex is initialized in the ctor and filled in the KeySelection() method — Juan Rendon LS
– Juan Rendon LS, Commented Jun 3, 2020 at 18:26
Did you use your class to create that plot? If so, when you share the code that created it that's exactly what I meant with a usage example. — Mast
– Mast ♦, Commented Jun 3, 2020 at 19:24
@Mast Yes I used it to plot those values and I used Livecharts_ for the diagram. — Juan Rendon LS
– Juan Rendon LS, Commented Jun 3, 2020 at 19:38

JanDotNet · Accepted Answer · 2020-06-07 13:23:14Z

Welcome to code review.

Avoid using writable instance variables

Using writable instance variables in various methods should be avoided if possible, because the code becomes very fast complex.

For example the property 'Chartvalues' will be set in 'constructor' and in method 'ChartOnDataClick' and used in other methods.

This desing can be simply improved by using a read-only property:

public ChartValues<ObservablePoint> Chartvalues => (ChartValues<ObservablePoint>)SeriesCollection[0].Values

and changing the initialization like:

public MainWindow()
{
    ....
    var lineSeries = new LineSeries
    {
        Title = "Line 1",
        Values = new ChartValues<ObservablePoint>(GetInitialPoints()),
        StrokeThickness = 4,
        ....
}

private IEnumerable<ObservablePoint> GetInitialPoints() //plot the values of the chart
{
    double[] xvals = { 0, 2, 3, 4, 5, 6, 7, 8, 9 };
    double[] yvals = { 0, 1, 2, 3, 1, 2, 3, 1, 2 };

    for (int i = 0; i < xvals.Length; i++)
    {
        yield new ObservablePoint { X = xvals[i], Y = yvals[i] };
    }
}

Use Keyboard.Modifiers instead of checking modifier keys seperatly

Instead of Keyboard.IsKeyDown(Key.LeftCtrl) || Keyboard.IsKeyDown(Key.RightCtrl) the simpler form (Keyboard.Modifiers & ModifierKeys.Control) != 0 can be used.

Keep it simple

shiftclick is actually not necessary. You can remove the variable and lots of code with it.

Name event handlers like event handlers

It is a good style to name event handler like event handler and extract functional code to methods.

For example:

private void MouseClickedEventHandler(object sender, MouseButtonEventArgs e)
{
    if (e.ChangedButton == MouseButton.Left)
    {
        var newPoint = diagram.ConvertToChartValues(e.GetPosition(diagram));
        AddPoint(newPoint);
    }
}

private void AddPoint(Point newPoint)
{
    var addedPoint = new ObservablePoint(Math.Round(newPoint.X, roundedNumber), Math.Round(newPoint.Y, roundedNumber));
    Chartvalues.Add(addedPoint);

    SortChartValues();
    highlightedIndex.Clear();

    for (int i = 0; i < Chartvalues.Count; i++) 
    {
        if (addedPoint.X == Chartvalues[i].X) // adds the clicked point to the list
        {
            highlightedIndex.Add(i);
            i = Chartvalues.Count;
        }
    }
}

When complexity increases, use object oriented design to keep it maintainable

The problem with writing all that event handling code in one class with global states (writable instance variables) is, that it does not scale. For your use case it may work... but adding new use cases will very likly break exiting functionality and code becomes unmaintainable.

Fortunately, we are working with a programming language supporting advanced object oriented design, so why not using it.

We have a chart that we want to interact with. At any given time, we are in a certain logical state (e.g.: Idle, Point selected, Moving selection rectangle, ...) and incoming events can change the current logical state to another one.

Instead of having one global state (all writable instance variables) where the logical state can be derived from, why not designing the logical states directly? This design is already known as state pattern.

In your case, state diagram may look like:

Corresponding implementation of the state pattern may look like:

internal interface IChartContext
{
    ChartPoint AddPoint(Point position);
    void Select(ChartPoint chartPoint);
    void ToggleSelection(ChartPoint chartPoint);
    void ExtendSelection(ChartPoint chartPoint);
    void ChangeState(State state);
}

internal abstract class State
{
    protected State(IChartContext ctx)
    {
        this.Ctx = ctx;
    }

    public virtual void OnLeftMouseUp(MouseButtonEventArgs e) { }
    public virtual void OnLeftMouseDown(MouseButtonEventArgs e) { }
    public virtual void OnMouseMove(MouseEventArgs e) { }

    internal IChartContext Ctx { get; }
}

internal class IdleState : State
{
    public IdleState(IChartContext ctx) : base(ctx) { }

    public override void OnLeftMouseDown(MouseButtonEventArgs e)
    {
        var point = e.GetPosition(this.Ctx.DrawingArea);
        var point = this.Ctx.AddPoint(point);
        this.Ctx.ChangeState(new PointMovingState(this.Ctx, point));
    }

    public override void OnMouseMove(MouseEventArgs e)
    {
        var point = e.GetPosition(this.Ctx.DrawingArea);
        var chartPoint = this.Ctx.GetChartPoint(point);
        if (chartPoint != null)
        {
            this.Ctx.ChangeState(new MouseOverPoint(this.Ctx, chartPoint));
        }
    }
}

internal class PointMovingState : State
{
    private readonly ChartPoint chartPoint;

    public PointMovingState(IChartContext ctx, ChartPoint chartPoint) : base(ctx) 
    {
        this.chartPoint = chartPoint;
    }

    public override void OnMouseMove(MouseEventArgs e)
    {
        var point = e.GetPosition(this.Ctx.DrawingArea);
        this.chartPoint.X = point.X;
        this.chartPoint.Y = point.Y;
    }

    public override OnLeftMouseUp(MouseButtonEventArgs e)
    {
        this.Ctx.ChangeState(new MouseOverPoint(this.Ctx, point));
    }
}

internal class MouseOverPoint : State
{
    private readonly ChartPoint chartPoint;

    public MouseOverPoint(IChartContext ctx, ChartPoint chartPoint) : base(ctx)
    {
        this.chartPoint = chartPoint;
    }

    public override void OnMouseMove(MouseEventArgs e)
    {
        var point = e.GetPosition(this.Ctx.DrawingArea);
        var chartPoint = this.Ctx.GetChartPoint(point);
        if (chartPoint == null)
        {
            this.Ctx.ChangeState(new IdleState(this.Ctx));
        }
        if (chartPoint != this.chartPoint)
        {
            this.Ctx.ChangeState(new MouseOverPoint(this.Ctx, chartPoint));
        }
    }

    public override OnLeftMouseDown(MouseButtonEventArgs e)
    {
        if (Keyboard.Modifiers & ModifierKeys.Control != 0)
        {
            this.Ctx.ToggleSelection(this.chartPoint);
        }
        else if (Keyboard.Modifiers & ModifierKeys.Shift != 0)
        {
            this.Ctx.ExtendSelection(this.chartPoint);
        }
        else
        {
            this.Ctx.Select(this.chartPoint);
        }

        this.Ctx.ChangeState(new PointMovingState(this.Ctx, point));
    }
}


public partial class MainWindow : Window, IChartContext
{
    private State state;

    public MainWindow()
    {
        InitializeComponent();

        // initialization logic     
        this.state = new IdleState(this);
    }

    public ChartPoint AddPoint(Point position) {
        // add logic for adding point       
    }
    void Select(ChartPoint chartPoint) {
        // add logic for select single point
    }
    void ToggleSelection(ChartPoint chartPoint) {
        // add logic for toggle selection
    }
    void ExtendSelection(ChartPoint chartPoint) {
        // add logic for extend selection
    }
    void ChangeState(State state) {
        this.state = state;
    }

    private void ChartOnMouseLeftButtonUp(object sender, MouseButtonEventArgs e) => this.state.OnLeftMouseUp(e);    
    private void ChartOnMouseLeftButtonDown(object sender, MouseButtonEventArgs e) => this.state.OnLeftMouseDown(e);    
    private void ChartOnMouseMove(object sender, MouseEventArgs e) => this.state.OnMouseMove(e);
}

Even if there is a lot of structual code, that design allows complex chart interaction and will scale with complexity. Therefore it is possible to add more use cases (states and transistions) without touching (and break) existing functionality.

For very simple use cases it may be over-engineered, but in your case I would think about using that pattern.

Stack Exchange Network

Shift selection

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Shift selection

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions