ronantakizawa/codereview-bench huggingface dataset:
Repository: AvaloniaUI/Avalonia
File: src/Avalonia.X11/Clipboard/EventStreamWindow.cs (C#)

Code before:
            if (filter(xev))
            {
                _listeners.RemoveAt(i);
                i--;
                tcs.SetResult(xev);
            }
        }
    }

    public Task<XEvent?> WaitForEventAsync(Func<XEvent, bool> predicate, TimeSpan? timeout = null)
    {
        timeout ??= TimeSpan.FromSeconds(5);
        
        if (timeout < TimeSpan.Zero)
            throw new TimeoutException();
        if(timeout > TimeSpan.FromDays(1))
            throw new ArgumentOutOfRangeException(nameof(timeout));
        
        var tcs = new TaskCompletionSource<XEvent?>();
        _addedListeners.Add((predicate, tcs, _time.Elapsed + timeout.Value));

        _timeoutTimer.Start();
        return tcs.Task;
    }
    
    public void Dispose()
    {
        _platform.Windows.Remove(_handle);
        if (_isForeign)
            XLib.XSelectInput(_platform.Display, _handle, IntPtr.Zero);
        else
            XLib.XDestroyWindow(_platform.Display, _handle);
        
        _handle = IntPtr.Zero;
        var toDispose = _listeners.ToList();
        toDispose.AddRange(_addedListeners);
        _listeners.Clear();
        _addedListeners.Clear();
        foreach(var l in toDispose)
            l.tcs.SetResult(null);
    }
}

Review comment (suggestion):
The `_timeoutTimer` should probably be stopped here. (Currently it will be automatically stopped on next tick since there won't be any listener remaining, but it's better to cleanup asap.)

