-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
There was a problem hiding this 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
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. |
Per Jason, I added a utility for removing parser annotations code. If the user tries to delete a wordform with parser annotations, the UI tells them to run the utility. I also changed the parser to delete all annotations when it runs and to not create any new ones. Hopefully the user will never need to run the utility. |
After you get remove this part of the old solution this looks ready to go. |
I'm confused. The section that you quoted doesn't appear in my repository or in the diff of WfiWordformUi.cs in this pull request. It only appears in reviewable.io. I'm not sure what is going on. |
Hmm. Let me look into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it seems like it is a reviewable bug. Odd, when I stopped combining commits for review and instead looked at each separately reviewable showed the correct current changes to the file.
Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)
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