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

Properly address property pages item ordering #7025

Open
MiYanni opened this issue Mar 15, 2021 · 7 comments
Open

Properly address property pages item ordering #7025

MiYanni opened this issue Mar 15, 2021 · 7 comments
Labels
Feature-Project-Properties-Designer The new project property pages which replace the legacy AppDesigner Triage-Investigate Reviewed and investigation needed by dev team
Milestone

Comments

@MiYanni
Copy link
Member

MiYanni commented Mar 15, 2021

Related: #6989

Currently, CPS does not seem to maintain item ordering. This can be seen in the Target Framework dropdown. For now, we are putting in a hotfix to manually order these items. However, the proper fix has a couple options:

  1. Investigate and fix CPS item ordering preservation
    a. For target framework, the items are declared here and this ordering should be preserved: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.SupportedTargetFrameworks.props
  2. Add functionality to the Rule definitions as to allow custom ordering.

These two options could both be completed, where the default is preserved ordering and it is optional to add your own ordering to the Rule definitions. When this PR is completed, please revert the hotfix here:

@tmeschter
Copy link
Contributor

A third option would be to update Microsoft.NET.SupportedTargetFrameworks.props to include explicit ordering information separate from the order of items in the .props.

@tmeschter
Copy link
Contributor

Fourth option: parse the Include for each item from that .props (e.g. .NETCoreApp,Version=v2.1) to group items by the framework family (e.g. .NETCoreApp) and then order by version number within that family.

@drewnoakes
Copy link
Member

I would vote for adding specific DisplayOrder metadata to the items returned by the SDK.

@jjmew jjmew added the Triage-Investigate Reviewed and investigation needed by dev team label Mar 16, 2021
@jjmew jjmew added this to the 16.10 milestone Mar 16, 2021
@jjmew jjmew added the Feature-Project-Properties-Designer The new project property pages which replace the legacy AppDesigner label Mar 16, 2021
@melytc
Copy link
Contributor

melytc commented Jun 9, 2021

Fourth option: parse the Include for each item from that .props (e.g. .NETCoreApp,Version=v2.1) to group items by the framework family (e.g. .NETCoreApp) and then order by version number within that family.

To avoid parsing, this is "already done" in the SDK side: instead of capturing all of the values in SupportedTargetFramework item, we could store and use each framework's ItemGroup Microsoft.NET.SupportedTargetFrameworks.props.

@jjmew jjmew modified the milestones: 16.10, 17.0 Jun 22, 2021
@MiYanni MiYanni modified the milestones: 17.0, Backlog Aug 24, 2021
@drewnoakes
Copy link
Member

CPS now has a way for us to obtain the original item ordering, which may fix this.

We'll need code such as:

var after = projectChangeDescription.After.Items as IDataWithOriginalSource<KeyValuePair<string, IImmutableDictionary<string, string>>>;

This gives an ICollection<KeyValuePair<string, IImmutableDictionary<string, string>>> which has the original MSBuild order.

There are three places we could do this in the current code. Search for https://github.com/dotnet/project-system/issues/7025 to find them.

@drewnoakes
Copy link
Member

There's a cut of this in the fix-7025-item-ordering branch on my fork. It's blocked on needing the IDataWithOriginalSource<> interface on evaluation data, not just build data. I've asked CPS the feasibility of adding this support in ProjectEvaluationSubscriptionService.

@drewnoakes
Copy link
Member

Evaluation data is handled differently to build data, and there's more re-use of the underlying immutable data structures over time. This means there's less likelihood we'd get ordered evaluation data using IDataWithOriginalSource<>. The approach should be fine for build data though.

@MiYanni MiYanni removed their assignment May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature-Project-Properties-Designer The new project property pages which replace the legacy AppDesigner Triage-Investigate Reviewed and investigation needed by dev team
Projects
None yet
Development

No branches or pull requests

5 participants