fix: Fix a bug that caused some unassigned face photos to be unselectable #774
Conversation
|
I really hope that I have not broken anything with this 😆 But it works for me so far, I look forward to feedback! |
| @@ -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>; | |||
There was a problem hiding this comment.
The type should be SelectionProxy but this doesn't work. Can someone help out here?
g3n35i5
force-pushed
the
fix/unassigned-faces-selection-problem
branch
2 times, most recently
from
17433bb to
e3e20f9
Compare
| let photosToDelete: IPhoto[] = [] | ||
| for (const photo of selection.values()) { | ||
| if (photo === undefined) { | ||
| continue | ||
| } | ||
| if (delIds.indexOf(photo.fileid) > -1) { | ||
| photosToDelete.push(photo) | ||
| } | ||
| } |
There was a problem hiding this comment.
- Why not just
selection.values().filter( ... )? - Can get a safer time complexity by making
delIdsaSetfirst
| // 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) { |
There was a problem hiding this comment.
This comment and check should no longer be relevant.
| @@ -220,6 +220,61 @@ export type ITick = { | |||
| key?: number; | |||
| }; | |||
|
|
|||
| export class SelectionProxy { | |||
There was a problem hiding this comment.
types.ts is intended only for interfaces, not implementations (this way, you can import type ...). This class can move to another file.
| } | ||
|
|
||
| get(photo: IPhoto): IPhoto | undefined { | ||
| return this.map.get(photo.key!); |
There was a problem hiding this comment.
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.
This resolves #721