ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/inc/til/winrt.h (C/C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    template<typename T>
    struct property
    {
        explicit constexpr property(auto&&... args) :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const noexcept
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        explicit operator bool() const noexcept
        {
#ifdef WINRT_Windows_Foundation_H
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
#endif
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept

Review comment (suggestion):
Ah, I had it in my head that this was from `V` to `T`. But it's actually from `T` to `T`, so that's OK.

@PankajBhojwani what did you see when you tried to return `const T&`? That definitely should work...

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
    template<typename T>
    struct property
    {
        explicit constexpr property(auto&&... args) noexcept(std::is_nothrow_constructible_v<T, decltype(args)...>) :
            _value{ std::forward<decltype(args)>(args)... } {}

        property& operator=(const property& other) = default;

        T operator()() const noexcept(std::is_nothrow_copy_constructible<T>::value)
        {
            return _value;
        }
        void operator()(auto&& arg)
        {
            _value = std::forward<decltype(arg)>(arg);
        }
        explicit operator bool() const noexcept
        {
#ifdef WINRT_Windows_Foundation_H
            if constexpr (std::is_same_v<T, winrt::hstring>)
            {
                return !_value.empty();
            }
            else
#endif
            {
                return _value;
            }
        }
        bool operator==(const property& other) const noexcept
        {
            return _value == other._value;
        }
        bool operator!=(const property& other) const noexcept
Repository: microsoft/terminal
File: src/interactivity/win32/windowproc.cpp (C++)

Code before:
    case WM_GETOBJECT:
    {
        Status = _HandleGetObject(hWnd, wParam, lParam);
        break;
    }

    case WM_DESTROY:
    {
        // signal to uia that they can disconnect our uia provider
        if (_pUiaProvider)
        {
            UiaReturnRawElementProvider(hWnd, 0, 0, nullptr);
        }
        break;
    }

    case WM_SIZING:
    {
        goto CallDefWin;
        break;
    }

    case WM_GETDPISCALEDSIZE:
    {
        const LRESULT result = _HandleGetDpiScaledSize(
            static_cast<WORD>(wParam), static_cast<SIZE*>(lParam));
        UnlockConsole();
        return result;
    }

    case WM_DPICHANGED:
    {
        _fInDPIChange = true;
        ServiceLocator::LocateGlobals().dpi = HIWORD(wParam);
        _UpdateSystemMetrics();
        s_ReinitializeFontsForDPIChange();

        // This is the RECT that the system suggests.
        const auto prcNewScale = (RECT*)lParam;
        SetWindowPos(hWnd,
                     HWND_TOP,
                     prcNewScale->left,
                     prcNewScale->top,
                     RECT_WIDTH(prcNewScale),
                     RECT_HEIGHT(prcNewScale),
                     SWP_NOZORDER | SWP_NOACTIVATE);

        _fInDPIChange = false;

        break;
    }

Review comment (suggestion):
```suggestion
            static_cast<WORD>(wParam), reinterpret_cast<SIZE*>(lParam));
```

Code after:
    case WM_GETOBJECT:
    {
        Status = _HandleGetObject(hWnd, wParam, lParam);
        break;
    }

    case WM_DESTROY:
    {
        // signal to uia that they can disconnect our uia provider
        if (_pUiaProvider)
        {
            UiaReturnRawElementProvider(hWnd, 0, 0, nullptr);
        }
        break;
    }

    case WM_SIZING:
    {
        goto CallDefWin;
        break;
    }

    case WM_GETDPISCALEDSIZE:
    {
        const auto result = _HandleGetDpiScaledSize(
            static_cast<WORD>(wParam), reinterpret_cast<SIZE*>(lParam));
        UnlockConsole();
        return result;
    }

    case WM_DPICHANGED:
    {
        _fInDPIChange = true;
        ServiceLocator::LocateGlobals().dpi = HIWORD(wParam);
        _UpdateSystemMetrics();
        s_ReinitializeFontsForDPIChange();

        // This is the RECT that the system suggests.
        const auto prcNewScale = (RECT*)lParam;
        SetWindowPos(hWnd,
                     HWND_TOP,
                     prcNewScale->left,
                     prcNewScale->top,
                     RECT_WIDTH(prcNewScale),
                     RECT_HEIGHT(prcNewScale),
                     SWP_NOZORDER | SWP_NOACTIVATE);

        _fInDPIChange = false;

        break;
    }
Repository: microsoft/terminal
File: src/interactivity/win32/windowproc.cpp (C++)

Code before:

    case WM_GETOBJECT:
    {
        Status = _HandleGetObject(hWnd, wParam, lParam);
        break;
    }

    case WM_DESTROY:
    {
        // signal to uia that they can disconnect our uia provider
        if (_pUiaProvider)
        {
            UiaReturnRawElementProvider(hWnd, 0, 0, nullptr);
        }
        break;
    }

    case WM_SIZING:
    {
        goto CallDefWin;
        break;
    }

    case WM_GETDPISCALEDSIZE:
    {
        const LRESULT result = _HandleGetDpiScaledSize(
            static_cast<WORD>(wParam), static_cast<SIZE*>(lParam));
        UnlockConsole();
        return result;
    }

    case WM_DPICHANGED:
    {
        _fInDPIChange = true;
        ServiceLocator::LocateGlobals().dpi = HIWORD(wParam);
        _UpdateSystemMetrics();
        s_ReinitializeFontsForDPIChange();

        // This is the RECT that the system suggests.
        const auto prcNewScale = (RECT*)lParam;
        SetWindowPos(hWnd,
                     HWND_TOP,
                     prcNewScale->left,
                     prcNewScale->top,
                     RECT_WIDTH(prcNewScale),
                     RECT_HEIGHT(prcNewScale),
                     SWP_NOZORDER | SWP_NOACTIVATE);

        _fInDPIChange = false;

        break;

Review comment (suggestion):
```suggestion
        const auto result = _HandleGetDpiScaledSize(
```

Code after:

    case WM_GETOBJECT:
    {
        Status = _HandleGetObject(hWnd, wParam, lParam);
        break;
    }

    case WM_DESTROY:
    {
        // signal to uia that they can disconnect our uia provider
        if (_pUiaProvider)
        {
            UiaReturnRawElementProvider(hWnd, 0, 0, nullptr);
        }
        break;
    }

    case WM_SIZING:
    {
        goto CallDefWin;
        break;
    }

    case WM_GETDPISCALEDSIZE:
    {
        const auto result = _HandleGetDpiScaledSize(
            static_cast<WORD>(wParam), reinterpret_cast<SIZE*>(lParam));
        UnlockConsole();
        return result;
    }

    case WM_DPICHANGED:
    {
        _fInDPIChange = true;
        ServiceLocator::LocateGlobals().dpi = HIWORD(wParam);
        _UpdateSystemMetrics();
        s_ReinitializeFontsForDPIChange();

        // This is the RECT that the system suggests.
        const auto prcNewScale = (RECT*)lParam;
        SetWindowPos(hWnd,
                     HWND_TOP,
                     prcNewScale->left,
                     prcNewScale->top,
                     RECT_WIDTH(prcNewScale),
                     RECT_HEIGHT(prcNewScale),
                     SWP_NOZORDER | SWP_NOACTIVATE);

        _fInDPIChange = false;

        break;
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/EditAction.cpp (C++)

Code before:

    void EditAction::ShortcutActionBox_QuerySubmitted(const AutoSuggestBox& sender, const AutoSuggestBoxQuerySubmittedEventArgs& args)
    {
        const auto submittedText = args.QueryText();

        // Validate that this is a valid shortcut action
        bool isValid = false;
        for (const auto& action : _ViewModel.AvailableShortcutActions())
        {
            if (action == submittedText)
            {
                isValid = true;
                break;
            }
        }

        if (isValid)
        {
            _ViewModel.ProposedShortcutActionName(winrt::box_value(submittedText));
            _lastValidAction = submittedText;
        }
        else
        {
            // Revert to the last valid action
            sender.Text(_lastValidAction);
        }
    }

    void EditAction::ShortcutActionBox_LostFocus(const IInspectable& sender, const RoutedEventArgs&)
    {
        // The auto suggest box does a weird thing where it reverts to the last query text when you
        // keyboard navigate out of it. Intercept it here and keep the correct text.
        const auto box = sender.as<AutoSuggestBox>();
        const auto currentText = box.Text();

        if (currentText != _lastValidAction && !_lastValidAction.empty())
        {
            box.Text(_lastValidAction);
        }
    }
}


Review comment (suggestion):
You could rewrite this as...


```suggestion
                _ViewModel.ProposedShortcutActionName(winrt::box_value(submittedText));
                _lastValidAction = submittedText;
                return;
            }
        }

        // If we get here, we never found a match.
        // Revert to the last valid action
        sender.Text(_lastValidAction);
```

Code after:

    void EditAction::ShortcutActionBox_QuerySubmitted(const AutoSuggestBox& sender, const AutoSuggestBoxQuerySubmittedEventArgs& args)
    {
        const auto submittedText = args.QueryText();

        for (const auto& action : _ViewModel.AvailableShortcutActions())
        {
            if (action == submittedText)
            {
                _ViewModel.ProposedShortcutActionName(winrt::box_value(submittedText));
                _lastValidAction = submittedText;
                return;
            }
        }

        // If we get here, we never found a match.
        // Revert to the last valid action
        sender.Text(_lastValidAction);
    }

    void EditAction::ShortcutActionBox_LostFocus(const IInspectable& sender, const RoutedEventArgs&)
    {
        // The auto suggest box does a weird thing where it reverts to the last query text when you
        // keyboard navigate out of it. Intercept it here and keep the correct text.
        const auto box = sender.as<AutoSuggestBox>();
        const auto currentText = box.Text();

        if (currentText != _lastValidAction && !_lastValidAction.empty())
        {
            box.Text(_lastValidAction);
        }
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/Compatibility.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "Compatibility.h"
#include "EnumEntry.h"
#include "Compatibility.g.cpp"
#include "CompatibilityViewModel.g.cpp"
#include "NavigateToPageArgs.g.h"

using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    CompatibilityViewModel::CompatibilityViewModel(Model::CascadiaSettings settings) :
        _settings{ settings }
    {
        INITIALIZE_BINDABLE_ENUM_SETTING(TextMeasurement, TextMeasurement, winrt::Microsoft::Terminal::Control::TextMeasurement, L"Globals_TextMeasurement_", L"Text");
    }

    bool CompatibilityViewModel::DebugFeaturesAvailable() const noexcept
    {
        return Feature_DebugModeUI::IsEnabled();
    }

    void CompatibilityViewModel::ResetApplicationState()
    {
        TraceLoggingWrite(
            g_hTerminalSettingsEditorProvider,
            "ResetApplicationState",
            TraceLoggingDescription("Event emitted when the user resets their application state"),
            TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
            TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));

Review comment (suggestion):
we shouldn't need the `.g.h` guys here - but if we do, please move them ABOVE the cpp files.

in general, these should come in auto-included via the Microsoft.Terminal.Settings.Editor.h top-level _projection type_ `#include`

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "Compatibility.h"
#include "EnumEntry.h"
#include "Compatibility.g.cpp"
#include "CompatibilityViewModel.g.cpp"

using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    CompatibilityViewModel::CompatibilityViewModel(Model::CascadiaSettings settings) :
        _settings{ settings }
    {
        INITIALIZE_BINDABLE_ENUM_SETTING(TextMeasurement, TextMeasurement, winrt::Microsoft::Terminal::Control::TextMeasurement, L"Globals_TextMeasurement_", L"Text");
    }

    bool CompatibilityViewModel::DebugFeaturesAvailable() const noexcept
    {
        return Feature_DebugModeUI::IsEnabled();
    }

    void CompatibilityViewModel::ResetApplicationState()
    {
        TraceLoggingWrite(
            g_hTerminalSettingsEditorProvider,
            "ResetApplicationState",
            TraceLoggingDescription("Event emitted when the user resets their application state"),
            TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
            TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
Repository: microsoft/vscode
File: src/vs/platform/quickinput/browser/quickInputBox.ts (TypeScript)

Code before:
	private container: HTMLElement;
	private findInput: FindInput;

	constructor(
		private parent: HTMLElement,
		inputBoxStyles: IInputBoxStyles,
		toggleStyles: IToggleStyles
	) {
		super();
		this.container = dom.append(this.parent, $('.quick-input-box'));
		this.findInput = this._register(new FindInput(
			this.container,
			undefined,
			{
				label: '',
				inputBoxStyles,
				toggleStyles,
				actionViewItemProvider: createToggleActionViewItemProvider(toggleStyles)
			}));
		const input = this.findInput.inputBox.inputElement;
		input.role = 'textbox';
		input.ariaHasPopup = 'menu';
		input.ariaAutoComplete = 'list';

		// Hide tooltip when user starts typing to prevent obscuring dropdown results
		this._register(dom.addDisposableListener(input, dom.EventType.KEY_DOWN, () => {
			getBaseLayerHoverDelegate().hideHover();
		}));
	}

	get onKeyDown() {
		return this.findInput.onKeyDown;
	}

	get onMouseDown() {
		return this.findInput.onMouseDown;
	}

	onDidChange = (handler: (event: string) => void): IDisposable => {
		return this.findInput.onDidChange(handler);
	};

	get value() {
		return this.findInput.getValue();
	}

	set value(value: string) {
		this.findInput.setValue(value);
	}

	select(range: IRange | null = null): void {

Review comment (suggestion):
For clean code, this should be handled by the input that Quick Pick takes advantage of and be a configuration option on that object upon creation.

Code after:
	private findInput: FindInput;

	constructor(
		private parent: HTMLElement,
		inputBoxStyles: IInputBoxStyles,
		toggleStyles: IToggleStyles
	) {
		super();
		this.container = dom.append(this.parent, $('.quick-input-box'));
		this.findInput = this._register(new FindInput(
			this.container,
			undefined,
			{
				label: '',
				inputBoxStyles,
				toggleStyles,
				actionViewItemProvider: createToggleActionViewItemProvider(toggleStyles),
				hideHoverOnValueChange: true
			}));
		const input = this.findInput.inputBox.inputElement;
		input.role = 'textbox';
		input.ariaHasPopup = 'menu';
		input.ariaAutoComplete = 'list';
	}

	get onKeyDown() {
		return this.findInput.onKeyDown;
	}

	get onMouseDown() {
		return this.findInput.onMouseDown;
	}

	onDidChange = (handler: (event: string) => void): IDisposable => {
		return this.findInput.onDidChange(handler);
	};

	get value() {
		return this.findInput.getValue();
	}

	set value(value: string) {
		this.findInput.setValue(value);
	}

	select(range: IRange | null = null): void {
		this.findInput.inputBox.select(range);
	}

	getSelection(): IRange | null {
		return this.findInput.inputBox.getSelection();
Repository: microsoft/vscode
File: src/vs/workbench/contrib/chat/common/model/chatProgressTypes/chatElicitationRequestPart.ts (TypeScript)

Code before:
		reject?: () => Promise<ElicitationState>,
		public readonly source?: ToolDataSource,
		public readonly moreActions?: IAction[],
		public readonly onHide?: () => void,
	) {
		if (reject) {
			this.reject = async () => {
				const state = await reject!();
				this.state.set(state, undefined);
			};
		}
	}

	accept(value: IAction | true): Promise<void> {
		return this._accept(value).then(state => {
			this.state.set(state, undefined);
		});
	}

	hide(): void {
		if (this._isHiddenValue.get()) {
			return;
		}
		this._isHiddenValue.set(true, undefined, undefined);
		this.onHide?.();
		this.state.set(ElicitationState.Rejected, undefined);
	}

	public toJSON() {
		const state = this.state.get();

		return {
			kind: 'elicitationSerialized',
			title: this.title,
			message: this.message,
			state: state === ElicitationState.Pending ? ElicitationState.Rejected : state,
			acceptedResult: this.acceptedResult,
			subtitle: this.subtitle,
			source: this.source,
			isHidden: this._isHiddenValue.get(),
		} satisfies IChatElicitationRequestSerialized;
	}
}


Review comment (suggestion):
A hidden part is not necessarily rejected. An elicitaiton could be accepted and then hidden. This should only happen if the state was still pending

Code after:
		reject?: () => Promise<ElicitationState>,
		public readonly source?: ToolDataSource,
		public readonly moreActions?: IAction[],
		public readonly onHide?: () => void,
	) {
		if (reject) {
			this.reject = async () => {
				const state = await reject!();
				this.state.set(state, undefined);
			};
		}
	}

	accept(value: IAction | true): Promise<void> {
		return this._accept(value).then(state => {
			this.state.set(state, undefined);
		});
	}

	hide(): void {
		if (this._isHiddenValue.get()) {
			return;
		}
		this._isHiddenValue.set(true, undefined, undefined);
		this.onHide?.();
		if (this.state.get() === ElicitationState.Pending) {
			this.state.set(ElicitationState.Rejected, undefined);
		}
	}

	public toJSON() {
		const state = this.state.get();

		return {
			kind: 'elicitationSerialized',
			title: this.title,
			message: this.message,
			state: state === ElicitationState.Pending ? ElicitationState.Rejected : state,
			acceptedResult: this.acceptedResult,
			subtitle: this.subtitle,
			source: this.source,
			isHidden: this._isHiddenValue.get(),
		} satisfies IChatElicitationRequestSerialized;
	}
}
Repository: microsoft/vscode
File: src/vs/platform/terminal/common/capabilities/commandDetection/promptInputModel.ts (TypeScript)

Code before:
			return;
		}

		this._cursorIndex = -1;

		// Remove any ghost text from the input if it exists on execute
		if (this._ghostTextIndex !== -1) {
			this._value = this._value.substring(0, this._ghostTextIndex);
			this._ghostTextIndex = -1;
		}

		const event = this._createStateObject();
		if (this._lastUserInput === '\u0003') {
			this._lastUserInput = '';
			this._onDidInterrupt.fire(event);
		}

		this._state = PromptInputState.Execute;
		this._onDidFinishInput.fire(event);
		this._onDidChangeInput.fire(event);
	}

	private _handleCommandFinished() {
		// Clear the prompt input value when command finishes to prepare for the next command
		// This prevents runCommand from detecting leftover text and sending ^C unnecessarily
		this._value = '';
	}

	@throttle(0)
	private _sync() {
		try {
			this._doSync();
		} catch (e) {
			this._logService.error('Error while syncing prompt input model', e);
		}
	}

	private _doSync() {
		if (this._state !== PromptInputState.Input) {
			return;
		}

		let commandStartY = this._commandStartMarker?.line;
		if (commandStartY === undefined) {
			return;
		}

		const buffer = this._xterm.buffer.active;
		let line = buffer.getLine(commandStartY);
		const absoluteCursorY = buffer.baseY + buffer.cursorY;
		let cursorIndex: number | undefined;

Review comment (suggestion):
I'm not sure if it will be good idea to fire an empty `_onDidChangeInput` here, because we havent started the "new" input yet.


Code after:
			return;
		}

		this._cursorIndex = -1;

		// Remove any ghost text from the input if it exists on execute
		if (this._ghostTextIndex !== -1) {
			this._value = this._value.substring(0, this._ghostTextIndex);
			this._ghostTextIndex = -1;
		}

		const event = this._createStateObject();
		if (this._lastUserInput === '\u0003') {
			this._lastUserInput = '';
			this._onDidInterrupt.fire(event);
		}

		this._state = PromptInputState.Execute;
		this._onDidFinishInput.fire(event);
		this._onDidChangeInput.fire(event);
	}

	private _handleCommandFinished() {
		// Clear the prompt input value when command finishes to prepare for the next command
		// This prevents runCommand from detecting leftover text and sending ^C unnecessarily
		this._value = '';
		this._onDidChangeInput.fire(this._createStateObject());
	}

	@throttle(0)
	private _sync() {
		try {
			this._doSync();
		} catch (e) {
			this._logService.error('Error while syncing prompt input model', e);
		}
	}

	private _doSync() {
		if (this._state !== PromptInputState.Input) {
			return;
		}

		let commandStartY = this._commandStartMarker?.line;
		if (commandStartY === undefined) {
			return;
		}

		const buffer = this._xterm.buffer.active;
		let line = buffer.getLine(commandStartY);
		const absoluteCursorY = buffer.baseY + buffer.cursorY;