This repository has been archived by the owner on Apr 18, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 320
fix: LEAP-31: Fix functioning of auto annotations #1605
Merged
Merged
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
33a8020
fix: LEAP-31: Fix functioning of auto annotations
Gondragos 2b87238
Add tests
Gondragos 57a432e
Fix eslint warnings
Gondragos d1725f7
Merge branch 'master' into fb-leap-31/auto-annotation-fixes
Gondragos dca56f7
Update ls-test
Gondragos 8d73389
Remove Region#revokeSuggestion
Gondragos d26e636
Merge remote-tracking branch 'origin/master' into fb-leap-31/auto-ann…
Gondragos 20a555e
Update ls-test
Gondragos ce80ee9
Refactor
Gondragos 4f7313d
Update ls-test
Gondragos 7ffefd7
Add comment
Gondragos 2d940dd
Merge branch 'master' into fb-leap-31/auto-annotation-fixes
hlomzik a6e31e1
Update ls-test
Gondragos f05362c
fix deps
Gondragos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,12 +86,39 @@ const RegionsMixin = types | |
|
||
const result = regions.filter(region => { | ||
if (excludeSelf && region === self) return false; | ||
return region.dynamic && region.type === type && region.labelName === labelName; | ||
const canBePartOfNotification = self.supportSuggestions ? self.dynamic : true; | ||
|
||
return canBePartOfNotification | ||
&& region.type === type | ||
&& region.labelName === labelName | ||
&& region.results?.[0]?.to_name === self.results?.[0]?.to_name; | ||
}); | ||
|
||
return result; | ||
}, | ||
|
||
// Indicates that it is not temporary region created just to display data like Textarea's one | ||
// and is not a suggestion | ||
get isRealRegion() { | ||
return self.annotation?.areas?.has(self.id); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's actually smart. Love it. |
||
}, | ||
|
||
get shouldNotifyDrawingFinished() { | ||
// extra calls on destroying will be skipped | ||
// @see beforeDestroy action | ||
if (!self.isRealRegion) return false; | ||
if (self.annotation.isSuggestionsAccepting) return false; | ||
// There are two modes: | ||
// If object tag support suggestions - the region should be marked as a dynamic one to make notifications | ||
// If object tag doesn't support suggestions - every region works as dynamic with auto suggestions | ||
const canBeReasonOfNotification = self.supportSuggestions ? self.dynamic && !self.fromSuggestion : true; | ||
|
||
const isSmartEnabled = self.results.some(r => { | ||
return r.from_name.smartEnabled; | ||
}); | ||
Gondragos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return isSmartEnabled && canBeReasonOfNotification; | ||
}, | ||
})) | ||
.actions(self => { | ||
return { | ||
|
@@ -114,6 +141,19 @@ const RegionsMixin = types | |
}, | ||
|
||
beforeDestroy() { | ||
// beforeDestroy may be called by accident for Textarea and etc. as part of updateObjects action | ||
// in that case the region already has no results | ||
|
||
// The other bad behaviour is that beforeDestroy may be called on accepting suggestions 'cause they are deleting in that case | ||
|
||
// So if you see this bad thing during debugging - now you know why | ||
// and why we need this check | ||
if (self.isRealRegion) { | ||
return self.beforeDestroyArea(); | ||
} | ||
}, | ||
|
||
beforeDestroyArea() { | ||
self.notifyDrawingFinished({ destroy: true }); | ||
}, | ||
|
||
|
@@ -258,7 +298,7 @@ const RegionsMixin = types | |
} | ||
|
||
// everything below is related to dynamic preannotations | ||
if (!self.dynamic || self.fromSuggestion) return; | ||
if (!self.shouldNotifyDrawingFinished) return; | ||
|
||
clearTimeout(self.drawingTimeout); | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,10 +237,10 @@ export const Annotation = types | |
return dataExists && pkExists; | ||
}, | ||
|
||
get onlyTextObjects() { | ||
return self.objects.reduce((res, obj) => { | ||
return res && ['text', 'hypertext', 'paragraphs'].includes(obj.type); | ||
}, true); | ||
get hasSuggestionsSupport() { | ||
return self.objects.some((obj) => { | ||
return obj.supportSuggestions; | ||
}); | ||
}, | ||
|
||
isReadOnly() { | ||
|
@@ -253,6 +253,7 @@ export const Annotation = types | |
draftSelected: false, | ||
autosaveDelay: 5000, | ||
isDraftSaving: false, | ||
isSuggestionsAccepting: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you please add a comment describing this flag? from the name alone it's not very clear what it supposed to do |
||
submissionStarted: 0, | ||
versions: {}, | ||
resultSnapshot: '', | ||
|
@@ -1058,6 +1059,7 @@ export const Annotation = types | |
suggestions: true, | ||
}); | ||
|
||
self.isSuggestionsAccepting = true; | ||
if (getRoot(self).autoAcceptSuggestions) { | ||
if (isFF(FF_DEV_1284)) { | ||
self.history.setReplaceNextUndoState(true); | ||
|
@@ -1066,14 +1068,10 @@ export const Annotation = types | |
} else { | ||
self.suggestions.forEach((suggestion) => { | ||
// regions that can't be accepted in usual way, should be auto-accepted; | ||
// textarea will have simple classification area with no type, so check result. | ||
// @todo per-regions is tough thing here as they can be in generated result, | ||
// connected to manual region, will check it later | ||
const results = suggestion.results ?? []; | ||
const onlyAutoAccept = ['richtextregion', 'text', 'textrange'].includes(suggestion.type) | ||
|| results.findIndex(r => r.type === 'textarea') >= 0; | ||
|
||
if (onlyAutoAccept) { | ||
const supportSuggestions = suggestion.supportSuggestions; | ||
|
||
// If we cannot display suggestions on object/control then just accept them | ||
if (!supportSuggestions) { | ||
self.acceptSuggestion(suggestion.id); | ||
if (isFF(FF_DEV_1284)) { | ||
// This is necessary to prevent the occurrence of new steps in the history after updating objects at the end of current method | ||
|
@@ -1082,6 +1080,7 @@ export const Annotation = types | |
} | ||
}); | ||
} | ||
self.isSuggestionsAccepting = false; | ||
|
||
if (!isFF(FF_DEV_1284)) { | ||
history.freeze('richtext:suggestions'); | ||
|
@@ -1316,6 +1315,11 @@ export const Annotation = types | |
} | ||
} | ||
} else { | ||
// @todo: there is a strange behaviour that should be documented somewhere | ||
// On serialization we use area id as result id to save it somewhere | ||
// and on deserialization we use result id as area id | ||
// but when we use suggestions we should keep in mind that we need to do it manually or use serialized data instead | ||
// or we can get weird regions duplication in some cases | ||
const area = self.areas.get(item.cleanId); | ||
|
||
if (area) { | ||
|
@@ -1337,14 +1341,6 @@ export const Annotation = types | |
}); | ||
self.suggestions.delete(id); | ||
|
||
// hack to unlock sending textarea results | ||
// to the ML backen every time | ||
// it just sets `fromSuggestion` back to `false` | ||
const isTextArea = area.results.findIndex(r => r.type === 'textarea') >= 0; | ||
|
||
// This is temporary exception until we find the way to do it right | ||
// and this was done to keep notifications on prompt editing or fixing answer from ML backend | ||
if (isTextArea) area.revokeSuggestion(); | ||
Gondragos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
|
||
rejectSuggestion(id) { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It potentially compare
undefined === undefined
here. Could it lead to any unexpected issues?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've just checked that. It seems that the only way to get
undefined === undefined
is collecting connected regions in the case with config that contains two classification tags with the sameto_name
. I'm not sure if we need to group them but it's anyhow is better than it was.