Our website uses cookies to enhance your browsing experience.
Graphic design work requires specialized tools—graphic editors. But what if the editor crashes during a critical task due to bugs? Let's use a static analyzer to search for potential errors and unusual code patterns in the open-source PixiEditor project.
What is PixiEditor? As the name suggests, it's a 2D editor focused on pixel art. However, it also supports procedural graphics, image editing, vector graphics, and animation. Notably, it's an open-source project under active development, meaning anyone can contribute to its evolution.
I must highlight one particularly impressive PixiEditor feature: The Node Graph. This tool essentially lets us program our scene visually.
The PVS-Studio team actively supports open-source projects by helping improve their code quality and reliability. So, let's examine the 2.0.1.15 version of this editor using our static analyzer and then open issues to inform the developers about potential problems.
The project is mostly well-written, and the developers deserve credit, but some errors and unusual code patterns still take place. This article covers some of them.
We performed the analysis using PVS-Studio static analyzer version 7.38.
Snippet 1
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(' ');
}
}The PVS-Studio warning: V3028 Consider inspecting the 'for' operator Initial and final values of the iterator are the same. CorelDrawPalParser.cs 191
The loop iterator uses identical start and end values, making the loop body unreachable.
What caused this? The developer likely intended to iterate from name.Length down to zero but mistakenly used name.Length in both positions. However, let's verify this by examining the method's only usage.
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;
}
}This method saves color palettes to a file. Each color is converted to CMYK (Cyan, Magenta, Yellow, Key/Black) format and is saved as:
"PixiEditor Color" 0 0 0 100We can now conclude that adding space characters in the WriteName method is unnecessary, making the for loop redundant. Though the loop never executes, it affects code quality and readability. Here's a cleaner implementation:
private void WriteName(StringBuilder sb, string name)
{
sb.Append('"');
sb.Append(name);
sb.Append('"');
}Snippet 2
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;
....
}The PVS-Studio warning: V3001 There are identical sub-expressions 'deltaBottomRight.IsNaNOrInfinity()' to the left and to the right of the '||' operator. ShapeCorners.cs 155
The analyzer detected identical deltaBottomRight.IsNanOrInfinity() subexpressions in the condition. This appears to be a typo—the second subexpression should likely be deltaBottomLeft.IsNanOrInfinity(), since deltaBottomLeft currently remains unused:
public bool IsPointInside(VecD point)
{
....
if ( deltaTopRight.IsNaNOrInfinity()
|| deltaTopLeft.IsNaNOrInfinity()
|| deltaBottomRight.IsNaNOrInfinity()
|| deltaBottomLeft.IsNaNOrInfinity())
return false;
....
}Snippet 3
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;
}
}
....
}The PVS-Studio warning: V3022 Expression 'totalBounds' is always null. The operator '?.' is meaningless. MergeNode.cs 87
Here, totalBounds always remains null until reassigned because it's initialized as RectD? totalBounds = null. This makes the totalBounds?.Union(topBound.Value) expression meaningless.
Snippet 4
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)
{
....
}
....
}
}The PVS-Studio warning: 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
The condition contains a logical error. It currently returns true if layer does NOT implement the IRasterizable interface OR layer is an instance of the ImageLayerNode class. This makes the subsequent layer is ImageLayerNode condition always false. Here's the corrected version:
if (layer is not IRasterizable or not ImageLayerNode)
continue;This error pattern is quite common. We recently found a similar error in the Files file manager, which we covered in an article. The issue stems from how our speaking language applies the not negative particle to all enumerated items, whereas in C# the not unary operator has higher precedence than or or and. You can read more about this behavior in the documentation.
Snippet 5
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";
....
}The PVS-Studio warning: V3091 Empirical analysis. It is possible that a typo is present inside the string literal. The 'Commands' word is suspicious. CommandNameListGenerator.cs 17
While we can't be certain this is an error, it's unusual for two different variables to contain identical string values, especially when their names suggest different purposes.
Snippet 6
public static Tweener<double> Double(....)
{
return new Tweener<double>(property, control, endValue, duration,
(start, end, t) => start + (end - start) * easing?.Ease(t) ?? t);
}The PVS-Studio warning: 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
This binary expression might produce unexpected results due to operator precedence confusion. Arithmetic operators would execute first, potentially resulting in null, followed by the?? operator execution. The entire expression would return t. In short, the expression would be handled as follows:
(start + (end - start) * easing?.Ease(t)) ?? tPerhaps, the correct option is to handle easing?.Ease(t) ?? t first, then perform the arithmetic operations on the left:
start + (end - start) * (easing?.Ease(t) ?? t)We recommend using parentheses for clarity.
Snippet 7
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);
}
}The PVS-Studio warning: 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
The unsubscribe operation above won't work correctly. It uses an anonymous function, creating a delegate instance different from the one used for subscription. Thus, one delegate is used for subscribing, and a completely different one for unsubscribing, despite the fact that they visually don't differ in any way.
To properly use anonymous functions for event subscription (if unsubscribing is needed later), store the lambda handler in a separate variable and use it for both subscription and unsubscription.
Snippet 8
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);
}
}
}
....
});
....
}The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'command' object NativeMenu.cs 68
The analyzer warns about potential NullReferenceException due to dereferencing the command variable, which could be null. The first condition in the if statement accounts for command?.Shortcut potentially being null, so other conditions combined with && avoid dangerous dereferencing. However, the command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None condition connected via || doesn't account for this. We can fix this by adding the ? operator:
if ( command?.Shortcut != null
&& command.Shortcut.Gesture != null
&& command.Shortcut.Gesture.Key != Key.None
|| command?.Shortcut?.Gesture.KeyModifiers != KeyModifiers.None)
{
....
}Or the author might consider placing parentheses correctly in the following way:
if ( command?.Shortcut != null &&
(command.Shortcut.Gesture != null
&& command.Shortcut.Gesture.Key != Key.None
|| command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None))
{
....
}This ensures that if command?.Shortcut is null, other conditions won't be checked, preventing dereferencing issues.
Snippet 9
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); // <=
}
}
}The PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. DockableTabDragBehavior.cs 431
A strange identical expression both in the then and else blocks. The SetTranslateTransorm(targetContainer, 0, 0) string in the else branch was likely forgotten after the copy-paste modification. Similar code fragments above with the same orientation == Orientation.Horizontal condition support this theory:
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);
}
....
}Alternatively, SetTranslateTransorm(targetContainer, 0, 0) might be correct for both cases, making the condition redundant.
Snippet 10
protected override void OnSelected(bool restoring)
{
if (restoring) return;
var document = ViewModelMain.Current?.DocumentManagerSubViewModel
.ActiveDocument;
document.Tools.UseRasterLineTool();
}The PVS-Studio warning: V3105 The 'document' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RasterLineToolViewModel.cs 69
The document variable uses the ? operator for initialization, making it potentially null. Subsequent dereferencing could cause NullReferenceException. We can prevent this by adding a null check for document:
protected override void OnSelected(bool restoring)
{
if (restoring) return;
var document = ViewModelMain.Current?.DocumentManagerSubViewModel
.ActiveDocument;
if (document != null)
document.Tools.UseRasterLineTool();
}Snippet 11
public override object ProvideValue(IServiceProvider serviceProvider)
{
return (ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools)
.FirstOrDefault(tool => tool.GetType().Name == TypeName);
}The PVS-Studio warning: V3153 Dereferencing the result of null-conditional access operator can lead to NullReferenceException. Consider removing parentheses around null-conditional access expression. ToolVM.cs 14
Dereferencing ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools might throw an exception since we got the expression by using the ?.operator. We can prevent this by adding the ? operator:
public override object ProvideValue(IServiceProvider serviceProvider)
{
return (ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools)
?.FirstOrDefault(tool => tool.GetType().Name == TypeName);
}Snippet 12
public void DrawLine(VecD from, VecD to, Paint paint)
{
ShapeCorners corners = new ShapeCorners(from, to, paint.StrokeWidth);
IDisposable? reset = null;
if (paint?.Paintable != null)
{
....
}
....
}The PVS-Studio warning: V3095 The 'paint' object was used before it was verified against null. Check lines: 272, 274. Canvas.cs 272
Here, the developer anticipates that the paint parameter could be null and checks this using the paint?.Paintable != null condition. However, paint.StrokeWidth is dereferenced before this check, potentially causing NullReferenceException.
Snippet 13
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; };
}
}The PVS-Studio warning: V3095 The 'popupPart' object was used before it was verified against null. Check lines: 90, 91. PortableColorPicker.cs 90
Here we've got the same situation as in the previous code fragment. We should first check the popupPart variable for null before dereferencing:
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; };
}
}The project demonstrates decent code quality with relatively few bugs—most errors appear to be oversights.
However, remember that as a project codebase grows, so does the potential for bugs and errors. This makes static analysis tools increasingly valuable.
Note that PVS-Studio offers free options. If you contribute to open-source projects (like PixiEditor), you can get a free license. Details are available in our article that covers free licensing.
Even if you're not an open-source developer, you can request a trial key on this page to try the latest PVS-Studio version for free.
Take care of yourself and your code!
0
0
0
36
0
0