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.

Sign up for GitHub

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

Jump to bottom

fix(image navigation): remove flickering, remove in between load images #1127

Merged
merged 3 commits into from
Jul 3, 2023
Merged

fix(image navigation): remove flickering, remove in between load images #1127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b45c4eb
fix(image navigation): unsubscribe for aborting all out of row images…
domsteinbach Jul 3, 2023
b8621f3
chore(comment): remove comment
domsteinbach Jul 3, 2023
70aa1b9
fix(incoming regions): revert incoming regions for compound navigation
domsteinbach Jul 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 45 additions & 30 deletions apps/dsp-app/src/app/workspace/resource/representation/still-image/still-image.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import {
} from '../../services/value-operation-event.service';
import { FileRepresentation } from '../file-representation';
import { RepresentationService } from '../representation.service';
import { Subscription } from 'rxjs';

/**
* represents a region resource.
Expand Down Expand Up @@ -135,6 +136,9 @@ export class StillImageComponent

@Output() loaded = new EventEmitter<boolean>();

imagesSub: Subscription;
fileInfoSub: Subscription;

loading = true;
failedToLoad = false;
originalFilename: string;
Expand Down Expand Up @@ -202,11 +206,13 @@ export class StillImageComponent
if (
changes['images'] &&
changes['images'].previousValue &&
changes['images'].currentValue &&
changes['images'].currentValue[0].fileValue.fileUrl !==
changes['images'].previousValue[0].fileValue.fileUrl
) {
this.loadImages();
}

if (this.currentTab === 'annotations') {
this.renderRegions();
}
Expand All @@ -227,19 +233,45 @@ export class StillImageComponent
this._viewer.destroy();
this._viewer = undefined;
}
if (this.imagesSub) {
this.imagesSub.unsubscribe();
}

if (this.fileInfoSub) {
this.fileInfoSub.unsubscribe();
}
}

private loadImages() {
this._rs
// closing, so no more loading of short in between images if turning
// multiple pages
this._viewer.close()
if (this.imagesSub) {
this.imagesSub.unsubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need state management for this component in the future. This is a hack at best, even though I understand its necessity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the whole component needs refactoring. The hack follows the "busieness logic" ...

}
this.imagesSub = this._rs
.getFileInfo(
this.images[0].fileValue.fileUrl,
this.images[0].fileValue.filename
)
.subscribe((res) => {
this.originalFilename = res['originalFilename'];
this._openImages();
this._unhighlightAllRegions();
});
).subscribe(
res => {
this.originalFilename = res['originalFilename'];
this._openImages();
this._unhighlightAllRegions();
},
error => {
this.failedToLoad = true;
// disable mouse navigation incl. zoom
this._viewer.setMouseNavEnabled(false);
// disable the navigator
this._viewer.navigator.element.style.display = 'none';
// disable the region draw mode
this.regionDrawMode = false;
// stop loading tiles
this._viewer.removeAllHandlers('open');
this.loading = false;
}
);
}

/**
Expand Down Expand Up @@ -437,7 +469,6 @@ export class StillImageComponent
updateRes.type = this.parentResource.type;
updateRes.property = Constants.HasStillImageFileValue;
updateRes.value = file;

this._dspApiConnection.v2.values
.updateValue(updateRes as UpdateResource<UpdateValue>)
.pipe(
Expand Down Expand Up @@ -768,28 +799,12 @@ export class StillImageComponent
this._viewer.addOnceHandler('open', (args) => {
// check if the current image exists
if (this.images[0].fileValue.fileUrl.includes(args.source['@id'])) {
this._rs.getFileInfo(args.source['@id'] + '/info.json')
.subscribe(
res => {
// enable mouse navigation incl. zoom
this._viewer.setMouseNavEnabled(true);
// enable the navigator
this._viewer.navigator.element.style.display = 'block';
this.loading = false;
},
error => {
this.failedToLoad = true;
// disable mouse navigation incl. zoom
this._viewer.setMouseNavEnabled(false);
// disable the navigator
this._viewer.navigator.element.style.display = 'none';
// disable the region draw mode
this.regionDrawMode = false;
// stop loading tiles
this._viewer.removeAllHandlers('open');
this.loading = false;
}
);
// enable mouse navigation incl. zoom
this._viewer.setMouseNavEnabled(true);
// enable the navigator
this._viewer.navigator.element.style.display = 'block';
this.loading = false;

}
});
this._viewer.open(tileSources);
Expand Down
36 changes: 30 additions & 6 deletions apps/dsp-app/src/app/workspace/resource/resource.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export class ResourceComponent implements OnChanges, OnDestroy {
// in case of incoming representations,
// this will be the currently selected (part-of main) resource
incomingResource: DspResource;
incomingResourceSub: Subscription;

// for the annotations e.g. regions in a still image representation
annotationResources: DspResource[];
Expand All @@ -104,6 +105,10 @@ export class ResourceComponent implements OnChanges, OnDestroy {
// --> TODO: will be expanded with | MovingImageRepresentation[] | AudioRepresentation[] etc.
representationsToDisplay: FileRepresentation[] = [];

stillImageRepresentationsForCompoundResourceSub: Subscription;

incomingRegionsSub: Subscription;

representationConstants = RepresentationConstants;

// in case of compound object,
Expand Down Expand Up @@ -207,6 +212,13 @@ export class ResourceComponent implements OnChanges, OnDestroy {
sub.unsubscribe()
);
}
if (this.stillImageRepresentationsForCompoundResourceSub) {
this.stillImageRepresentationsForCompoundResourceSub.unsubscribe();
}

if (this.incomingRegionsSub) {
this.incomingRegionsSub.unsubscribe();
}
}

// ------------------------------------------------------------------------
Expand Down Expand Up @@ -299,10 +311,12 @@ export class ResourceComponent implements OnChanges, OnDestroy {

this.representationsToDisplay =
this.collectRepresentationsAndAnnotations(resource);

if (!this.representationsToDisplay.length && !this.compoundPosition) {
// the resource could be a compound object
this._incomingService
if (this.stillImageRepresentationsForCompoundResourceSub) {
this.stillImageRepresentationsForCompoundResourceSub.unsubscribe()
}
this.stillImageRepresentationsForCompoundResourceSub = this._incomingService
.getStillImageRepresentationsForCompoundResource(
resource.res.id,
0,
Expand All @@ -325,6 +339,7 @@ export class ResourceComponent implements OnChanges, OnDestroy {
this._errorHandler.showMessage(error);
}
);

} else {
this.requestIncomingResources(resource);
}
Expand Down Expand Up @@ -396,7 +411,11 @@ export class ResourceComponent implements OnChanges, OnDestroy {
}

getIncomingResource(iri: string) {
this._dspApiConnection.v2.res.getResource(iri).subscribe(
if (this.incomingResourceSub) {
this.incomingResourceSub.unsubscribe();
}
this.incomingResourceSub =
this._dspApiConnection.v2.res.getResource(iri).subscribe(
(response: ReadResource) => {
const res = new DspResource(response);

Expand Down Expand Up @@ -526,7 +545,6 @@ export class ResourceComponent implements OnChanges, OnDestroy {
.properties[
Constants.HasStillImageFileValue
] as ReadStillImageFileValue[];

for (const img of fileValues) {
const regions: Region[] = [];

Expand Down Expand Up @@ -621,7 +639,10 @@ export class ResourceComponent implements OnChanges, OnDestroy {
}

// get all representations for compound resource of this offset sequence
this._incomingService
if (this.stillImageRepresentationsForCompoundResourceSub) {
this.stillImageRepresentationsForCompoundResourceSub.unsubscribe();
}
this.stillImageRepresentationsForCompoundResourceSub = this._incomingService
.getStillImageRepresentationsForCompoundResource(
this.resource.res.id,
offset
Expand Down Expand Up @@ -685,7 +706,10 @@ export class ResourceComponent implements OnChanges, OnDestroy {
* @param offset the offset to be used (needed for paging). First request uses an offset of 0.
*/
protected getIncomingRegions(resource: DspResource, offset: number): void {
this._incomingService
if (this.incomingRegionsSub) {
this.incomingRegionsSub.unsubscribe()
}
this.incomingRegionsSub = this._incomingService
.getIncomingRegions(resource.res.id, offset)
.subscribe(
(regions: ReadResourceSequence) => {
Expand Down