fix: visible-order range selection + preview-after-Space sequence
Two related bugs around visible vs API order. 1. Multi-select range selection (Shift+Click, Shift+Arrow): - The previous selectRange walked the API photos array and only ADDED to the existing selection, never replacing or shrinking. So Shift+clicking to the left often "did nothing" (already-selected ids skipped) and the selection never matched the user's intended range. - Replace with a store action that walks visiblePhotoIds (the visual row-major sequence Timeline already publishes), de-dupes ids (tag-grouped views can repeat photos), and REPLACES the selection. - Track the range anchor as rangeStartId (a photo id) instead of an index so it survives filter changes and works correctly when API index != visual position. - Drop the now-redundant lastSelectedIndex / globalIndex plumbing from selectPhoto / togglePhotoSelection — call sites simplify to pass just the photo id. 2. Preview navigation after pressing Space: - The Space hotkey path called openPreview(id) without a sequence and relied on the store's fallback to whatever Timeline most recently published. Make it explicit: read visiblePhotoIds from the store snapshot at fire time and pass it through. Same effect in the happy case but eliminates any subtle publisher timing question. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -166,10 +166,9 @@ export function Timeline() {
|
||||
const {
|
||||
selectedPhotos,
|
||||
activePhotoId,
|
||||
lastSelectedIndex,
|
||||
rangeStartIndex,
|
||||
selectPhoto,
|
||||
togglePhotoSelection,
|
||||
selectRange,
|
||||
clearSelection,
|
||||
openPreview,
|
||||
} = usePhotoStore()
|
||||
@@ -220,19 +219,6 @@ export function Timeline() {
|
||||
return result
|
||||
}, [items])
|
||||
|
||||
// Range-selection helper. Operates on the global photos array, not on
|
||||
// virtualizer items.
|
||||
const selectRange = (endIndex: number) => {
|
||||
const startIndex = rangeStartIndex ?? lastSelectedIndex ?? 0
|
||||
const minIndex = Math.min(startIndex, endIndex)
|
||||
const maxIndex = Math.max(startIndex, endIndex)
|
||||
for (let i = minIndex; i <= maxIndex; i++) {
|
||||
if (i < photos.length && !selectedPhotos.includes(photos[i].id)) {
|
||||
togglePhotoSelection(photos[i].id, i)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Virtual scrolling setup with per-item heights.
|
||||
const virtualizer = useVirtualizer({
|
||||
count: items.length,
|
||||
@@ -384,9 +370,9 @@ export function Timeline() {
|
||||
const dest = photoRows[nextRow]?.cells[nextCol]
|
||||
if (!dest) return
|
||||
if (e.shiftKey) {
|
||||
selectRange(dest.globalIndex)
|
||||
selectRange(dest.photo.id)
|
||||
} else {
|
||||
selectPhoto(dest.photo.id, dest.globalIndex)
|
||||
selectPhoto(dest.photo.id)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -410,9 +396,9 @@ export function Timeline() {
|
||||
case 'a':
|
||||
if (e.ctrlKey || e.metaKey) {
|
||||
e.preventDefault()
|
||||
photos.forEach((photo, index) => {
|
||||
photos.forEach((photo) => {
|
||||
if (!selectedPhotos.includes(photo.id)) {
|
||||
togglePhotoSelection(photo.id, index)
|
||||
togglePhotoSelection(photo.id)
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -509,7 +495,7 @@ export function Timeline() {
|
||||
}}
|
||||
>
|
||||
<div className="flex" style={{ gap: `${GAP}px` }}>
|
||||
{item.cells.map(({ photo, globalIndex }) => (
|
||||
{item.cells.map(({ photo }) => (
|
||||
<PhotoThumbnail
|
||||
key={photo.id}
|
||||
photo={photo}
|
||||
@@ -517,12 +503,12 @@ export function Timeline() {
|
||||
isSelected={selectedPhotos.includes(photo.id)}
|
||||
isInActiveHeap={activeHeapMembers.has(photo.id)}
|
||||
onClick={(e) => {
|
||||
if (e.shiftKey && lastSelectedIndex !== null) {
|
||||
selectRange(globalIndex)
|
||||
if (e.shiftKey) {
|
||||
selectRange(photo.id)
|
||||
} else if (e.ctrlKey || e.metaKey) {
|
||||
togglePhotoSelection(photo.id, globalIndex)
|
||||
togglePhotoSelection(photo.id)
|
||||
} else {
|
||||
selectPhoto(photo.id, globalIndex)
|
||||
selectPhoto(photo.id)
|
||||
}
|
||||
}}
|
||||
onDoubleClick={() => openPreview(photo.id, visibleSequence)}
|
||||
|
||||
@@ -300,8 +300,13 @@ export function useKeyboardShortcuts(props: KeyboardShortcutsProps) {
|
||||
// Space toggles the preview view (open from grid, close from preview).
|
||||
// Double-click on a thumbnail does the same.
|
||||
const openPreviewFromGrid = () => {
|
||||
const id = usePhotoStore.getState().activePhotoId ?? getFirstPhotoId?.() ?? null
|
||||
if (id) openPreview(id)
|
||||
const state = usePhotoStore.getState()
|
||||
const id = state.activePhotoId ?? getFirstPhotoId?.() ?? null
|
||||
if (!id) return
|
||||
// Pass the current visible sequence explicitly so preview navigation
|
||||
// walks the order the user actually sees, even if the active photo
|
||||
// was selected before the publisher caught up.
|
||||
openPreview(id, state.visiblePhotoIds)
|
||||
}
|
||||
|
||||
const togglePreview = () => {
|
||||
|
||||
@@ -7,20 +7,27 @@ interface PhotoStore {
|
||||
photos: Photo[]
|
||||
selectedPhotos: string[]
|
||||
activePhotoId: string | null
|
||||
lastSelectedIndex: number | null
|
||||
rangeStartIndex: number | null
|
||||
/** The id of the photo where the current range-selection anchor lives.
|
||||
* Set when the user clicks (without modifiers) or arrow-navigates,
|
||||
* read by selectRange to figure out the start of a Shift+Click /
|
||||
* Shift+Arrow range. Tracked as an id (not an index) so it survives
|
||||
* filter changes and works correctly in tag-grouped mode where
|
||||
* visual indices != API indices. */
|
||||
rangeStartId: string | null
|
||||
viewMode: ViewMode
|
||||
/** Flat sequence of photo ids in the order they currently appear in
|
||||
* the timeline grid (including duplicates from tag-grouping). The
|
||||
* preview view walks this sequence so arrow nav matches the order
|
||||
* the user actually sees. Owned by the Timeline component, which
|
||||
* rewrites it whenever its layout items change. */
|
||||
* the timeline grid (including duplicates from tag-grouping). Used
|
||||
* for both preview navigation order and range selection. Owned by
|
||||
* the Timeline component. */
|
||||
visiblePhotoIds: string[]
|
||||
|
||||
setPhotos: (photos: Photo[]) => void
|
||||
selectPhoto: (id: string, index: number) => void
|
||||
togglePhotoSelection: (id: string, index: number) => void
|
||||
selectRange: (endIndex: number) => void
|
||||
selectPhoto: (id: string) => void
|
||||
togglePhotoSelection: (id: string) => void
|
||||
/** Replace the selection with every photo between the current
|
||||
* rangeStartId and the supplied endId, walking visiblePhotoIds in
|
||||
* visual order. If there's no anchor yet, anchors at endId. */
|
||||
selectRange: (endId: string) => void
|
||||
deselectPhoto: (id: string) => void
|
||||
clearSelection: () => void
|
||||
setActivePhoto: (id: string | null) => void
|
||||
@@ -38,47 +45,80 @@ export const usePhotoStore = create<PhotoStore>((set) => ({
|
||||
photos: [],
|
||||
selectedPhotos: [],
|
||||
activePhotoId: null,
|
||||
lastSelectedIndex: null,
|
||||
rangeStartIndex: null,
|
||||
rangeStartId: null,
|
||||
viewMode: 'grid',
|
||||
visiblePhotoIds: [],
|
||||
|
||||
setPhotos: (photos) => set({ photos }),
|
||||
|
||||
selectPhoto: (id, index) => set({
|
||||
|
||||
selectPhoto: (id) => set({
|
||||
selectedPhotos: [id],
|
||||
activePhotoId: id,
|
||||
lastSelectedIndex: index,
|
||||
rangeStartIndex: index,
|
||||
rangeStartId: id,
|
||||
}),
|
||||
|
||||
togglePhotoSelection: (id, index) => set((state) => {
|
||||
|
||||
togglePhotoSelection: (id) => set((state) => {
|
||||
const isSelected = state.selectedPhotos.includes(id)
|
||||
return {
|
||||
selectedPhotos: isSelected
|
||||
? state.selectedPhotos.filter(photoId => photoId !== id)
|
||||
? state.selectedPhotos.filter((photoId) => photoId !== id)
|
||||
: [...state.selectedPhotos, id],
|
||||
lastSelectedIndex: index,
|
||||
rangeStartIndex: isSelected ? state.rangeStartIndex : index,
|
||||
// A toggle-add anchors the range here so a subsequent Shift+click
|
||||
// extends from this id. A toggle-remove leaves the anchor alone.
|
||||
rangeStartId: isSelected ? state.rangeStartId : id,
|
||||
activePhotoId: id,
|
||||
}
|
||||
}),
|
||||
|
||||
selectRange: (endIndex) => {
|
||||
// Note: The actual range selection logic should be handled in the Timeline component
|
||||
// which has access to the photos array
|
||||
set({
|
||||
lastSelectedIndex: endIndex,
|
||||
})
|
||||
},
|
||||
|
||||
|
||||
selectRange: (endId) =>
|
||||
set((state) => {
|
||||
const ids = state.visiblePhotoIds
|
||||
// No published sequence yet → just behave like a single-pick.
|
||||
if (ids.length === 0) {
|
||||
return {
|
||||
selectedPhotos: [endId],
|
||||
activePhotoId: endId,
|
||||
rangeStartId: endId,
|
||||
}
|
||||
}
|
||||
const anchorId = state.rangeStartId ?? state.activePhotoId ?? endId
|
||||
const startIdx = ids.indexOf(anchorId)
|
||||
const endIdx = ids.indexOf(endId)
|
||||
if (startIdx < 0 || endIdx < 0) {
|
||||
return {
|
||||
selectedPhotos: [endId],
|
||||
activePhotoId: endId,
|
||||
rangeStartId: endId,
|
||||
}
|
||||
}
|
||||
const min = Math.min(startIdx, endIdx)
|
||||
const max = Math.max(startIdx, endIdx)
|
||||
// De-dupe because the visible sequence can repeat photos in
|
||||
// tag-grouped mode.
|
||||
const seen = new Set<string>()
|
||||
const next: string[] = []
|
||||
for (let i = min; i <= max; i++) {
|
||||
const id = ids[i]
|
||||
if (!seen.has(id)) {
|
||||
seen.add(id)
|
||||
next.push(id)
|
||||
}
|
||||
}
|
||||
return {
|
||||
selectedPhotos: next,
|
||||
activePhotoId: endId,
|
||||
// Anchor stays put — the user can keep extending from the
|
||||
// original click point, matching Lightroom / Finder behavior.
|
||||
}
|
||||
}),
|
||||
|
||||
deselectPhoto: (id) => set((state) => ({
|
||||
selectedPhotos: state.selectedPhotos.filter(photoId => photoId !== id)
|
||||
})),
|
||||
|
||||
clearSelection: () => set({
|
||||
|
||||
clearSelection: () => set({
|
||||
selectedPhotos: [],
|
||||
lastSelectedIndex: null,
|
||||
rangeStartIndex: null,
|
||||
rangeStartId: null,
|
||||
}),
|
||||
|
||||
setActivePhoto: (id) => set({ activePhotoId: id }),
|
||||
|
||||
Reference in New Issue
Block a user