Skip to content
This repository was archived by the owner on Apr 18, 2024. It is now read-only.

Commit fc89cc1

Browse files
authored
fix: LEAP-443: Select only one annotation on init (#1640)
### Main change **Only one annotation is selected during task opening.** We have the concept of selecting annotation, when we not just assign it as current one, but also update data in tags, trigger external event, load annotation history, setup hotkeys, set initial values. Previosly it was required to select every annotation to do some extra work in regions/tags inside it because of some quirky legacy code. I fixed what I found so far and ran all possible tests — they all are green, so consider this change mostly safe. Benefits: - improved performance as only one annotation is selected, reducing number of calculations and renders - no excess network requests to retrieve annotation history for every of these annotations - more clean code - one more step to remove one of the oldest legacy concept of `states`: it's not used in init anymore Things changed: - `STORE_INIT_OK` global var added to catch tags that can't properly work with new system (edge cases we missed) - apparently we have only one unit test for tags — Ranker; it's securely mocked Concerns: - `states` in Video tag select only *Labels controls, but changed code is in `fixBrokenAnnotation` which will do nothing to Video anyway - in potential missed edge case app will just crash - we could potentially check that we are inside init process by some flag, right? ### Other changes - removed Predictions panel and related code like showAllPredictions - removed panels from App and down the stream — we don't use them anywhere (3rd party LSF can be affected, but not sure anyone using panels there) - converted `LabelStudio.js` to TypeScript - fixed bunch of annoying react warnings (non-html props, wrong values, missed refs) - started to reorganize our "envs" to get rid of them at some point; for now it's `getRoot()` removed from there - removed `SidebarPage` as it was useless, plus removed multi-tabs support from `SidebarTabs` as `panels` were not used; plus that's all only for the old interface which is about to be removed - removed `hydrated` as it's not used anymore after #1166 - changed order of annotations to original one without reversing; thus order will always be the same, fixing the issue when list was reversed again after task reload; visual order is defined in `AnnotationsCarousel` anyway, so no changes in UX - fixed annotations order in View All to be the same as in annotation tabs * Remove View All Predictions and anything related It's not used anymore for a while * Move LabelStudio to TS, improve a little - Type drafts for main LSF class - getRootElement moved from env - Fixed methods signatures - Fixed typo in method name Most of the imports have TS errors because they are not typed; let it be like this for now. * Simplify SidebarTabs and remove unused panels param `panels` were not used anywhere, and this is part of old interface, that will be removed soon. + fix global Htx typing + fix conditions in App * Remove `hydrated`, we don't need it after #1166 * Fix some annoying react warnings * Don't select every annotation on init Also don't call any unneccessary methods for render. Just init regions. * Add flags to test correct behavior of init * Fix Relation init * fix our fixBrokenAnnotation() method Don't use states(), we already have simple toNames * Some other small fixes React warnings, simplier code * Fix merged results - they can also use toNames This is needed only for Video rectangles * Temporary fix for unit tests We don't have a real init, so overpass it * Fix history reinit for some cases Should fix failed e2e with TimeSeries, Audio and Video * Fix annotation selected by default: should be 1st one Annotations added to the start of the list, so the last one added is the first one in the list and is selected by default * Remove unused Annotation#update model prop * Add feature flag to cover annotation select * Fix FF name * Add a description for this FF * Add doc with lot of investigations * Tiny fix for panel classnames no extra whitespaces * Fix from #1665 * Fix typos in LSF.init.md * Trigger LS build * Fix typo to trigger LS build again * Move simpleInit to volatile This allows to check FF indirectly in other apps * Add more info about TopBar * Add couple of deprecation messages * Stop reversing annotations list Reversing the list caused problems when task is reloaded and list is reversed again. The visual order is defined in AnnotationsCarousel, so technical order is not that important. View All used technical order which can cause discomfort when you see different order in different places. So that was also fixed. Everything is behind the same FF, without it the order is an old one, reversed. * Move annotations ordering to utilities; sort imports * Fix annotation selected after View All --------- Co-authored-by: hlomzik <[email protected]>
1 parent 48dee3c commit fc89cc1

File tree

25 files changed

+513
-361
lines changed

25 files changed

+513
-361
lines changed

LSF.init.md

+204
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
# LSF init
2+
3+
Different thoughts and investingations related to LSF init.
4+
5+
## App render
6+
7+
We have 3 UI versions: old (awful), medium (outliner v1), modern (draggable panels)
8+
9+
Flags:
10+
11+
- `ff_front_1170_outliner_030222_short` — initial **FF_1170**
12+
- `fflag_feat_front_dev_3873_labeling_ui_improvements_short` — modern **FF_3873**
13+
14+
Components:
15+
16+
- modern (FF_1170 + FF_3873): `SideTabsPanels` + `SidePanels/TabPanels` + `OutlinerTree` + `BottomBar`
17+
- medium (FF_1170): `SidePanels` + `OutlinerTree`
18+
- old: `SidebarTabs` + `AnnotationTab` + `Entities/RegionTree` and surprisingly `BottomBar` if FF_3873 enabled without FF_1170
19+
20+
They all have `TopBar` with 2 different layouts:
21+
- modern (FF_3873): annotations tab by `AnnotationsCarousel` + custom actions
22+
- medium and old: annotations dropdown by `TopBar/Annotations` + `TopBar/Actions`
23+
24+
## configureStore, environments and init
25+
26+
configureStore() has the idea of environment, which comes from development/production run
27+
and contains some very important but not irreplaceable things:
28+
- rootElement() (yes, that's a function!) is redundant
29+
- getData() should be embedded
30+
- getExample() is definitely too much — example should be set from outside
31+
- configureApplication() just passes through a lot of methods with default values which are just a placeholders
32+
33+
## selected annotations
34+
35+
All annotations should've been always selected on init, most probably because of some weird issues in the past.
36+
Don't think we need it now. That could be pretty heavy operation. And it's something we should always think about.
37+
38+
## `AppStore#initializeStore`
39+
40+
Main flow: LabelStudio -> constructor -> createApp -> configureStore -> initializeStore (+src/Component does the similar)
41+
42+
Also it's called at these places right after `assignTask()`:
43+
- AnnotationPreview (?)
44+
- Config/Preview
45+
- ReviewPage -> setTask
46+
- lsf-sdk -> setLSFTask
47+
48+
## `hydrated`
49+
50+
Should prevent some types of objects from requesting resources too early because the whole LSF will be recreated.
51+
52+
It's default to `false` in AppStore;
53+
but set to `true` by default in configureStore if value is not reassigned.
54+
Can only be changed on init if Audio3 FF is on.
55+
56+
For some reason it's set to `false` in lsf-sdk constructor, but `true` in ReviewPage's setTask.
57+
But is set to `true` in lsf-sdk setLSFTask.
58+
59+
Apparently it was not used anymore after https://github.com/HumanSignal/label-studio-frontend/pull/1166
60+
61+
## `userGenerate` and `sentUserGenerate`
62+
63+
wtf?? lot of logic without comments
64+
65+
There is `Annotation#exists` getter to check that annotation was saved and can be updated.
66+
67+
`userGenerate` is used to mark annotations that were added by user in current session or is still a draft. Is set to true when:
68+
69+
- annotation created by user in `createAnnotation()`
70+
- annotation created from prediction in `addAnnotationFromPrediction()`
71+
- draft is loaded without annotation in `setAnnotation()` in lsf-sdk
72+
- is set by default to true, so it’s worth to check other places; but it defaults to false in `createItem()`
73+
74+
Can be submitted in current session, but this flag will remain the same.
75+
76+
## `setAnnotation()`
77+
78+
called from 3 places:
79+
80+
- `setLSFTask()`
81+
- `onStorageInitialized()` — called from `AppStore#initializeStore()`, one of the core LSF methods; if there is a task and that’s not a Label Stream it selects annotation based on `initialAnnotation` and `lastAnnotation`
82+
- `onDeleteAnnotation()`
83+
84+
## Huge mess with "side panels"
85+
86+
We have three versions of interface:
87+
1. with DEV-1170 FF off we have old interface with `components/SidebarTabs`
88+
2. with DEV-1170 FF on and DEV-3873 off we have intermediate interface with `components/SidePanels/SidePanels.tsx`
89+
3. with both FFs on we have new desired interface with `components/SidePanels/TabPanels/SideTabsPanels.tsx`
90+
91+
They both are only disabled in couple places.
92+
93+
Also the whole `App.panels` prop is only used in the first, the oldest version.
94+
95+
Apparently, `panels` were used only to add Comments tab and even this tab was moved into AnnotationTab after
96+
https://github.com/HumanSignal/label-studio-frontend/pull/774. So they are removed along with anything related.
97+
98+
## `assignTask`
99+
100+
101+
102+
## `initializeStore`
103+
104+
`afterReset()` — refill StaredStorage Stores in `extender.js`, but looks like it's not used
105+
106+
initRoot — config
107+
108+
for every prediction: addPrediction, selectPrediction, deserializeResults
109+
110+
for every annotation: addAnnotation, selectAnnotation, deserializeResults, reinitHistory (!!)
111+
112+
setInitialValues for last annotation
113+
114+
setHistory to set annotation history — some messed up thing in general
115+
116+
set initialized = true and invoke storageInitialized
117+
118+
119+
### problematic places during store init
120+
121+
They might require annotation to be selected.
122+
123+
ToolManagerMixin.afterAttach() — does lot of things with tools, selecting, unselecting, assigning
124+
125+
RelationStore.afterAttach() — was referring to selected annotation; fixed
126+
127+
Annotation.afterAttach() calls super helpful method annotationAttached(), but it's only implemented in Pairwise
128+
129+
130+
## What could happen on `deserializeResults()`?
131+
132+
prepareAnnotation -> fixBrokenAnnotation -> some changes on result json
133+
134+
deserializeSingleResult for every result in JSON — some area/result/state manipulations,
135+
should be no side effects.
136+
it calls `updateAppearenceFromState` for merged labels and results (only Video tag regions),
137+
but this should only be called on selected annotation.
138+
139+
cleanClassificationAreas — WUT???
140+
141+
updateFromResult for every global classification — a bit redundant unless we select annotation;
142+
but there is `hidden` param already, used only on Annotation History deserialization.
143+
144+
deserializeRelation for every relation
145+
146+
147+
# Streams
148+
149+
- one-way data flow
150+
- describe all possible external things that can affect current view
151+
152+
# Still left to fix after https://github.com/HumanSignal/label-studio-frontend/pull/1640
153+
154+
- Annotation History should be updated with correct annotation when we select it from View All; why not to do the same on usual annotation select? and if we do why do we need special case here?
155+
156+
- **Experiment**: use dev addons from outside via additional entry point for webpack. And how to distinguish this from e2e runs? Additional param?
157+
158+
- `description` option should be renamed to `instructions`
159+
160+
161+
# Other improvements
162+
163+
164+
https://github.com/HumanSignal/label-studio-frontend/blob/d0dd212bf7b12cb7f4469c9c2aa32dfe166f5abe/src/stores/RegionStore.js#L181-L183
165+
could be shifted towards self.selection.selectRegions(regions) and then
166+
https://github.com/HumanSignal/label-studio-frontend/blob/d0dd212bf7b12cb7f4469c9c2aa32dfe166f5abe/src/stores/RegionStore.js#L45-L47
167+
can be changed to usual method called only once.
168+
169+
170+
171+
172+
# Current changes
173+
174+
### Main change
175+
176+
**Only one annotation is selected during task opening.**
177+
178+
We have the concept of selecting annotation, when we not just assign it as current one, but also update data in tags, trigger external event, load annotation history, setup hotkeys, set initial values.
179+
180+
Previosly it was required to select every annotation to do some extra work in regions/tags inside it because of some quirky legacy code. I fixed what I found so far and ran all possible tests — they all are green, so consider this change mostly safe.
181+
182+
Benefits:
183+
- improved performance as only one annotation is selected, reducing number of calculations and renders
184+
- no excess network requests to retrieve annotation history for every of these annotations
185+
- more clean code
186+
- one more step to remove one of the oldest legacy concept of `states`: it's not used in init anymore
187+
188+
Things changed:
189+
- `STORE_INIT_OK` global var added to catch tags that can't properly work with new system (edge cases we missed)
190+
- apparently we have only one unit test for tags — Ranker; it's securely mocked
191+
192+
Concerns:
193+
- `states` in Video tag select only *Labels controls, but changed code is in `fixBrokenAnnotation` which will do nothing to Video anyway
194+
- in potential missed edge case app will just crash
195+
- we could potentially check that we are inside init process by some flag, right?
196+
197+
### Other changes
198+
- removed Predictions panel and related code like showAllPredictions
199+
- removed panels from App and down the stream — we don't use them anywhere (3rd party LSF can be affected, but not sure anyone using panels there)
200+
- converted `LabelStudio.js` to TypeScript
201+
- fixed bunch of annoying react warnings (non-html props, wrong values, missed refs)
202+
- started to reorganize our "envs" to get rid of them at some point; for now it's `getRoot()` removed from there
203+
- removed `SidebarPage` as it was useless, plus removed multi-tabs support from `SidebarTabs` as `panels` were not used; plus that's all only for the old interface which is about to be removed
204+
- removed `hydrated` as it's not used anymore after https://github.com/HumanSignal/label-studio-frontend/pull/1166

src/Component.js

+1-5
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { Component } from 'react';
22
import App from './components/App/App';
33
import { configureStore } from './configureStore';
4-
import { registerPanels } from './registerPanels';
54

65
export class LabelStudio extends Component {
76
state = {
@@ -26,10 +25,7 @@ export class LabelStudio extends Component {
2625

2726
render() {
2827
return this.state.initialized ? (
29-
<App
30-
store={this.store}
31-
panels={registerPanels(this.props.panels) ?? []}
32-
/>
28+
<App store={this.store} />
3329
) : null;
3430
}
3531
}

src/LabelStudio.js renamed to src/LabelStudio.tsx

+67-29
Original file line numberDiff line numberDiff line change
@@ -1,55 +1,98 @@
1+
import { configure } from 'mobx';
2+
import { destroy } from 'mobx-state-tree';
13
import { render, unmountComponentAtNode } from 'react-dom';
4+
import { toCamelCase } from 'strman';
5+
6+
import { LabelStudio as LabelStudioReact } from './Component';
27
import App from './components/App/App';
38
import { configureStore } from './configureStore';
4-
import { LabelStudio as LabelStudioReact } from './Component';
5-
import { registerPanels } from './registerPanels';
6-
import { configure } from 'mobx';
7-
import { EventInvoker } from './utils/events';
89
import legacyEvents from './core/External';
9-
import { toCamelCase } from 'strman';
10-
import { isDefined } from './utils/utilities';
1110
import { Hotkey } from './core/Hotkey';
1211
import defaultOptions from './defaultOptions';
13-
import { destroy } from 'mobx-state-tree';
1412
import { destroy as destroySharedStore } from './mixins/SharedChoiceStore/mixin';
15-
import { cleanDomAfterReact, findReactKey } from './utils/reactCleaner';
13+
import { EventInvoker } from './utils/events';
1614
import { FF_LSDV_4620_3_ML, isFF } from './utils/feature-flags';
15+
import { cleanDomAfterReact, findReactKey } from './utils/reactCleaner';
16+
import { isDefined } from './utils/utilities';
17+
18+
declare global {
19+
interface Window { Htx: any }
20+
}
1721

1822
configure({
1923
isolateGlobalState: true,
2024
});
2125

26+
type Callback = (...args: any[]) => any;
27+
28+
type LSFUser = any;
29+
type LSFTask = any;
30+
31+
// @todo type LSFOptions = SnapshotIn<typeof AppStore>;
32+
// because those options will go as initial values for AppStore
33+
// but it's not types yet, so here is some excerpt of its params
34+
type LSFOptions = Record<string, any> & {
35+
interfaces: string[],
36+
keymap: Keymap,
37+
user: LSFUser,
38+
users: LSFUser[],
39+
task: LSFTask,
40+
}
41+
2242
export class LabelStudio {
23-
static instances = new Set();
43+
static Component = LabelStudioReact;
44+
45+
static instances = new Set<LabelStudio>();
2446

2547
static destroyAll() {
2648
this.instances.forEach(inst => inst.destroy?.());
2749
this.instances.clear();
2850
}
2951

30-
constructor(root, userOptions = {}) {
31-
const options = Object.assign({}, defaultOptions, userOptions ?? {});
52+
options: Partial<LSFOptions>;
53+
root: Element | string;
54+
store: any;
55+
56+
destroy: (() => void) | null = null;
57+
events = new EventInvoker();
58+
59+
getRootElement(root: Element | string) {
60+
let element: Element | null = null;
61+
62+
if (typeof root === 'string') {
63+
element = document.getElementById(root);
64+
} else {
65+
element = root;
66+
}
67+
68+
if (!element) {
69+
throw new Error(`Root element not found (selector: ${root})`);
70+
}
71+
72+
return element;
73+
}
74+
75+
constructor(root: Element | string, userOptions: Partial<LSFOptions> = {}) {
76+
const options = { ...defaultOptions, ...userOptions };
3277

3378
if (options.keymap) {
3479
Hotkey.setKeymap(options.keymap);
3580
}
3681

3782
this.root = root;
38-
this.events = new EventInvoker();
39-
this.options = options ?? {};
40-
this.destroy = (() => { /* noop */ });
83+
this.options = options;
4184

42-
this.supportLgacyEvents(options);
85+
this.supportLegacyEvents();
4386
this.createApp();
4487

45-
this.constructor.instances.add(this);
88+
LabelStudio.instances.add(this);
4689
}
4790

48-
on(...args) {
49-
this.events.on(...args);
91+
on(eventName: string, callback: Callback) {
92+
this.events.on(eventName, callback);
5093
}
5194

52-
off(eventName, callback) {
95+
off(eventName: string, callback: Callback) {
5396
if (isDefined(callback)) {
5497
this.events.off(eventName, callback);
5598
} else {
@@ -58,8 +101,8 @@ export class LabelStudio {
58101
}
59102

60103
async createApp() {
61-
const { store, getRoot } = await configureStore(this.options, this.events);
62-
const rootElement = getRoot(this.root);
104+
const { store } = await configureStore(this.options, this.events);
105+
const rootElement = this.getRootElement(this.root);
63106

64107
this.store = store;
65108
window.Htx = this.store;
@@ -71,10 +114,7 @@ export class LabelStudio {
71114
clearRenderedApp();
72115
}
73116
render((
74-
<App
75-
store={this.store}
76-
panels={registerPanels(this.options.panels) ?? []}
77-
/>
117+
<App store={this.store} />
78118
), rootElement);
79119
};
80120

@@ -125,12 +165,12 @@ export class LabelStudio {
125165
*/
126166
this.store = null;
127167
this.destroy = null;
128-
this.constructor.instances.delete(this);
168+
LabelStudio.instances.delete(this);
129169
}
130170
};
131171
}
132172

133-
supportLgacyEvents() {
173+
supportLegacyEvents() {
134174
const keys = Object.keys(legacyEvents);
135175

136176
keys.forEach(key => {
@@ -144,5 +184,3 @@ export class LabelStudio {
144184
});
145185
}
146186
}
147-
148-
LabelStudio.Component = LabelStudioReact;

src/common/Pagination/Pagination.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export const Pagination: FC<PaginationProps> = forwardRef<any, PaginationProps>(
4848
pageSizeSelectable = true,
4949
hotkey,
5050
onChange,
51-
}) => {
51+
}, _ref) => {
5252
const [inputMode, setInputMode] = useState(false);
5353

5454
const handleChangeSelect = (e:ChangeEvent<HTMLSelectElement>) => {

0 commit comments

Comments
 (0)