Code after:
            if (filter(xev))
            {
                _listeners.RemoveAt(i);
                i--;
                tcs.SetResult(xev);
            }
        }
    }

    public Task<XEvent?> WaitForEventAsync(Func<XEvent, bool> predicate, TimeSpan? timeout = null)
    {
        timeout ??= TimeSpan.FromSeconds(5);
        
        if (timeout < TimeSpan.Zero)
            throw new TimeoutException();
        if(timeout > TimeSpan.FromDays(1))
            throw new ArgumentOutOfRangeException(nameof(timeout));
        
        var tcs = new TaskCompletionSource<XEvent?>();
        _addedListeners.Add((predicate, tcs, _time.Elapsed + timeout.Value));

        _timeoutTimer.Start();
        return tcs.Task;
    }
    
    public void Dispose()
    {
        _timeoutTimer.Stop();

        _platform.Windows.Remove(_handle);
        if (_isForeign)
            XLib.XSelectInput(_platform.Display, _handle, IntPtr.Zero);
        else
            XLib.XDestroyWindow(_platform.Display, _handle);
        
        _handle = IntPtr.Zero;
        var toDispose = _listeners.ToList();
        toDispose.AddRange(_addedListeners);
        _listeners.Clear();
        _addedListeners.Clear();
        foreach(var l in toDispose)
            l.tcs.SetResult(null);
    }
}
Repository: AvaloniaUI/Avalonia
File: src/Avalonia.Controls/DataValidationErrors.cs (C#)

Code before:
using System;
using System.Collections.Generic;
using System.Linq;
using Avalonia.Reactive;
using Avalonia.Controls.Metadata;
using Avalonia.Controls.Templates;
using Avalonia.Data;

namespace Avalonia.Controls
{
    /// <summary>
    /// A control which displays an error notifier when there is a DataValidationError. 
    /// Provides attached properties to track errors on a control
    /// </summary>
    /// <remarks>
    /// You will probably only want to create instances inside of control templates.
    /// </remarks>
    [PseudoClasses(":error")]
    public class DataValidationErrors : ContentControl
    {
        
        /// <summary>
        /// Defines the DataValidationErrors.Errors attached property.
        /// </summary>
        public static readonly AttachedProperty<IEnumerable<object>?> ErrorsProperty =
            AvaloniaProperty.RegisterAttached<DataValidationErrors, Control, IEnumerable<object>?>("Errors");

        /// <summary>
        /// Defines the DataValidationErrors.HasErrors attached property.
        /// </summary>
        public static readonly AttachedProperty<bool> HasErrorsProperty =
            AvaloniaProperty.RegisterAttached<DataValidationErrors, Control, bool>("HasErrors");
        

Review comment (suggestion):
You shouldn't remove the newline here... 

@maxkatz6 since formatting only changes aren't allowed we should be much stricter on stuff like this. Still hoping we can get editorconfig rules setup at some point.

Code after:
using System;
using System.Collections.Generic;
using System.Linq;
using Avalonia.Reactive;
using Avalonia.Controls.Metadata;
using Avalonia.Controls.Templates;
using Avalonia.Data;
namespace Avalonia.Controls
{
    /// <summary>
    /// A control which displays an error notifier when there is a DataValidationError. 
    /// Provides attached properties to track errors on a control
    /// </summary>
    /// <remarks>
    /// You will probably only want to create instances inside of control templates.
    /// </remarks>
    [PseudoClasses(":error")]
    public class DataValidationErrors : ContentControl
    {
        
        /// <summary>
        /// Defines the DataValidationErrors.Errors attached property.
        /// </summary>
        public static readonly AttachedProperty<IEnumerable<object>?> ErrorsProperty =
            AvaloniaProperty.RegisterAttached<DataValidationErrors, Control, IEnumerable<object>?>("Errors");

        /// <summary>
        /// Defines the DataValidationErrors.HasErrors attached property.
        /// </summary>
        public static readonly AttachedProperty<bool> HasErrorsProperty =
            AvaloniaProperty.RegisterAttached<DataValidationErrors, Control, bool>("HasErrors");
        
        /// <summary>
Repository: AvaloniaUI/Avalonia
File: src/Avalonia.Base/Platform/Storage/SaveFilePickerResult.cs (C#)

Code before:
namespace Avalonia.Platform.Storage;

/// <summary>
/// Extended result of the <see cref="IStorageProvider.SaveFilePickerWithResultAsync(FilePickerSaveOptions)"/> operation.
/// </summary>
public readonly struct SaveFilePickerResult
{
    internal SaveFilePickerResult(IStorageFile? file)
    {
        File = file;
    }

    /// <summary>
    /// Saved <see cref="IStorageFile"/> or null if user canceled the dialog.
    /// </summary>
    public IStorageFile? File { get; }

    /// <summary>
    /// Selected file type or null if not supported.
    /// </summary>
    public FilePickerFileType? SelectedFileType { get; init; }
}


Review comment (suggestion):
The original API specified `File { get; init; }`. Internal constructors on public structs don't prevent the user from constructing a default instance so it's a bit odd that they can't construct one with a value.

More generally, I don't think we gain anything here by preventing users from creating such simple data types. Especially since it's read-only. This can of course be discussed!

Code after:
namespace Avalonia.Platform.Storage;

/// <summary>
/// Extended result of the <see cref="IStorageProvider.SaveFilePickerWithResultAsync(FilePickerSaveOptions)"/> operation.
/// </summary>
public readonly struct SaveFilePickerResult
{
    internal SaveFilePickerResult(IStorageFile? file)
    {
        File = file;
    }

    /// <summary>
    /// Saved <see cref="IStorageFile"/> or null if user canceled the dialog.
    /// </summary>
    public IStorageFile? File { get; init; }

    /// <summary>
    /// Selected file type or null if not supported.
    /// </summary>
    public FilePickerFileType? SelectedFileType { get; init; }
}
Repository: AvaloniaUI/Avalonia
File: src/Avalonia.Dialogs/Internal/ManagedFileChooserFilterViewModel.cs (C#)

Code before:
using System.Linq;
using System.Text.RegularExpressions;
using Avalonia.Platform.Storage;

namespace Avalonia.Dialogs.Internal
{
    public class ManagedFileChooserFilterViewModel : AvaloniaDialogsInternalViewModelBase
    {
        private readonly Regex[]? _patterns;
        public string Name { get; }
        internal int Index { get; }

        public ManagedFileChooserFilterViewModel(FilePickerFileType filter, int index)
        {
            Index = index;
            Name = filter.Name;

            if (filter.Patterns?.Contains("*.*") == true)
            {
                return;
            }

             _patterns = filter.Patterns?
                           .Select(e => new Regex("^" + Regex.Escape(e).Replace("\\*", ".*") + "$", RegexOptions.Singleline | RegexOptions.IgnoreCase))
                           .ToArray();
        }

        public bool Match(string filename)
        {
            return _patterns == null || _patterns.Any(ext => ext.IsMatch(filename));
        }

        public override string ToString() => Name;
    }
}


Review comment (suggestion):
I know that this VM is supposed to be internal, but contrary to our interfaces marked as `NotClientImplementable`, this constructor was part of the public API, even if it shouldn't have.

We can revisit for v12, but if we want to backport this to v11, please revert this. Either add a new overload or just keep the property internal with an `init` setter.


Code after:
using System.Linq;
using System.Text.RegularExpressions;
using Avalonia.Platform.Storage;

namespace Avalonia.Dialogs.Internal
{
    public class ManagedFileChooserFilterViewModel : AvaloniaDialogsInternalViewModelBase
    {
        private readonly Regex[]? _patterns;
        public string Name { get; }
        internal int Index { get; }

        public ManagedFileChooserFilterViewModel(FilePickerFileType filter) : this(filter, 0)
        {
        }
        
        public ManagedFileChooserFilterViewModel(FilePickerFileType filter, int index)
        {
            Index = index;
            Name = filter.Name;

            if (filter.Patterns?.Contains("*.*") == true)
            {
                return;
            }

             _patterns = filter.Patterns?
                           .Select(e => new Regex("^" + Regex.Escape(e).Replace("\\*", ".*") + "$", RegexOptions.Singleline | RegexOptions.IgnoreCase))
                           .ToArray();
        }

        public bool Match(string filename)
        {
            return _patterns == null || _patterns.Any(ext => ext.IsMatch(filename));
        }

        public override string ToString() => Name;
    }
}
Repository: AvaloniaUI/Avalonia
File: src/Avalonia.Base/Rendering/Composition/Drawing/ServerCompositionSimplePen.cs (C#)

Code before:
using Avalonia.Media;

namespace Avalonia.Rendering.Composition.Server;

internal partial class ServerCompositionSimplePen : IPen
{
    IDashStyle? IPen.DashStyle => DashStyle;

    /// <inheritdoc/>
    public override void Dispose()
    {
        // Dispose of the brush resource, this will remove the pen from the brush observers.
        // This was causing the pen to be retained in memory by long lived brush resources (e.g. those defined in
        // the theme or app resources), hence was causing memory leaks; see Issue #16451
        SetValue(ref _brush, null!);
        base.Dispose();
    }
}


Review comment (suggestion):
The property change will cause the pen to be queued for invalidation, which isn't needed since the object is being disposed. I think a better fix would be to call `RemoveObserversFromProperty(ref _brush)`.

Code after:
using Avalonia.Media;

namespace Avalonia.Rendering.Composition.Server;

internal partial class ServerCompositionSimplePen : IPen
{
    IDashStyle? IPen.DashStyle => DashStyle;

    /// <inheritdoc/>
    public override void Dispose()
    {
        // Remove the pen from the brush observers.
        // Without this, the pen was being retained in memory by long lived brush resources (e.g. those defined in
        // the theme or app resources), hence was causing memory leaks; see Issue #16451
        RemoveObserversFromProperty(ref _brush);
        _brush = null;
        base.Dispose();
    }
}
Repository: AvaloniaUI/Avalonia
File: src/Skia/Avalonia.Skia/Gpu/OpenGl/GlSkiaExternalObjectsFeature.cs (C#)

Code before:
    public IBitmapImpl SnapshotWithAutomaticSync()
    {
        using (_gpu.EnsureCurrent())
            return TakeSnapshot();
    }

    private int CopyToNewTexture(int textureType, int sourceTextureId, int internalFormat, int width, int height)
    {
        var gl = _gpu.GlContext.GlInterface;

        using var _ = _gpu.EnsureCurrent();

        if (_fbo == 0)
            _fbo = gl.GenFramebuffer();

        // Bind source texture
        gl.BindFramebuffer(GL_FRAMEBUFFER, _fbo);
        gl.FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, textureType, sourceTextureId, 0);

        // Create destination texture
        var destTextureId = gl.GenTexture();
        gl.BindTexture(textureType, destTextureId);
        gl.TexImage2D(textureType, 0, internalFormat, width, height, 0, GL_RGBA, GL_UNSIGNED_BYTE, IntPtr.Zero);

        // Copy
        gl.BindFramebuffer(GL_READ_FRAMEBUFFER, _fbo);
        gl.CopyTexSubImage2D(textureType, 0, 0, 0, 0, 0, width, height);

        gl.Flush();

        return destTextureId;
    }
}


Review comment (suggestion):
Note that `GL_READ_FRAMEBUFFER` is not Available with OpenGLES 2.0. Using `GL_FRAMEBUFFER` is safe since it affects both read and write framebuffers.

Code after:
        var signal = (GlSkiaImportedSemaphore)signalSemaphore;
        using (_gpu.EnsureCurrent())
        {
            wait.Semaphore.WaitTimelineSemaphore(_image, waitForValue);
            try
            {
                return TakeSnapshot();
            }
            finally
            {
                signal.Semaphore.SignalTimelineSemaphore(_image, signalValue);
            }
        }
    }

    public IBitmapImpl SnapshotWithAutomaticSync()
    {
        using (_gpu.EnsureCurrent())
            return TakeSnapshot();
    }

    private int CopyToNewTexture(int textureType, int sourceTextureId, int internalFormat, int width, int height)
    {
        var gl = _gpu.GlContext.GlInterface;

        using var _ = _gpu.EnsureCurrent();

        // Snapshot current values
        gl.GetIntegerv(GL_FRAMEBUFFER_BINDING, out var oldFbo);
        gl.GetIntegerv(GL_SCISSOR_TEST, out var oldScissorTest);

        // Bind source texture
        gl.BindFramebuffer(GL_FRAMEBUFFER, _fboProvider.Fbo);
        gl.Disable(GL_SCISSOR_TEST);
        gl.FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, textureType, sourceTextureId, 0);

        // Create destination texture
        var destTextureId = gl.GenTexture();
        gl.BindTexture(textureType, destTextureId);
        gl.TexImage2D(textureType, 0, internalFormat, width, height, 0, GL_RGBA, GL_UNSIGNED_BYTE, IntPtr.Zero);

        // Copy
        gl.CopyTexSubImage2D(textureType, 0, 0, 0, 0, 0, width, height);

        // Flush
        gl.FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, textureType, 0, 0);
        gl.Flush();

        // Restore old values
        gl.BindFramebuffer(GL_FRAMEBUFFER, oldFbo);
        if (oldScissorTest != 0)
Repository: AvaloniaUI/Avalonia
File: native/Avalonia.Native/src/OSX/menu.mm (Objective-C++)

Code before:
            [s_appMenuItem setSubmenu:[NSMenu new]];
        }
    }
    else
    {
        s_appMenuItem = nullptr;
    }
}

extern void SetServicesMenu (IAvnMenu* menu)
{
    auto nativeMenu = dynamic_cast<AvnAppMenu*>(menu);
    [NSApplication sharedApplication].servicesMenu = nativeMenu->GetNative();
}

extern IAvnMenu* GetAppMenu ()
{
    return s_appMenu;
}

extern NSMenuItem* GetAppMenuItem ()
{
    return s_appMenuItem;
}

static IAvnMenu* s_dockMenu = nullptr;

extern void SetDockMenu(IAvnMenu* menu)
{
    if (s_dockMenu != nullptr)
        s_dockMenu->Release();
    s_dockMenu = menu;
    if (s_dockMenu != nullptr)
        s_dockMenu->AddRef();
}

extern IAvnMenu* GetDockMenu()
{
    return s_dockMenu;
}



Review comment (suggestion):
Actually, you can just save `NSMenu*` reference somewhere in `app.mm`, no need to keep a COM reference here.

Code after:
            [s_appMenuItem setSubmenu:[NSMenu new]];
        }
    }
    else
    {
        s_appMenuItem = nullptr;
    }
}

