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

Fix LT-20509: Can't delete words with undefined phonemes #166

Open
wants to merge 4 commits into
base: release/9.1
Choose a base branch
from

Conversation

jtmaxwell3
Copy link
Collaborator

@jtmaxwell3 jtmaxwell3 commented Sep 23, 2024

I have a fix for this, but I'm not sure it is the best solution.

Whenever a word with an undefined phoneme is parsed, a problem report is created for the wordform and added to the language's annotations. This doesn't seem to be used anywhere, but it may be used by clients of FieldWorks. The problem report prevents the user from deleting the wordform, even if the wordform doesn't appear anywhere else. I added code to delete problem reports before trying to decide whether a wordform can be deleted. An alternative is to not create problem reports in the first place. However, this only prevents future problems. It doesn't get rid of existing problem reports.


This change is Reviewable

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

I'm still thinking on this one, I don't like the idea of CanDelete having this kind of side effect. It is unexpected. I would rather have it detect that the only blocking situation is the annotations and then return true, and have the delete remove annotations before doing the rest of the delete.

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved

@jtmaxwell3
Copy link
Collaborator Author

I was also unhappy with this solution, but the code for detecting whether an object can be deleted is deep in liblcm and I couldn't figure it out. There is no way for a user to delete the problem reports, so it has to be done automatically. I would have preferred the deletion to occur before CanDelete, but the caller is in a generic class and so it didn't seem appropriate to put it there.

An alternative is to not create problem reports in the first place, and to delete existing problem reports when projects are read in. But I worry that someone outside of FieldWorks is depending on them.

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.

3 participants