Skip to content

Conversation

@jonahgraham
Copy link
Contributor

@jonahgraham jonahgraham commented Mar 7, 2023

In the case that two content types match a given input, the depth was being used to select between them. This was done to make sure more specific types for related types were selected as appropriate.

The way content types are selected is by first sorting all the candidate types, and then choosing the first one.

This change makes the sort only consider the depth of the content type if the content types have a descendent-ancestor relationship. If they are not in the same lineage, the depth is not a criteria for sorting the content types.

New content type tests have been added to cover the case from #351, specifically, conflict4 matches the conflict for *.hpp files between the tm4e and cdt plug-ins.

The comment update on conflict2 test and the new conflict2a test should probably have been done when Bug 86915 was completed because in 9cbda7e the order of content types was changed, but the comment was not changed, and it wasn't apparent to me if there was an explicit test that covered the conflict case of described content types.

Fixes #151

@mickaelistria
Copy link
Contributor

I think the priority should not matter when 1 contnet-type is a super/sub-type of the other: in such case, the most specialized content-type needs to be returned as it encompass the higher priority one as well.
I would recommend that we write good tests in ContentTypeManagerTest to ensure we first agree on the expectations.

@merks
Copy link
Contributor

merks commented Mar 8, 2023

@jonahgraham Maybe you have a concrete case/example where the old algorithm produces a result different than the one you would like?

@mickaelistria
Copy link
Contributor

mickaelistria commented Mar 8, 2023

Yes, this comes from eclipse-tm4e/tm4e#437 (comment) and eclipse-tm4e/tm4e#499

@jonahgraham
Copy link
Contributor Author

I have created the issue in this repo now, see #351

@jonahgraham jonahgraham force-pushed the prefer_priority branch 2 times, most recently from 8343b2e to de4d741 Compare March 8, 2023 19:49
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

(copied from #351 (comment) )

As far as I can tell, depth is not documented as an important factor

Indeed depth is not an important factor per se, the important factor is to return "most specialized" content-type in a lineage, ie basicContentType(priority=high) being a parent of specializedContentType(priority=low), then querying a file which has both content-type should return specializedContentType independently of the priority. That is what enables the most value.
Depth was an implementation choice to achieve that, but it doesn't make sense to use it when the content-types we compare are not of the same lineage: basicContentType has 2 independent children specializedContentType1(priority=high) and specializedContentType2(priority=normal) and that one has a child surSpecializedContentType2(priority=normal), then the expectation is that specializedContentType1 is returned first because depth isn't an interesting criterion to compare specializedContentType1 and surSpecializedContentType2.

An implication of both is this trickier case:
basicContentType

  • specializedContentType1(priority=high)
    • surSpecializedContentType1(priority=low)
  • specializedContentType2(priority=normal)
    • surSpecializedContentType2(priority=normal)

Then we would expect that the best content-type here is surSpecializedContentType1. Basically, to fulfill both requirements the priority of a content-type is the max of its lineage.

@jonahgraham jonahgraham changed the title Sort content types by priority first Only consider depth of content types when they are related Apr 7, 2023
@jonahgraham
Copy link
Contributor Author

[...] should return specializedContentType independently of the priority.

This comment caused me lots of confusion as in practice that isn't what happens unless the content of the file is also analyzed, which seems mostly to only happen when dealing with binary or xml files in practice. I added a new test case, conflict2a and fixed a comment on conflict2 that covers this case. The best summary of the behaviour came when it was last written in Bug 86915 back in 2005.

An implication of both is this trickier case:

My PR does not handle this case.

Once the CI confirms the tests behave the same as on my machine I will mark this ready for review. I updated the description + summary of the PR to properly reflect the current state.

@jonahgraham jonahgraham marked this pull request as ready for review April 7, 2023 23:21
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

Test Results

     29 files  ±0       29 suites  ±0   37m 46s ⏱️ - 2m 31s
2 378 tests ±0  2 377 ✔️ +1  1 💤 ±0  0  - 1 
6 824 runs  ±0  6 821 ✔️ +1  3 💤 ±0  0  - 1 

Results for commit 3a3f0d4. ± Comparison against base commit a6d80dd.

♻️ This comment has been updated with latest results.

In the case that two content types match a given input, the depth
was being used to select between them. This was done top make sure
more specific types for related types were selected as appropriate.

The way content types are selected is by first sorting all the
candidate types, and then choosing the first one.

This change makes the sort only consider the depth of the content
type if the content types have a descendent-ancestor relationship.
If they are not in the same lineage, the depth is not a criteria
for sorting the content types.

New content type tests have been added to cover the case from eclipse-platform#351,
specifically, conflict4 matches the conflict for *.hpp files between
the tm4e and cdt plug-ins.

The comment update on conflict2 test and the new conflict2a test
should probably have been done when Bug 86915 was completed because
in 9cbda7e the order of content types
was changed, but the comment was not changed, and it wasn't apparent
to me if there was an explicit test that covered the conflict case
of described content types.

Fixes eclipse-platform#151
@mickaelistria mickaelistria merged commit e2e5a64 into eclipse-platform:master Apr 12, 2023
@mickaelistria
Copy link
Contributor

Thank you!

@jonahgraham jonahgraham deleted the prefer_priority branch April 15, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

By default hpp files are not opened with CDT editor anymore if tm4e language pack is installed

3 participants