extern void SetServicesMenu (IAvnMenu* menu)
{
    auto nativeMenu = dynamic_cast<AvnAppMenu*>(menu);
    [NSApplication sharedApplication].servicesMenu = nativeMenu->GetNative();
}

extern IAvnMenu* GetAppMenu ()
{
    return s_appMenu;
}

extern NSMenuItem* GetAppMenuItem ()
{
    return s_appMenuItem;
}
Repository: AvaloniaUI/Avalonia
File: src/Avalonia.Native/IAvnMenu.cs (C#)

Code before:

        public void Opening()
        {
            _parent?.RaiseOpening();
        }

        public void Closed()
        {
            _parent?.RaiseClosed();
        }
    }

    partial interface IAvnMenu
    {
        void RaiseNeedsUpdate();
        void RaiseOpening();
        void RaiseClosed();
        void Deinitialise();
    }
}
namespace Avalonia.Native.Interop.Impl
{
    partial class __MicroComIAvnMenuProxy
    {
        private Action _queueReset = null!;
        private Action _updateIfNeeded = null!;
        private List<__MicroComIAvnMenuItemProxy> _menuItems = new List<__MicroComIAvnMenuItemProxy>();
        private Dictionary<NativeMenuItemBase, __MicroComIAvnMenuItemProxy> _menuItemLookup = new Dictionary<NativeMenuItemBase, __MicroComIAvnMenuItemProxy>();

