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: LEAP-828: Add handling for carriage return in domManager #5615

Merged
merged 11 commits into from
Apr 3, 2024
8 changes: 4 additions & 4 deletions web/dist/apps/labelstudio/version.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"message": "Update web/libs/datamanager/src/utils/urlJSON.ts",
"commit": "6fb43b0a8f7e717062e6d19221f359d945088729",
"date": "2024-04-01T21:27:15.000Z",
"branch": "fb-LEAP-846/virtual-date-filter"
"message": "Merge branch 'develop' into 'fb-leap-828/empty-text-fields'",
"commit": "81b2c70d08f7840f69f9df998d79edc122453c67",
"date": "2024-04-03T12:01:19.000Z",
"branch": "fb-leap-828/empty-text-fields"
}
8 changes: 4 additions & 4 deletions web/dist/libs/datamanager/version.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"message": "Update web/libs/datamanager/src/utils/urlJSON.ts",
"commit": "6fb43b0a8f7e717062e6d19221f359d945088729",
"date": "2024-04-01T21:27:15.000Z",
"branch": "fb-LEAP-846/virtual-date-filter"
"message": "Merge branch 'develop' into 'fb-leap-828/empty-text-fields'",
"commit": "81b2c70d08f7840f69f9df998d79edc122453c67",
"date": "2024-04-03T12:01:19.000Z",
"branch": "fb-leap-828/empty-text-fields"
}
2 changes: 1 addition & 1 deletion web/dist/libs/editor/main.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion web/dist/libs/editor/main.js.map

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions web/dist/libs/editor/version.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"message": "Update web/libs/datamanager/src/utils/urlJSON.ts",
"commit": "6fb43b0a8f7e717062e6d19221f359d945088729",
"date": "2024-04-01T21:27:15.000Z",
"branch": "fb-LEAP-846/virtual-date-filter"
"message": "Merge branch 'develop' into 'fb-leap-828/empty-text-fields'",
"commit": "81b2c70d08f7840f69f9df998d79edc122453c67",
"date": "2024-04-03T12:01:19.000Z",
"branch": "fb-leap-828/empty-text-fields"
}
15 changes: 15 additions & 0 deletions web/libs/editor/src/mixins/AreaMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,21 @@ export const AreaMixinBase = types
self.results.push(r);
},

/**
* Applies additional data from the given result.
* In the results we have almost all data meaningful stored in value but in regions we have two places for it:
* - region itself (fields in model)
* - related results (in results array)
* so for some fields we should control more if we want to apply fields that could be in both places into the region.
* This method also helps to avoid region type detection at the deserialization stage.
*
* @param {Object} result - The result object containing additional data.
* @returns {void}
*/
applyAdditionalDataFromResult(_result) {
// This method should be overridden if we need to get some additional data from result on deserialize
},

removeResult(r) {
const index = self.results.indexOf(r);

Expand Down
22 changes: 21 additions & 1 deletion web/libs/editor/src/regions/RichTextRegion.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,26 @@ const Model = types
}
},

/**
* Applies additional data from the given result.
* In the results we have almost all data meaningful stored in value but in regions we have two places for it:
* - region itself (fields in model)
* - related results (in results array)
* so for some fields we should control more if we want to apply fields that could be in both places into the region.
* This method also helps to avoid region type detection at the deserialization stage.
*
* @param {Object} result - The result object containing additional data.
* @returns {void}
*/
applyAdditionalDataFromResult(result) {
hlomzik marked this conversation as resolved.
Show resolved Hide resolved
const isMainResult = result?.type?.endsWith('labels');
const hasText = isDefined(result?.value?.text);

if (isMainResult && hasText) {
self.text = result.value.text;
}
},

