# Пиксель к пикселю: проверка проекта PixiEditor

Работа с графикой невозможна без специальных инструментов — графических редакторов\. Но что если ваш редактор вылетит прям во время работы из\-за багов? Давайте с помощью статического анализатора поищем потенциальные ошибки и странные места в исходном коде открытого проекта PixiEditor\.

![1299_PixiEditor_errors_ru/image1.png](https://import.viva64.com/docx/blog/1299_PixiEditor_errors_ru/image1.png)

## Введение

Что же такое [PixiEditor](https://pixieditor.net/)? Легко догадаться, что это 2D\-редактор , который ориентирован преимущественно на пиксельную графику\. Однако он также подходит для процедурной графики, редактирования изображений, векторной графики и анимации\. Стоит отметить, что это open source проект, который активно обновляется, и буквально любой может приложить руку к его дальнейшему развитию\.

Не могу не упомянуть крутую фишку PixiEditor — Node Graph\. Благодаря этому инструменту можно буквально программировать сцену\.

![1299_PixiEditor_errors_ru/image2.png](https://import.viva64.com/docx/blog/1299_PixiEditor_errors_ru/image2.png)

Команда PVS\-Studio активно участвует в развитии открытых проектов и помогает повышать качество и надёжность их кода\. Поэтому давайте поищем ошибки в этом редакторе версии [2\.0\.1\.15](https://github.com/PixiEditor/PixiEditor/tree/2.0.1.15) с помощью нашего статического анализатора, затем откроем issue и расскажем разработчикам о потенциальных проблемах\.

Проект достаточно качественно написан, поэтому хочется похвалить разработчиков, но ошибки и странные места всё равно присутствуют\. О некоторых из них расскажем в этой статье\.

Проверка будет осуществляться с помощью статического анализатора PVS\-Studio версии [7\.38](https://pvs-studio.ru/ru/blog/posts/1276/)\.

## Проверяем проект

**Фрагмент кода 1**

```cpp
private void WriteName(StringBuilder sb, string name)
{
  sb.Append('"');
  sb.Append(name);
  sb.Append('"');

  for (int j = name.Length; j < name.Length; j++)
  {
    sb.Append(' ');
  }
}
```

Сообщение PVS\-Studio: [V3028](https://pvs-studio.ru/ru/docs/warnings/v3028/) Consider inspecting the 'for' operator\. Initial and final values of the iterator are the same\. [CorelDrawPalParser\.cs 191](https://github.com/PixiEditor/PixiEditor/blob/5a7ab355da2e5491a8acf27580942348eca76f2e/src/PixiEditor/Models/IO/PaletteParsers/CorelDrawPalParser.cs#L191)

В качестве начального и конечного значения итератора задано одно и то же значение\. Следовательно, код внутри цикла никогда не отработает\.

Почему так произошло? Возможно, разработчик, хотел начать итерации с `name.Length` и закончить нулём, но опечатался и в условии также написал `name.Length`\. Однако не будем спешить с выводами, лучше давайте посмотрим единственное место, в котором вызывается этот метод\.

```cpp
public override async Task<bool> Save(string path, PaletteFileData data)
{
  StringBuilder sb = new StringBuilder(SWATCH_NAME.Length + 20);

  try
  {
    await using (Stream stream = File.OpenWrite(path))
    {
      using (TextWriter writer = new StreamWriter(stream, 
                                                  Encoding.ASCII, 
                                                  1024, 
                                                  true))
      {
        foreach (var color in data.Colors)
        {
          this.ConvertRgbToCmyk(color, 
                                out byte c, 
                                out byte m, 
                                out byte y, 
                                out byte k);

          this.WriteName(sb, SWATCH_NAME);
          this.WriteNumber(sb, c);
          this.WriteNumber(sb, m);
          this.WriteNumber(sb, y);
          this.WriteNumber(sb, k);

          writer.WriteLine(sb.ToString());
          sb.Length = 0;
        }
      }
    }
    return true;
  }
  catch
  {
    return false;
  }
}
```

Метод осуществляет сохранение палитры с цветами в файл\. Каждый цвет переводится в CMYK \(Cyan, Magenta, Yellow, Key/Black\) схему и сохраняется в таком формате:

```cpp
"PixiEditor Color"  0   0   0 100
```

Теперь мы можем прийти к выводу, что добавление символа пробела в строку в методе `WriteName` ненужно, поэтому цикл `for` является лишним\. И хоть код внутри него никогда не вызывается, это портит его качество и читаемость\. Сделать код чище можно следующим образом:

```cpp
private void WriteName(StringBuilder sb, string name)
{
  sb.Append('"');
  sb.Append(name);
  sb.Append('"');
}
```

**Фрагмент кода 2**

```cpp
public bool IsPointInside(VecD point)
{
  var top = TopLeft - TopRight;
  var right = TopRight - BottomRight;
  var bottom = BottomRight - BottomLeft;
  var left = BottomLeft - TopLeft;

  var deltaTopLeft = point - TopLeft;
  var deltaTopRight = point - TopRight;
  var deltaBottomRight = point - BottomRight;
  var deltaBottomLeft = point - BottomLeft;

  if (   deltaTopRight.IsNaNOrInfinity() 
      || deltaTopLeft.IsNaNOrInfinity() 
      || deltaBottomRight.IsNaNOrInfinity()
      || deltaBottomRight.IsNaNOrInfinity())
        return false;

  ....
}
```

Сообщение PVS\-Studio: [V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'deltaBottomRight\.IsNaNOrInfinity\(\)' to the left and to the right of the '\|\|' operator\. [ShapeCorners\.cs](https://github.com/PixiEditor/Drawie/blob/753784f70c3a455dce172ad2e92be63329f9d4c3/src/Drawie.Backend.Core/Numerics/ShapeCorners.cs#L155) 155

Анализатор обнаружил использование в условии двух одинаковых подвыражений `deltaBottomRight.IsNanOrInfinity()`\. Скорее всего, произошла опечатка, и на самом деле вместо второго подвыражения должно быть `deltaBottomLeft.IsNanOrInfinity()`, поскольку `deltaBottomLeft` сейчас не используется:

```cpp
public bool IsPointInside(VecD point)
{
    ....

  if (   deltaTopRight.IsNaNOrInfinity() 
      || deltaTopLeft.IsNaNOrInfinity() 
      || deltaBottomRight.IsNaNOrInfinity()
      || deltaBottomLeft.IsNaNOrInfinity())
        return false;

  ....
}
```

**Фрагмент кода 3**

```cpp
public override RectD? GetPreviewBounds(int frame, 
                                        string elementToRenderName = "")
{
  ....

  RectD? totalBounds = null;

  if (   Top.Connection != null 
      && Top.Connection.Node is IPreviewRenderable topPreview)
  {
    var topBounds = topPreview.GetPreviewBounds(frame, elementToRenderName);
    if (topBounds != null)
    {
      totalBounds = totalBounds?.Union(topBounds.Value) ?? topBounds;
    }
  }

  ....
}
```

Сообщение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'totalBounds' is always null\. The operator '?\.' is meaningless\. [MergeNode\.cs 87](https://github.com/PixiEditor/PixiEditor/blob/c934ade2efbe28cc0e9629e80247bb9d889da99b/src/PixiEditor.ChangeableDocument/Changeables/Graph/Nodes/MergeNode.cs#L87)

В этом случае `totalBounds` всегда будет `null` до момента его переприсваивания, потому что переменная инициализирована как `RectD? totalBounds = null`\. Поэтому в выражении `totalBounds?.Union(topBound.Value)` нет смысла\.

**Фрагмент кода 4**

```cpp
private List<IChangeInfo> ApplyToFrame(Document target, 
                                       StructureNode targetLayer, 
                                       int frame)
{
  ....

  foreach (var guid in ordererd)
  {
    var layer = target.FindMemberOrThrow<StructureNode>(guid);

    AddMissingKeyFrame(targetLayer, frame, layer, changes, target);

    if (layer is not IRasterizable or ImageLayerNode)
      continue;

    if (layer is ImageLayerNode imageLayerNode)
    {
      ....
    }
        ....
    }
}
```

Сообщение PVS\-Studio: [V3207](https://pvs-studio.ru/ru/docs/warnings/v3207/) The 'not IRasterizable or ImageLayerNode' logical pattern may not work as expected\. The 'not' pattern is matched only to the first expression from the 'or' pattern\. [CombineStructureMembersOnto\_Change\.cs 160](https://github.com/PixiEditor/PixiEditor/blob/c934ade2efbe28cc0e9629e80247bb9d889da99b/src/PixiEditor.ChangeableDocument/Changes/Drawing/CombineStructureMembersOnto_Change.cs#L160)

Разработчик при написании условия допустил логическую ошибку\. Сейчас оно работает следующим образом: условие вернёт `true`, если `layer` **НЕ** реализует интерфейс `IRasterizable` или `layer`\-экземпляр класса `ImageLayerNode`\. В таком случае следующее за ним условие `layer is ImageLayerNode` всегда будет ложно\. Его можно исправить следующим образом:

```cpp
if (layer is not IRasterizable or not ImageLayerNode)
      continue;
```

Такой паттерн ошибки достаточно распространён\. Недавно подобная ошибка была обнаружена в файловом менеджере Files, о чём мы написали в нашей [статье](https://pvs-studio.ru/ru/blog/posts/csharp/1295/)\. Всему виной то, что в нашем естественном языке отрицательная частица `not` применяется ко всем словам, которые мы перечисляем за ней\. В C\# же `not` является унарным оператором, который имеет соответственный приоритет и будет применён до `or` или `and`\. Почитать об этом поведении можно в [документе](https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-11-04.md#not-and-or-patterns)\.

**Фрагмент кода 5**

```cpp
public class CommandNameListGenerator : IIncrementalGenerator
{
  private const string Commands = 
  "PixiEditor.Models.Commands.Attributes.Commands";

  private const string Evaluators = 
  "PixiEditor.Models.Commands.Attributes.Evaluators";

  private const string Groups = 
  "PixiEditor.Models.Commands.Attributes.Commands";

  private const string InternalNameAttribute = 
  "PixiEditor.Models.Commands.Attributes.InternalNameAttribute";

  ....
}
```

Сообщение PVS\-Studio: [V3091](https://pvs-studio.ru/ru/docs/warnings/v3091/) Empirical analysis\. It is possible that a typo is present inside the string literal\. The 'Commands' word is suspicious\. [CommandNameListGenerator\.cs 17](https://github.com/PixiEditor/PixiEditor/blob/c934ade2efbe28cc0e9629e80247bb9d889da99b/src/PixiEditor.Gen/CommandNameListGenerator.cs#L17)

На этот раз нельзя сказать точно, ошибка это или нет, но достаточно странно, что две разные переменные имеют в качестве значения одну и ту же строку\. Притом заметно, как значения похожих переменных зависят от их названий\.

**Фрагмент кода 6**

```cpp
public static Tweener<double> Double(....)
{
    return new Tweener<double>(property, control, endValue, duration,
        (start, end, t) => start + (end - start) * easing?.Ease(t) ?? t);
}
```

Сообщение PVS\-Studio: [V3123](https://pvs-studio.ru/ru/docs/warnings/v3123/) Perhaps the '??' operator works in a different way than it was expected\. Its priority is lower than priority of other operators in its left part\. [Tweener\.cs 14](https://github.com/PixiEditor/PixiEditor/blob/c934ade2efbe28cc0e9629e80247bb9d889da99b/src/PixiEditor.UI.Common/Tweening/Tweener.cs#L14)

Здесь бинарное выражение, которое может дать не тот результат из\-за путаницы в приоритете операций\. Сначала будут применены арифметические операторы, в результате которых потенциально получится `null`, а затем будет применён оператор `??`, и всё выражение вернёт `t`\. Короче говоря, выражение будет обработано таким образом:

```cpp
(start + (end - start) * easing?.Ease(t)) ?? t
```

Возможно, правильная логика заключается в том, чтобы сначала обработать выражение `easing?.Ease(t) ?? t`, а затем арифметические операции слева:

```cpp
start + (end - start) * (easing?.Ease(t) ?? t)
```

 Для лучшего понимания в таком случае рекомендуется использовать скобки\.

**Фрагмент кода 7**

```cpp
private static void UseLanguageFlowDirectionPropertyChanged(....)
{
  ....

  if (!obj.NewValue.Value)
  {
    obj.Sender.SetValue(Control.FlowDirectionProperty, 
                        FlowDirection.LeftToRight);
    ILocalizationProvider.Current.OnLanguageChanged -= 
    (lang) => OnLanguageChangedFlowDirection(obj.Sender);
  }
  else
  {
    OnLanguageChangedFlowDirection(obj.Sender);
    ILocalizationProvider.Current.OnLanguageChanged += 
    (lang) => OnLanguageChangedFlowDirection(obj.Sender);
  }
}
```

Сообщение PVS\-Studio: [V3084](https://pvs-studio.ru/ru/docs/warnings/v3084/) Anonymous function is used to unsubscribe from 'OnLanguageChanged' event\. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration\. [Translator\.cs 78](https://github.com/PixiEditor/PixiEditor/blob/c934ade2efbe28cc0e9629e80247bb9d889da99b/src/PixiEditor.UI.Common/Localization/Translator.cs#L78)

Приведённая выше отписка от события не будет работать\. Она происходит с помощью анонимной функции и приводит к созданию отдельного экземпляра делегата, отличного от того, который используется ниже в подписке\. Таким образом, для подписки используется один делегат, а для отписки совсем другой, несмотря на то, что визуально они ничем не отличаются\.

Для того, чтобы корректно использовать анонимные функции при подписке на события \(если потребуется дальнейшая отписка\), можно сохранять обработчик\-лямбду в отдельную переменную и использовать её как при подписке, так и при отписке от события\.

**Фрагмент кода 8**

```cpp
public static async void CommandChanged(AvaloniaPropertyChangedEventArgs e)
{
  ....

  RelayCommand<object> wrapper = new RelayCommand<object>(parameter =>
  {
    if (!ShortcutController.ShortcutExecutionBlocked)
    {
      if (   command?.Shortcut != null 
          && command.Shortcut.Gesture != null 
          && command.Shortcut.Gesture.Key != Key.None 
          || command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None)
      {
        ViewModelMain.Current.ShortcutController.KeyPressed(
        false,
        command.Shortcut.Key,
        command.Shortcut.Modifiers);
      }
      else
      {
        if (iCommand.CanExecute(parameter))
        {
          iCommand.Execute(parameter);
        }
      }       
    }
    
    ....
       
  });

  ....
}
```

Сообщение PVS\-Studio: [V3042](https://pvs-studio.ru/ru/docs/warnings/v3042/) Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'command' object [NativeMenu\.cs 68](https://github.com/PixiEditor/PixiEditor/blob/c934ade2efbe28cc0e9629e80247bb9d889da99b/src/PixiEditor/Models/Commands/XAML/NativeMenu.cs#L68)

Анализатор сообщает нам о возможном `NullReferenceException` из\-за разыменования переменной `command`, которая может оказаться `null`\. Первое условие в `if` учитывает, что `command?.Shortcut` может быть `null`, поэтому в других условиях, объединённых с помощью `&&`, опасное разыменование не возникнет\. Но это не учтено в условии ` command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None`, которое связано с другими с помощью `||`\. Исправим, добавив в выражение оператор `?`:

```cpp
if (   command?.Shortcut != null 
    && command.Shortcut.Gesture != null 
    && command.Shortcut.Gesture.Key != Key.None 
    || command?.Shortcut?.Gesture.KeyModifiers != KeyModifiers.None)
{
  ....
}
```

Или можно правильно расставить скобки подобным образом:

```cpp
if (   command?.Shortcut != null && 
    (command.Shortcut.Gesture != null 
     && command.Shortcut.Gesture.Key != Key.None 
     || command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None))
{
  ....
}
```

В таком случае, если `command?.Shortcut` будет `null`, то другие условия не будут проверяться, и разыменования не возникнет\.

**Фрагмент кода 9**

```cpp
private void PointerMoved(object? sender, PointerEventArgs e)
{
  if (targetStart > draggedStart && draggedDeltaEnd >= targetMid)
  {
    ....
  }
  else if (targetStart < draggedStart && draggedDeltaStart <= targetMid)
  {
    ....
  }
  else
  {
    if (orientation == Orientation.Horizontal)
    {
        SetTranslateTransform(targetContainer, 0, 0);          // <=
    }
    else
    {
        SetTranslateTransform(targetContainer, 0, 0);          // <=
    }
  }
}
```

Сообщение PVS\-Studio: [V3004](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. [DockableTabDragBehavior\.cs 431](https://github.com/PixiEditor/PixiDocks/blob/1604a0bb1fdf1d0016bfc82752c85b3266bed2c2/src/PixiDocks.Avalonia/Behaviors/DockableTabDragBehavior.cs#L431)

Странно, что `then`\-выражение такое же, как у `else`\. Скорее всего, строку `SetTranslateTransorm(targetContainer, 0, 0)` в `else`\-ветке забыли изменить после копипаста\. На это также указывают похожие куски кода выше, в которых есть такое же условие `orientation == Orientation.Horizontal`:

```cpp
if (targetStart > draggedStart && draggedDeltaEnd >= targetMid)
{
  if (orientation == Orientation.Horizontal)
  {
    SetTranslateTransform(targetContainer, -draggedBounds.Width, 0);
  }
  else
  {
    SetTranslateTransform(targetContainer, 0, -draggedBounds.Height);
  }
  ....
}
else if (targetStart < draggedStart && draggedDeltaStart <= targetMid)
{
  if (orientation == Orientation.Horizontal)
  {
    SetTranslateTransform(targetContainer, draggedBounds.Width, 0);
  }
  else
  {
    SetTranslateTransform(targetContainer, 0, draggedBounds.Height);
  }
  ....
}
```

Возможно, что в любом случае действительно должно быть `SetTranslateTransorm(targetContainer, 0, 0)`, и условие избыточно\.

**Фрагмент кода 10**

```cpp
protected override void OnSelected(bool restoring)
{
  if (restoring) return;

  var document = ViewModelMain.Current?.DocumentManagerSubViewModel
                                       .ActiveDocument;
  document.Tools.UseRasterLineTool();
}
```

Сообщение PVS\-Studio: [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/) The 'document' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. RasterLineToolViewModel\.cs 69

Переменная `document` инициализирована с помощью оператора `?`, поэтому она может быть `null`, и в результате последующего разыменования может возникнуть `NullReferenceException`\. Обезопасить код можно с помощью проверки `document` на `null`:

```cpp
protected override void OnSelected(bool restoring)
{
  if (restoring) return;

  var document = ViewModelMain.Current?.DocumentManagerSubViewModel
                                       .ActiveDocument;

  if (document != null)
    document.Tools.UseRasterLineTool();
}
```

**Фрагмент кода 11**

```cpp
public override object ProvideValue(IServiceProvider serviceProvider)
{
    return (ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools)
            .FirstOrDefault(tool => tool.GetType().Name == TypeName);
}
```

Сообщение PVS\-Studio: [V3153](https://pvs-studio.ru/ru/docs/warnings/v3153/) Dereferencing the result of null\-conditional access operator can lead to NullReferenceException\. Consider removing parentheses around null\-conditional access expression\. [ToolVM\.cs 14](https://github.com/PixiEditor/PixiEditor/blob/c934ade2efbe28cc0e9629e80247bb9d889da99b/src/PixiEditor/ViewModels/ToolVM.cs#L14)

Разыменование `ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools` может привести к выбросу исключения, поскольку выражение получено с помощью оператора `?`\. Можно не допустить этого, добавив оператор `?`:

```cpp
public override object ProvideValue(IServiceProvider serviceProvider)
{
    return (ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools)
            ?.FirstOrDefault(tool => tool.GetType().Name == TypeName);
}
```

**Фрагмент кода 12**

```cpp
public void DrawLine(VecD from, VecD to, Paint paint)
{
  ShapeCorners corners = new ShapeCorners(from, to, paint.StrokeWidth);
  IDisposable? reset = null;
  if (paint?.Paintable != null)
  {
    ....
  }

  ....
}
```

Сообщение PVS\-Studio: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'paint' object was used before it was verified against null\. Check lines: 272, 274\. [Canvas\.cs 272](https://github.com/PixiEditor/Drawie/blob/753784f70c3a455dce172ad2e92be63329f9d4c3/src/Drawie.Backend.Core/Surfaces/Canvas.cs#L285)

Здесь разработчик предполагает, что параметр `paint` может оказаться `null`, и проверяет это с помощью условия `paint?.Paintable != null`\. Но до этого условия происходит разыменование `paint.StrokeWidth`, и может возникнуть `NullReferenceException`\.

**Фрагмент кода 13**

```cpp
protected override void OnApplyTemplate(TemplateAppliedEventArgs e)
{
  base.OnApplyTemplate(e);
  var popupPart = e.NameScope.Find<Popup>("popup");
  popupPart.Closed += PopupPartOnClosed;
  if (popupPart != null)
  {
    popupPart.PointerPressed += (sender, args) => { args.Handled = true; };
  }
}
```

Сообщение PVS\-Studio: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'popupPart' object was used before it was verified against null\. Check lines: 90, 91\. [PortableColorPicker\.cs 90](https://github.com/PixiEditor/ColorPicker/blob/61055feed27354e6be969055fc0ee5db3c7d3b94/src/ColorPicker.AvaloniaUI/PortableColorPicker.cs#L90)

Та же ситуация, как в предыдущем фрагменте кода\. Следует сначала проверить переменную `popupPart` на `null`, а затем произвести разыменование:

```cpp
protected override void OnApplyTemplate(TemplateAppliedEventArgs e)
{
  base.OnApplyTemplate(e);
  var popupPart = e.NameScope.Find<Popup>("popup");

  if (popupPart != null)
  {
    popupPart.Closed += PopupPartOnClosed;
    popupPart.PointerPressed += (sender, args) => { args.Handled = true; };
  }
}
```

## Подводим итоги

Проект достаточно хорошо написан и содержит не так много багов\. И большинство ошибок было допущено, скорее всего, по невнимательности\.

Однако следует понимать, что с увеличением кодовой базы проекта количество багов и ошибок также будет расти\. Поэтому будет очень полезно использовать инструменты статического анализа\.

Стоит отметить, что наш анализатор PVS\-Studio можно использовать совершенно бесплатно\. Если вы участвуете в разработке открытого проекта \(например, PixiEditor\), то можете получить бесплатную версию\. Подробности описаны в [статье](https://pvs-studio.ru/ru/blog/posts/1286/) про бесплатное лицензирование\.

Но даже если вы не являетесь разработчиком открытого проекта, то всё равно можете запросить пробный ключ на этой [странице](https://pvs-studio.ru/ru/pvs-studio/try-free/) и бесплатно попробовать последнюю версию анализатора PVS\-Studio\. 

Берегите себя и свой код\!