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

Merged
merged 8 commits into from
Oct 8, 2024

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.

@jtmaxwell3
Copy link
Collaborator Author

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.

@jasonleenaylor
Copy link
Contributor

Src/FdoUi/WfiWordformUi.cs line 84 at r1 (raw file):

			// Delete problem annotations of the word form.
			// These aren't used within FieldWorks, and they block deletion.
			NonUndoableUnitOfWorkHelper.Do(m_cache.ActionHandlerAccessor, () =>

After you get remove this part of the old solution this looks ready to go.

@jtmaxwell3
Copy link
Collaborator Author

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.

@jasonleenaylor
Copy link
Contributor

Hmm. Let me look into it.

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.

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.
:lgtm_strong:

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)

@JakeOliver28 JakeOliver28 enabled auto-merge (squash) October 8, 2024 14:20
@JakeOliver28 JakeOliver28 merged commit 9126788 into release/9.1 Oct 8, 2024
5 checks passed
@JakeOliver28 JakeOliver28 deleted the LT-20509 branch October 8, 2024 14:33
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