Repository navigation
LT-22822: Don't display autogenerated Natural Classes - #1198
Conversation
Feature-based natural classes with no name are user-defined, since auto-generated natural classes are always assigned a non-empty name.
* Hide auto-generated feature-based NCs from displaying in NCs list.
* Create a RecordFilter subclass ("ExcludeAutoGeneratedNaturalClassFilter")
to filter out auto-gen NCs, and use it to filter the NC list in
areaConfiguration.xml.
* Hide auto-generated feature-based NCs from the phonological rule NC
choosers, so they can't be assigned to other APRs or phonological rules.
* Update AffixRuleFormulaControl.SetMappingNaturalClass and the
NaturalClass case of RuleFormulaControl.m_insertionControl_Insert
to show only NCs that are not auto-generated.
Cover ExcludeAutoGeneratedNaturalClassFilter: it rejects auto-generated classes, passes named, unnamed and segment-based classes, and still works when it receives its cache only through the Cache setter, as a filter restored from persisted settings does. Move the name matching out of IsFeatureBasedNCUserDefined into an internal IsAutoGeneratedName(ncName, format) helper, so it can be tested against format strings other than the English one. The behavior of IsFeatureBasedNCUserDefined is unchanged. Add tests that a null class is not auto-generated, that a format starting with the rule name recognizes generated names without matching ordinary ones, and that a format with no rule name placeholder recognizes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment hygiene (advisory)1 comment-style violation(s) in the lines this branch adds since
Fix them per |
NUnit Tests 1 files ± 0 1 suites ±0 11m 37s ⏱️ + 3m 23s Results for commit 07ee72b. ± Comparison against base commit c6cd25f. This pull request removes 8 and adds 56 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1198 +/- ##
==========================================
+ Coverage 40.69% 40.71% +0.02%
==========================================
Files 1567 1569 +2
Lines 356898 357046 +148
Branches 41755 41783 +28
==========================================
+ Hits 145241 145382 +141
- Misses 182321 182322 +1
- Partials 29336 29342 +6
🚀 New features to boost your workflow:
|
|
Unaddressed bugs mentioned by Devin:
This is a known possible but rare edge-case issue that can't currently be handled. After we make a model change to add a flag to NCs to track whether they were generated and do away with the naming-based detection method, this issue will go away.
This is intentional and is actually fixing what was a bug in the code. An unnamed feature class is user defined, not auto-generated, so its feature-editing command should not be visible. |
If we use a default hidden filter to hide auto-generated NCs, there are a couple of code paths that will circumvent the filter and cause autogen NCs to re-display in the NCs list. Instead, we remove them at the source--using a dynamic loader for the NC record list. Update tests to match new behavior. For clarity, rename test helper method from AddRuleNamedFeatureNaturalClass to AddAutoGeneratedFeatureNaturalClass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| { | ||
| protected override IEnumerable<int> GetObjectSet() | ||
| { | ||
| return ExcludeAutoGenerated(base.GetObjectSet(), m_cache); |
There was a problem hiding this comment.
Once a rule stops using one of its auto-generated classes, I can't find a way for the user to delete it now. Nothing deletes the class when the context is removed from the rule, when the rule itself is deleted, or when Set Mapping Features clears an affix mapping back to a copy (AffixRuleFormulaControl.SetMappingFeatures removes the mapping and leaves its class in NaturalClassesOS). Until now the user could find these leftovers in Grammar > Natural Classes and delete them; this list now hides them. They still show in the environment natural-class chooser, so the user will see "Created automatically for rule …" entries there that they can't remove.
Should a class be deleted when its rule stops using it? Or should the list still show the leftovers?
There was a problem hiding this comment.
Yes, a class should be deleted when its rule stops using it. I am inclined to do this in a separate PR, unless you think it would prove to be a very small fix. As it is, my understanding is that users are generally not actually removing the leftover classes. Though it is possible for a user to find all classes in the Natural Classes list, there was not a clear indicator of which ones were or were not linked to classes (the rulename would vanish from the autogenerated classname when the rule was deleted, but this could also happen for other reasons not involving deletion of a rule). So the proliferation of generated classes that aren't referencing anything is already an issue we have.
Beth & Kevin have shared that users do not want to see or even be aware of the autogenerated Natural Classes, so showing the leftovers in the list isn't what we'll want to do.
There was a problem hiding this comment.
I've created Jira issue LT-22875 for deleting auto-generated NCs when they are no longer used by any rule.
| /// True for feature-based natural classes generated automatically for a rule. | ||
| /// Phoneme-based classes are never auto-generated. | ||
| /// </summary> | ||
| public static bool IsAutoGeneratedNaturalClass(IPhNaturalClass nc) |
There was a problem hiding this comment.
This decides from the name alone, so an auto-generated class the user has taken over is hidden too. Two cases already have tests:
- the user gave it an abbreviation (
RuleNamedClass_AbbreviatedC_DrawsFeature), so environments can use it as[C]; - the user added text in front of the generated name (
ClassNamedForARuleBehindAPrefix_IsNotUserDefined).
In both cases the class now drops out of the Natural Classes list and the rule choosers, while environments and other rules may still use it. The user can no longer find it to edit it. Is that intended?
There was a problem hiding this comment.
I think I should check with Jason & Ken about this. The intent was if we have an auto-generated class but the user has altered its name, it should be treated as user defined. So the first case you mentioned was intended to be treated as auto-generated still, but not the second. However, having some classes that started as auto-generated and now are treated as user defined doesn't seem like something we can keep once we've shifted to using a flag on NCs for auto-generated versus user defined natural classes.
We may want to go ahead and consider them user defined for now, but expect that to change and require some kind of one-time cleanup step after the model change when we begin using that flag. E.g. something like duplicating autogenerated classes when users have assigned a single autogenerated class to more than one rule, so that we end up with one class per auto-generated rule. (That said, Beth & Kevin seem to think this would be a rare occurrence.)
There was a problem hiding this comment.
After talking with Beth & Kevin, we have landed on the following criteria for a NC that was auto-generated to be considered user defined:
- Its name and/or its abbreviation has been edited by the user.
- The user has assigned it to more than one rule. (Even if they have not changed the name or abbreviation.)
In the second case, we also add a "Shared: " prefix to the auto-generated name before displaying the NC list. If a shared NC is removed from some rules so that it is once again used by at most one rule, the "Shared: " prefix is removed.
Also, if a generated NC has been assigned by a user to more than one rule, it should be considered user defined. Add method to check if a NC is used by more than one rule. Update tests.
Auto-generated NCs are considered user defined if they are shared by multiple rules. In that case add a "Shared: " prefix to the generated class name. Remove the prefix & consider the NC auto-generated if at any point it is not longer shared. Add a NaturalClassRecordClerk that ensures prefixes accurately reflect whether or not an NC is shared before displaying the NC list. Add SyncSharedPrefix(s) methods to RuleFormulaControl to check whether or not a shared prefix is needed and add or remove it from the class name as necessary.
Since the new localizable shared prefix does not yet have translations for the interface languages, it's especially important to make sure the prefix and generated class name are recognized in either the interface language or in English. Also, update one test and a few comments.
* Test that NCs with added abbreviations are considered user defined. * Test that NCs shared by more than one rule are user defined. * Test that NCs with a shared name prefix but that are used by at most one rule are not user defined. * Test that the shared prefixes on all NCs are synced correctly before displaying the NCs list. * Fix comments & update "label" keyword to "prefix" for consistency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Don't return after finding an owning rule for a NC. Its possible there are still referrers related to other rules.
|
Unaddressed bugs mentioned by Devin:
This is intentional. Generated NCs shared by more than one rule are now considered user defined, so they appear in formulas using their abbreviations, but they don't have abbreviations, so the placeholder *** is shown. |
Ensure auto-generated NCs are not displayed in the Natural Classes list.
Normally an auto-generated NC is any Natural Class whose name follows
the auto-generated naming format. We also have the following criteria for
a NC that was auto-generated to be considered user defined:
(Even if they have not changed the name or abbreviation.)
In the second case, we also add a "Shared: " prefix to the auto-generated
name before displaying the NC list. If a shared NC is removed from some
rules so that it is once again used by at most one rule,
the "Shared: " prefix is removed.
Add English as a fallback language for the auto-generated
name format and for the generated "Shared: " prefix format.
translation for the localizable "Shared: " prefix and later a
translation is added.
Fix IsFeatureBasedNCUserDefined logic
instead of auto-generated. Auto-generated feature-based
natural classes are always assigned a non-empty name.
abbreviation added by the user.
(i.e. by more than one rule).
Add methods to sync a prefix on the name of NCs shared
by more than one rule. (SyncSharedPrefix(es) in RuleFormulaControl)
prefixes on shared NCs are all up to date.
(See NaturalClassRecordClerk)
Exclude auto-generated feature-based NCs from the NCs record list.
which excludes auto-generated NCs, and use it to dynamically
load the NC list in areaConfiguration.xml.
Hide auto-generated feature-based NCs from the phonological rule NC
choosers, so they can't be assigned to other APRs or phonological rules.
NaturalClass case of RuleFormulaControl.m_insertionControl_Insert
to show only NCs that are not auto-generated.
Note: auto-generated feature-based NCs still appear in environments.
After we have made a model change to add a flag to NCs to identify
auto-generated vs user-defined, we will be able to hide auto-generated
NCs in environment choosers as well.
This change is