        private void UpdateTitle(string? title)
        {
            if (OperatingSystemEx.IsMacOS())
            {
                // macOS does not process access key markers, so remove them.
                title = AccessText.RemoveAccessKeyMarker(title);
            }
            if (string.IsNullOrWhiteSpace(title))
            {
                title = "";
            }
            SetTitle(title);
        }

        public void RaiseNeedsUpdate()
        {
            (ManagedMenu as INativeMenuExporterEventsImplBridge).RaiseNeedsUpdate();

            _updateIfNeeded();
        }

        public void RaiseOpening()

Review comment (suggestion):
Since I got rid of the common interface, I changed the internal calls in IAvnMenu to Actions so they could be called by both Menu and DockMenu calls. If you feel this should be different I'm open to it.

Code after:

        public void Opening()
        {
            _parent?.RaiseOpening();
        }

        public void Closed()
        {
            _parent?.RaiseClosed();
        }
    }

    partial interface IAvnMenu
    {
        void RaiseNeedsUpdate();
        void RaiseOpening();
        void RaiseClosed();
        void Deinitialise();
    }
}
namespace Avalonia.Native.Interop.Impl
{
    partial class __MicroComIAvnMenuProxy
    {
        private AvaloniaNativeMenuExporter _exporter;
        private List<__MicroComIAvnMenuItemProxy> _menuItems = new List<__MicroComIAvnMenuItemProxy>();
        private Dictionary<NativeMenuItemBase, __MicroComIAvnMenuItemProxy> _menuItemLookup = new Dictionary<NativeMenuItemBase, __MicroComIAvnMenuItemProxy>();