serialize() {
const res = {
value: {},
Expand Down Expand Up @@ -187,7 +207,7 @@ const Model = types
if (isFF(FF_LSDV_4620_3)) {

// 1. first try to find range by xpath in the original layout

const offsets = self.parent.relativeOffsetsToGlobalOffsets(self.start, self.startOffset, self.end, self.endOffset);

if (offsets) {
Expand Down
107 changes: 83 additions & 24 deletions web/libs/editor/src/stores/Annotation/Annotation.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { destroy, detach, flow, getEnv, getParent, getRoot, isAlive, onSnapshot, types } from 'mobx-state-tree';

import throttle from 'lodash.throttle';
import {destroy, detach, flow, getEnv, getParent, getRoot, isAlive, onSnapshot, types} from 'mobx-state-tree';
import Constants from '../../core/Constants';
import { errorBuilder } from '../../core/DataValidator/ConfigValidator';
import { guidGenerator } from '../../core/Helpers';
import { Hotkey } from '../../core/Hotkey';
import {errorBuilder} from '../../core/DataValidator/ConfigValidator';
import {guidGenerator} from '../../core/Helpers';
import {Hotkey} from '../../core/Hotkey';
import TimeTraveller from '../../core/TimeTraveller';
import Tree, { TRAVERSE_STOP } from '../../core/Tree';
import Tree, {TRAVERSE_STOP} from '../../core/Tree';
import Types from '../../core/Types';
import Area from '../../regions/Area';
import Result from '../../regions/Result';
Expand All @@ -16,21 +15,84 @@ import {
FF_DEV_1598,
FF_DEV_2100,
FF_DEV_2432,
FF_DEV_3391, FF_LLM_EPIC,
FF_DEV_3391,
FF_LLM_EPIC,
FF_LSDV_3009,
FF_LSDV_4583,
FF_LSDV_4832,
FF_LSDV_4988,
isFF
} from '../../utils/feature-flags';
import { delay, isDefined } from '../../utils/utilities';
import { CommentStore } from '../Comment/CommentStore';
import {delay, isDefined} from '../../utils/utilities';
import {CommentStore} from '../Comment/CommentStore';
import RegionStore from '../RegionStore';
import RelationStore from '../RelationStore';
import { UserExtended } from '../UserStore';
import {UserExtended} from '../UserStore';

const hotkeys = Hotkey('Annotations', 'Annotations');

/**
* Omit value fields from the object.
*
* This should fix a problem with wrong region type detection caused by overlapping fields from a result and from an area.
* The current problem is related to the text field of richtext region that could appear in the result for the textarea as well.
* As these fields have the same name and different types it could make the region to be detected as a classification region instead of richtext,
* and we may miss its displaying.
*
* For now, it is mostly related to the rich text region with per region textareas.
* The problem may appear when we get wrong order of results for deserialization.
* The other reason may be the omitted main result (We declare that all the data that we need to restore the main region is also contained in the per-region results).
* @example Wrong order:
* [{
* "id": "id_1",
* "from_name": "comment",
* "to_name": "text",
* "type": "textarea",
* "value": {
* "start": 0,
* "end": 11,
* "text": ["A comment for the region],
* },
* },
* {
* "id": "id_1",
* "from_name": "labels",
* "to_name": "text",
* "type": "labels",
* "value": {
* "start": 0,
* "end": 11,
* "labels": ["Label 1"],
* "text": "Just a text",
* },
* }]
*
* @example Omitted main result:
* [{
* "id": "only_per_region_textarea",
* "from_name": "comment",
* "to_name": "text",
* "type": "textarea",
* "value": {
* "start": 0,
* "end": 11,
* "labels": ["Label 1"],
* "text": ["A comment for the region"],
* },
* }]
*
* @param value {Object} object to fix
* @returns {Object} new object without value fields
*/
function omitValueFields(value) {
const newValue = {...value};

Result.properties.value.propertyNames.forEach(propName => {
delete newValue[propName];
});
return newValue;
}

const TrackedState = types
.model('TrackedState', {
areas: types.map(Area),
Expand Down Expand Up @@ -1033,8 +1095,8 @@ export const Annotation = types
if (tagNames.has(obj.from_name) && tagNames.has(obj.to_name)) {
res.push(obj);
}
// Insert image dimensions from result

// Insert image dimensions from result
(() => {
if (!isDefined(obj.original_width)) return;
if (!tagNames.has(obj.to_name)) return;
Expand All @@ -1043,7 +1105,7 @@ export const Annotation = types

if (tag.type !== 'image') return;

const imageEntity = tag.findImageEntity(obj.item_index ?? 0);
const imageEntity = tag.findImageEntity(obj.item_index ?? 0);

if (!imageEntity) return;

Expand Down Expand Up @@ -1191,15 +1253,6 @@ export const Annotation = types
const areaId = `${id || guidGenerator()}#${self.id}`;
const resultId = `${data.from_name}@${areaId}`;
const value = self.prepareValue(rawValue, tagType);
// This should fix a problem when the order of results is broken
const omitValueFields = (value) => {
const newValue = { ...value };

Result.properties.value.propertyNames.forEach(propName => {
delete newValue[propName];
});
return newValue;
};

if (isFF(FF_DEV_3391)) {
to_name = `${to_name}@${self.id}`;
Expand All @@ -1214,6 +1267,8 @@ export const Annotation = types
object: to_name,
...data,
// We need to omit value properties due to there may be conflicting property types, for example a text.
// if we don't it can create a classification instead of proper area
/** @see `omitValueFields` */
hlomzik marked this conversation as resolved.
Show resolved Hide resolved
...omitValueFields(value),
value,
};
Expand All @@ -1231,7 +1286,11 @@ export const Annotation = types
}
}

area.addResult({ ...data, id: resultId, type, value, from_name, to_name });
const newResult = { ...data, id: resultId, type, value, from_name, to_name };

area.addResult(newResult);
// apply additional data, that were skipped in favour of region type detection
area.applyAdditionalDataFromResult?.(newResult);

// if there is merged result with region data and type and also with the labels
// and object allows such merge — create new result with these labels
Expand Down Expand Up @@ -1349,7 +1408,7 @@ export const Annotation = types
area.setValue(state);
});
self.suggestions.delete(id);

},

rejectSuggestion(id) {
Expand Down
17 changes: 15 additions & 2 deletions web/libs/editor/src/tags/object/RichText/domManager.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
import { flatten, isDefined } from '../../../utils/utilities';
import {flatten, isDefined} from '../../../utils/utilities';

// line feed
const LF = '\n';
// carriage return
const CF = '\r';

type DDExtraText = string;


/**
* Normalize text for displaying it.
* It replaces all line breaks with '\n' symbol.
* This is a variant used historically, but it converts \r\n to \n\n which might be not correct
* @todo check if we can convert \r\n to \n without getting any problems
*/
function normalizeText(text: string) {
return text.replace(/[\n\r]/g, '\\n');
}
Expand Down Expand Up @@ -366,7 +375,11 @@ class DomData {
let fromIdx = this.displayedTextPos;
const contentParts = [];

while (displayedText[fromIdx] === LF) {
// it should be just LF but in some OS / browsers it's CRLF (at least)
// `fromIdx` is only an inner counter that does not affect start/end offsets in results
// and the only problem here could be with normalizeText function
/** @see `normalizeText` */
while (displayedText[fromIdx] === LF || displayedText[fromIdx] === CF) {
fromIdx++;
}
let toIdx = fromIdx;
Expand Down
Loading