Skip to content
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

preferences: fix merge method in PreferenceProvider #12126

Merged

Conversation

AlexandraBuzila
Copy link
Contributor

What it does

When merging preferences, in the PreferenceProvider, also merge properties of type array. Currently, properties of type object are merged, but for arrays only the value from the target JSON is used.

This fixes the first use case described in issue #8987, where launch configs defined in the workspace are not visible, if other launch configs exist in a launch.json file.

How to test

  1. Create a workspace with a test folder and a .theia-workspace file, e.g.:
{
    "folders": [
        {
            "path": "test"
        }
    ],
    "settings": {
        "launch": {
            "configurations": [
                {
                    "name": "from .theia-workspace",
                    "request": "launch",
                    "type": "node",
                }
            ]
        }
    }
}
  1. Go to the Debug view and check that the launch configs from the workspace file are present in the dropdown at the top of the view.
  2. Add a .theia/launch.json file with at least one launch configuration, e.g.:
{
    "version": "0.2.0",
    "configurations": [
        {
            "name": "from .theia",
            "request": "launch",
            "type": "node",
        }
    ]
}
  1. Go to the Debug view and check that the launch configs from launch.json are visible in the dropdown, alongside the ones from the workspace file.

Review checklist

Reminder for reviewers

- when merging preferences, also merge properties of type array
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Code LGTM and the changes work as described.

@paul-marechal paul-marechal merged commit b28c8b9 into eclipse-theia:master Jan 31, 2023
@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 1, 2023

@AlexandraBuzila can we close the related issue? It didn't auto-close, it seems.

@AlexandraBuzila
Copy link
Contributor Author

I think the main problem that was reported in that ticket is solved by this PR, however there was a second issue mentioned in the ticket description: if there are launch configurations defined both in "someProject/.theia/launch.json" and "someProject/.vscode/launch.json", only the ones from the .theia folder are visible.

If the current behavior is expected and should not be changed, then the ticket can be closed from my point of view 👍

@AlexandraBuzila AlexandraBuzila deleted the merge-launch-configs branch February 1, 2023 13:53
@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 1, 2023

If we're not sure it's completely fixed, you did the right thing leaving it open, IMO. I was just wondering.

@paul-marechal paul-marechal added this to the 1.35.0 milestone Feb 1, 2023
@marcdumais-work
Copy link
Contributor

Hi,

I believe that this PR introduced a bug. I've been working to improve the browser test suite and when I rebased to include it, over 100 tests from launch-preferences.spec.js started failing. This test file is a bit obscure, but I think that with this PR here, we end-up with duplicated launch configurations.

Please see: #12153

@AlexandraBuzila
Copy link
Contributor Author

Hi @marcdumais-work! Thank you for catching this, I will have a look ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants