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

fix: Fix a bug that caused some unassigned face photos to be unselectable #774

Merged
merged 1 commit into from
Aug 19, 2023
Merged

fix: Fix a bug that caused some unassigned face photos to be unselectable #774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 47 additions & 40 deletions src/components/SelectionManager.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@ import AlbumsIcon from 'vue-material-design-icons/ImageAlbum.vue';
import AlbumRemoveIcon from 'vue-material-design-icons/BookRemove.vue';
import FolderMoveIcon from 'vue-material-design-icons/FolderMove.vue';

import { IDay, IHeadRow, IPhoto, IRow, IRowType, ISelectionAction } from '../types';

type Selection = Map<number, IPhoto>;
import { IDay, IHeadRow, IPhoto, IRow, IRowType, ISelectionAction, SelectionProxy } from '../types';

/**
* The distance for which the touch selection is clamped.
Expand Down Expand Up @@ -124,14 +122,14 @@ export default defineComponent({
data: () => ({
show: false,
size: 0,
selection: new Map<number, IPhoto>(),
selection: new SelectionProxy(),
defaultActions: null! as ISelectionAction[],

touchAnchor: null as IPhoto | null,
prevTouch: null as Touch | null,
touchTimer: 0,
touchMoved: false,
touchPrevSel: null as Selection | null,
touchPrevSel: null as SelectionProxy | null,
prevOver: null as IPhoto | null,
touchScrollInterval: 0,
touchScrollDelta: 0,
Expand Down Expand Up @@ -246,8 +244,17 @@ export default defineComponent({
this.$emit('delete', photos);
},

deleteSelectedPhotosById(delIds: number[], selection: Selection) {
return this.deletePhotos(delIds.map((id) => selection.get(id)).filter((p): p is IPhoto => p !== undefined));
deleteSelectedPhotosById(delIds: number[], selection: SelectionProxy) {
let photosToDelete: IPhoto[] = []
for (const photo of selection.values()) {
if (photo === undefined) {
continue
}
if (delIds.indexOf(photo.fileid) > -1) {
photosToDelete.push(photo)
}
}
Comment on lines +248 to +256
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Why not just selection.values().filter( ... )?
  2. Can get a safer time complexity by making delIds a Set first

return this.deletePhotos(photosToDelete);
},

updateLoading(delta: number) {
Expand Down Expand Up @@ -286,11 +293,11 @@ export default defineComponent({
},

/** Is this fileid (or anything if not specified) selected */
has(fileid?: number) {
if (fileid === undefined) {
has(photo?: IPhoto) {
if (photo === undefined) {
return this.selection.size > 0;
}
return this.selection.has(fileid);
return this.selection.has(photo);
},

/** Get the actions list */
Expand Down Expand Up @@ -342,7 +349,7 @@ export default defineComponent({
this.touchAnchor = photo;
this.prevOver = photo;
this.prevTouch = event.touches[0];
this.touchPrevSel = new Map(this.selection);
this.touchPrevSel = this.selection.copy();
this.touchMoved = false;
this.touchTimer = window.setTimeout(() => {
if (this.touchAnchor === photo) {
Expand Down Expand Up @@ -499,7 +506,7 @@ export default defineComponent({
reverse = overPhoto.dayid > this.touchAnchor.dayid != this.isreverse;
}

const newSelection = new Map(this.touchPrevSel);
const newSelection = this.touchPrevSel!.copy();
const updatedDays = new Set<number>();

// Walk over rows
Expand All @@ -524,7 +531,7 @@ export default defineComponent({
if (!p) break; // shouldn't happen, ever

// This is there now
newSelection.set(p.fileid, p);
newSelection.add(p);

// Perf: only update heads if not selected
if (!(p.flag & this.c.FLAG_SELECTED)) {
Expand All @@ -541,10 +548,10 @@ export default defineComponent({
}

// Remove unselected
for (const [fileid, p] of this.selection) {
if (!newSelection.has(fileid)) {
this.selectPhoto(p, false, true);
updatedDays.add(p.dayid);
for (const photo of this.selection.values()) {
if (!newSelection.has(photo)) {
this.selectPhoto(photo, false, true);
updatedDays.add(photo.dayid);
}
}

Expand All @@ -563,19 +570,19 @@ export default defineComponent({
return; // ignore placeholders
}

const nval = val ?? !this.selection.has(photo.fileid);
const nval = val ?? !this.selection.has(photo);
if (nval) {
photo.flag |= this.c.FLAG_SELECTED;
this.selection.set(photo.fileid, photo);
this.selection.add(photo);
this.selectionChanged();
} else {
photo.flag &= ~this.c.FLAG_SELECTED;

// Only do this if the photo in the selection set is this one.
// The problem arises when there are duplicates (e.g. face rect)
// in the list, which creates an inconsistent state if we do this.
if (this.selection.get(photo.fileid) === photo) {
this.selection.delete(photo.fileid);
if (this.selection.get(photo) === photo) {
Comment on lines 581 to +584
Copy link
Owner

Choose a reason for hiding this comment

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

This comment and check should no longer be relevant.

this.selection.delete(photo);
this.selectionChanged();
}
}
Expand Down Expand Up @@ -679,7 +686,7 @@ export default defineComponent({
Array.from(toClear).forEach((photo: IPhoto) => {
photo.flag &= ~this.c.FLAG_SELECTED;
heads.add(this.heads[photo.dayid]);
this.selection.delete(photo.fileid);
this.selection.delete(photo);
this.selectionChanged();
});
heads.forEach(this.updateHeadSelected);
Expand All @@ -693,26 +700,26 @@ export default defineComponent({
}

// FileID => Photo for new day
const dayMap = new Map<number, IPhoto>();
const dayMap = new Map<string, IPhoto>();
day.detail?.forEach((photo) => {
dayMap.set(photo.fileid, photo);
dayMap.set(photo.key!, photo);
});

this.selection.forEach((photo, fileid) => {
this.selection.forEach((photo, key) => {
// Process this day only
if (photo.dayid !== day.dayid) {
return;
}

// Remove all selections that are not in the new day
const newPhoto = dayMap.get(fileid);
const newPhoto = dayMap.get(photo.key!);
if (!newPhoto) {
this.selection.delete(fileid);
this.selection.delete(photo);
return;
}

// Update the photo object
this.selection.set(fileid, newPhoto);
this.selection.add(newPhoto);
newPhoto.flag |= this.c.FLAG_SELECTED;
});

Expand All @@ -722,7 +729,7 @@ export default defineComponent({
/**
* Download the currently selected files
*/
async downloadSelection(selection: Selection) {
async downloadSelection(selection: SelectionProxy) {
if (selection.size >= 100) {
if (!confirm(this.t('memories', 'You are about to download a large number of files. Are you sure?'))) {
return;
Expand All @@ -734,14 +741,14 @@ export default defineComponent({
/**
* Check if all files selected currently are favorites
*/
allSelectedFavorites(selection: Selection) {
allSelectedFavorites(selection: SelectionProxy) {
return Array.from(selection.values()).every((p) => p.flag & this.c.FLAG_IS_FAVORITE);
},

/**
* Favorite the currently selected photos
*/
async favoriteSelection(selection: Selection) {
async favoriteSelection(selection: SelectionProxy) {
const val = !this.allSelectedFavorites(selection);
for await (const favIds of dav.favoritePhotos(Array.from(selection.values()), val)) {
}
Expand All @@ -751,7 +758,7 @@ export default defineComponent({
/**
* Delete the currently selected photos
*/
async deleteSelection(selection: Selection) {
async deleteSelection(selection: SelectionProxy) {
if (selection.size >= 100) {
if (!confirm(this.t('memories', 'You are about to delete a large number of files. Are you sure?'))) {
return;
Expand All @@ -771,52 +778,52 @@ export default defineComponent({
/**
* Open the edit date dialog
*/
async editMetadataSelection(selection: Selection, sections?: number[]) {
async editMetadataSelection(selection: SelectionProxy, sections?: number[]) {
globalThis.editMetadata(Array.from(selection.values()), sections);
},

/**
* Open the files app with the selected file (one)
* Opens a new window.
*/
async viewInFolder(selection: Selection) {
async viewInFolder(selection: SelectionProxy) {
if (selection.size !== 1) return;
dav.viewInFolder(selection.values().next().value);
},

/**
* Archive the currently selected photos
*/
async archiveSelection(selection: Selection) {
async archiveSelection(selection: SelectionProxy) {
if (selection.size >= 100) {
if (!confirm(this.t('memories', 'You are about to touch a large number of files. Are you sure?'))) {
return;
}
}

for await (let delIds of dav.archiveFilesByIds(Array.from(selection.keys()), !this.routeIsArchive)) {
for await (let delIds of dav.archiveFilesByIds(Array.from(selection.fileIDs()), !this.routeIsArchive)) {
this.deleteSelectedPhotosById(delIds, selection);
}
},

/**
* Move selected photos to album
*/
async addToAlbum(selection: Selection) {
async addToAlbum(selection: SelectionProxy) {
globalThis.updateAlbums(Array.from(selection.values()));
},

/**
* Move selected photos to folder
*/
async moveToFolder(selection: Selection) {
async moveToFolder(selection: SelectionProxy) {
(<any>this.$refs.moveToFolderModal).open(Array.from(selection.values()));
},

/**
* Move selected photos to another person
*/
async moveSelectionToPerson(selection: Selection) {
async moveSelectionToPerson(selection: SelectionProxy) {
if (!this.config.show_face_rect && !this.routeIsRecognizeUnassigned) {
showError(this.t('memories', 'You must enable "Mark person in preview" to use this feature'));
return;
Expand All @@ -827,7 +834,7 @@ export default defineComponent({
/**
* Remove currently selected photos from person
*/
async removeSelectionFromPerson(selection: Selection) {
async removeSelectionFromPerson(selection: SelectionProxy) {
// Make sure route is valid
const { user, name } = this.$route.params;
if (this.$route.name !== 'recognize' || !user || !name) {
Expand Down
57 changes: 56 additions & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,61 @@ export type ITick = {
key?: number;
};

export class SelectionProxy {
Copy link
Owner

Choose a reason for hiding this comment

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

types.ts is intended only for interfaces, not implementations (this way, you can import type ...). This class can move to another file.

private map: Map<string, IPhoto> = new Map<string, IPhoto>();

add(photo: IPhoto): this {
this.map.set(photo.key!, photo);
return this;
}

get(photo: IPhoto): IPhoto | undefined {
return this.map.get(photo.key!);
Copy link
Owner

Choose a reason for hiding this comment

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

This, precisely, could be the issue. Photo.key is nullable because, well, it can be null. The bang operator discards this information without a check. There are indeed a bunch of places where we "fake" a IPhoto object that doesn't have a key.

By now though, I'm hoping the selection manager doesn't have to deal with these things. So a runtime assertion here might suffice, so we know if something breaks.

}

clear() {
this.map.clear();
}

delete(photo: IPhoto): boolean {
return this.map.delete(photo.key!);
}

has(photo: IPhoto): boolean {
return this.map.has(photo.key!);
}

values(): IterableIterator<IPhoto> {
return this.map.values();
}

get size() {
return this.map.size;
}

fileIDs(): number[] {
let fileIDs: number[] = [];
this.forEach((photo, _) => {
fileIDs.push(photo.fileid);
});
return fileIDs;
}

forEach(callbackfn: (value: IPhoto, key: string, map: Map<string, IPhoto>) => void, thisArg?: any): void {
this.map.forEach((value, key) => {
callbackfn.call(thisArg, value, key, this);
});
}

copy(): SelectionProxy {
let newProxy = new SelectionProxy();
this.forEach((photo, _) => {
newProxy.add(photo);
});
return newProxy;
}
}

export type ISelectionAction = {
/** Identifier (optional) */
id?: string;
Expand All @@ -228,7 +283,7 @@ export type ISelectionAction = {
/** Icon component */
icon: any;
/** Action to perform */
callback: (selection: Map<number, IPhoto>) => Promise<void>;
callback: (selection: any) => Promise<void>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type should be SelectionProxy but this doesn't work. Can someone help out here?

/** Condition to check for including */
if?: (self?: any) => boolean;
/** Allow for public routes (default false) */
Expand Down