        private void UpdateTitle(string? title)
        {
            if (OperatingSystemEx.IsMacOS())
            {
                // macOS does not process access key markers, so remove them.
                title = AccessText.RemoveAccessKeyMarker(title);
            }
            if (string.IsNullOrWhiteSpace(title))
            {
                title = "";
            }
            SetTitle(title);
        }

        public void RaiseNeedsUpdate()
        {
            (ManagedMenu as INativeMenuExporterEventsImplBridge).RaiseNeedsUpdate();

            _exporter.UpdateIfNeeded();
        }

        public void RaiseOpening()
        {
Repository: Azure/azure-rest-api-specs
File: .github/shared/test/tag.test.js (JavaScript)

Code before:

import { resolve } from "path";
import { Tag } from "../src/tag.js";

describe("Tag", () => {
  it("can be created with mock swaggers", async () => {
    const tag = new Tag("tag", ["swagger"]);

    expect(tag.name).toBe("tag");
    expect(tag.readme).toBeUndefined();

    expect(tag.inputFiles.size).toBe(1);
    const swagger = [...tag.inputFiles.values()][0];
    expect(swagger.path).toBe(resolve("swagger"));

    await expect(swagger.getRefs()).rejects.toThrowError(/Failed to read file for swagger/i);
  });

  it("sorts input files in toJSONAsync", async () => {
    const tag = new Tag("test-tag", ["b.json", "a.json"]);
    const json = /** @type {import('../src/tag.js').TagJSON} */ (await tag.toJSONAsync());
    const files = json.inputFiles.map(
      (f) => /** @type {import('../src/swagger.js').SwaggerJSON} */ (f).path,
    );
    expect(files[0].localeCompare(files[1])).toBeLessThan(0);
  });
});


Review comment (suggestion):
```suggestion
    expect(files.length).toBe(2);
    expect(files[0].localeCompare(files[1])).toBeLessThan(0);
```

Code after:

import { resolve } from "path";
import { Tag } from "../src/tag.js";

describe("Tag", () => {
  it("can be created with mock swaggers", async () => {
    const tag = new Tag("tag", ["swagger"]);

    expect(tag.name).toBe("tag");
    expect(tag.readme).toBeUndefined();

    expect(tag.inputFiles.size).toBe(1);
    const swagger = [...tag.inputFiles.values()][0];
    expect(swagger.path).toBe(resolve("swagger"));

    await expect(swagger.getRefs()).rejects.toThrowError(/Failed to read file for swagger/i);
  });

  it("sorts input files in toJSONAsync", async () => {
    const tag = new Tag("test-tag", ["b.json", "a.json"]);
    const json = /** @type {import('../src/tag.js').TagJSON} */ (await tag.toJSONAsync());
    const files = json.inputFiles.map(
      (f) => /** @type {import('../src/swagger.js').SwaggerJSON} */ (f).path,
    );
    expect(files.length).toBe(2);
    expect(files[0].localeCompare(files[1])).toBeLessThan(0);
  });
});