Skip to content

Add Drag and Drop For Environment Variables #40105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Darkace01
Copy link

Summary of the Pull Request

This PR introduces drag-and-drop functionality for environment variables, allowing users to easily update the order of variables based on their preferences. Users can now rearrange variables directly in the UI by dragging and dropping, making it more intuitive and efficient to customise the order without manual editing.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Updated EnvironmentVariablesMainPage.xaml to use a ListView instead of an ItemsControl, enabling item dragging and reordering. The new ListView features a grid layout with a FontIcon, TextBox, and Button, while maintaining visibility control through data binding.

Added EditVariableValuesList_DragItemsCompleted method in EnvironmentVariablesMainPage.xaml.cs to update the text box with the new order of items after drag-and-drop operations, ensuring consistency with the underlying data model.

Validation Steps Performed

  • Manually tested the affected functionality.
  • Change was minimal and did not impact existing automated tests.
  • Verified that the new/modified feature works as expected and did not cause regressions in related areas.

Updated `EnvironmentVariablesMainPage.xaml` to use a `ListView` instead of an `ItemsControl`, enabling item dragging and reordering. The new `ListView` features a grid layout with a `FontIcon`, `TextBox`, and `Button`, while maintaining visibility control through data binding.

Added `EditVariableValuesList_DragItemsCompleted` method in `EnvironmentVariablesMainPage.xaml.cs` to update the text box with the new order of items after drag-and-drop operations, ensuring consistency with the underlying data model.
@Darkace01
Copy link
Author

@microsoft-github-policy-service agree

@yeelam-gordon yeelam-gordon requested a review from Copilot June 19, 2025 00:27
@yeelam-gordon yeelam-gordon added the Product-Environment Variables Refers to Environment Variables PowerToys label Jun 19, 2025
Copilot

This comment was marked as outdated.

Replaced hardcoded string separator `";"` with a constant `ValueListSeparator` in `EnvironmentVariablesMainPage.xaml.cs`. This change improves maintainability by centralizing the separator definition, making it easier to modify in the future. All instances of the separator in `string.Join` have been updated accordingly.
@Darkace01 Darkace01 requested a review from Copilot June 25, 2025 08:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request adds drag-and-drop support to the environment variables UI, enabling users to rearrange variables more intuitively. Key changes include:

  • Introducing a constant (ValueListSeparator) and updating join calls in EnvironmentVariablesMainPage.xaml.cs.
  • Replacing an ItemsControl with a ListView in EnvironmentVariablesMainPage.xaml to support dragging and reordering.
  • Adding the EditVariableValuesList_DragItemsCompleted event handler to update the UI upon reordering.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml.cs Updated join logic with a constant and added a new drag completion event handler.
src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml Replaced ItemsControl with a ListView enabling drag-and-drop functionality.

Copy link

@dcq01 dcq01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I'm a grad student working on a research project about using large language models to automate code review. Based on your commit a0e65cb and the changes in src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml.cs, my tool generated this comment:

  1. Null Handling: Ensure that variable and variable.ValuesList are not null before attempting to join the values. If either is null, handle this case explicitly.

    if (variable == null || variable.ValuesList == null) {
        // Handle the null case appropriately
        return;
    }
  2. Potential for IndexOutOfRange: Implement additional checks to ensure that the index is valid before attempting to move items in ReorderButtonDown_Click and ReorderButtonUp_Click.

  3. Event Unsubscription and Subscription: Use a try-finally block to ensure that the event handler is always reattached after setting the text of EditVariableDialogValueTxtBox.

    EditVariableDialogValueTxtBox.TextChanged -= EditVariableDialogValueTxtBox_TextChanged;
    try {
        EditVariableDialogValueTxtBox.Text = newValues;
    } finally {
        EditVariableDialogValueTxtBox.TextChanged += EditVariableDialogValueTxtBox_TextChanged;
    }
  4. Consistency of Separator: Ensure that the ValueListSeparator is consistently used throughout the codebase wherever the values are joined. If there are other parts of the code that still use the hardcoded ";" separator, it could lead to inconsistencies in how values are represented.

  5. TextBox Event Handling: Ensure that the event EditVariableDialogValueTxtBox.TextChanged is not being triggered unnecessarily. If the new value is the same as the current value, it might be better to avoid setting the text and modifying the event handlers.

  6. Potential Performance Impact: If ValuesList contains a large number of items, consider using string.Join directly on the Select without converting to an array to avoid performance implications.
    Change:

    var newValues = string.Join(ValueListSeparator, variable.ValuesList?.Select(x => x.Text).ToArray());

    to:

    var newValues = string.Join(ValueListSeparator, variable.ValuesList?.Select(x => x.Text));
  7. Code Duplication: The code for creating newValues is duplicated across multiple methods. Consider refactoring this into a separate method to adhere to the DRY (Don't Repeat Yourself) principle.

    private void UpdateValuesTextBox(Variable variable)
    {
        var newValues = string.Join(ValueListSeparator, variable.ValuesList?.Select(x => x.Text));
        EditVariableDialogValueTxtBox.TextChanged -= EditVariableDialogValueTxtBox_TextChanged;
        EditVariableDialogValueTxtBox.Text = newValues;
        EditVariableDialogValueTxtBox.TextChanged += EditVariableDialogValueTxtBox_TextChanged;
    }
  8. User Experience: When inserting a new ValuesListItem with an empty string, consider how this affects the user experience. If the user is not expecting an empty entry, it might be better to prompt them or handle it differently.

  9. Unit Tests for ValueListSeparator: Add unit tests to verify the behavior of the code when using the ValueListSeparator constant. Ensure it is correctly used in all relevant methods.

As part of my research, I'm trying to understand how useful these comments are in real-world development. If you have a moment, I'd be super grateful if you could quickly reply to these two yes/no questions:

  1. Does this comment provide suggestions from a dimension you hadn’t considered?
    1. Do you find this comment helpful?

Thanks a lot for your time and feedback! And sorry again if this message is a bother.

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Jul 10, 2025
@Darkace01 Darkace01 requested a review from dcq01 July 28, 2025 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer. Product-Environment Variables Refers to Environment Variables PowerToys
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants