Redesign Video List as Table (#555)

* bank changes * bank changes * bank changes * bank changes * bank changes * bank changes * bank changes * bank changes * bank changes * bank changes * is it done? * changelog * changelog * wtf * tweaks * tweaks * tweaks * tweaks * bugfix * bugfix * bugfix * self review * fixes #548 * fix out of place blue focus color * fixes #556
aza547 · Nov 30, 2024 · d5bbbe9 · d5bbbe9
1 parent e6e8442
commit d5bbbe9
Show file tree

Hide file tree

Showing 32 changed files with 2,168 additions and 1,607 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,10 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## Unreleased
 ### Changed
 ### Added
+- Redesign the video selection panel to be more performant and useful.
+
 ### Fixed
 - Fix an issue where the upload/download icons would flicker.
 - Relax pull grouping timer as apparently Windows does a bad job of automatically keeping you in sync with an NTP server. 
 - [Issue 550](https://github.com/aza547/wow-recorder/issues/550) - Add the 90s Challenger's Peril correction to M+ chest calculation.
+- Fix a bug where deleted videos were sometimes not correctly deleted.
 - Fix an issue where the CMAA 2 setting in WoW could cause blurry video. 
 
 ## [6.0.4] - 2024-10-27

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -264,6 +264,7 @@
     "@radix-ui/react-toggle": "^1.1.0",
     "@radix-ui/react-toggle-group": "^1.1.0",
     "@radix-ui/react-tooltip": "^1.1.2",
+    "@tanstack/react-table": "^8.20.5",
     "atomic-queue": "^5.0.4",
     "axios": "^1.6.8",
     "byte-size": "^8.1.0",

diff --git a/release/app/package.json b/release/app/package.json
@@ -1,6 +1,6 @@
 {
   "name": "WarcraftRecorder",
-  "version": "6.0.5",
+  "version": "6.1.0",
   "description": "A World of Warcraft screen recorder",
   "main": "./dist/main/main.js",
   "author": {

diff --git a/src/main/Manager.ts b/src/main/Manager.ts
@@ -987,7 +987,14 @@ export default class Manager {
       } else {
         // Try to just delete the video from disk
         try {
-          await deleteVideoDisk(src);
+          // Bit weird we have to check a boolean here given all the error handling
+          // going on. That's just me taking an easy way out rather than fixing this
+          // more elegantly. TL;DR deleteVideoDisk doesn't throw anything.
+          const success = await deleteVideoDisk(src);
+
+          if (!success) {
+            throw new Error('Failed deleting video, will mark for delete');
+          }
         } catch (error) {
           // If that didn't work for any reason, try to at least mark it for deletion,
           // so that it can be picked up on refresh and we won't show videos the user

diff --git a/src/main/types.ts b/src/main/types.ts
@@ -1,4 +1,5 @@
 import { Size } from 'electron';
+import { uniqueId } from 'lodash';
 import { RawChallengeModeTimelineSegment } from './keystone';
 import { VideoCategory } from '../types/VideoCategory';
 import ConfigService from './ConfigService';
@@ -262,6 +263,10 @@ type RendererVideo = Metadata & {
   isProtected: boolean;
   cloud: boolean;
   multiPov: RendererVideo[];
+
+  // Used by frontend to uniquely identify a video, as videoName
+  // is identical for a disk and cloud viewpoint.
+  uniqueId: string;
 };
 
 type SoloShuffleTimelineSegment = {
@@ -310,8 +315,6 @@ type AppState = {
   page: Pages;
   category: VideoCategory;
   playingVideo: RendererVideo | undefined; // the video being played by the player
-  selectedVideoName: string | undefined;
-  numVideosDisplayed: number;
   videoFilterQuery: string;
   videoFullScreen: boolean;
 };

diff --git a/src/main/util.ts b/src/main/util.ts
@@ -179,19 +179,23 @@ const tryUnlink = async (file: string): Promise<boolean> => {
  */
 const deleteVideoDisk = async (videoPath: string) => {
   console.info('[Util] Deleting video', videoPath);
-  const success = await tryUnlink(videoPath);
+  const deletedMp4 = await tryUnlink(videoPath);
 
-  if (!success) {
-    // If we can't delete the video file, make sure we don't delete the metadata
-    // file either, which would leave the video file dangling.
-    return;
+  if (!deletedMp4) {
+    return false;
   }
 
   const metadataPath = getMetadataFileNameForVideo(videoPath);
-  await tryUnlink(metadataPath);
+  const deletedJson = await tryUnlink(metadataPath);
+
+  if (!deletedJson) {
+    return false;
+  }
 
   const thumbnailPath = getThumbnailFileNameForVideo(videoPath);
-  await tryUnlink(thumbnailPath);
+  const deletedPng = await tryUnlink(thumbnailPath);
+
+  return deletedPng;
 };
 
 /**
@@ -228,15 +232,19 @@ const loadVideoDetailsDisk = async (
     const metadata = await getMetadataForVideo(video.name);
     const thumbnailSource = getThumbnailFileNameForVideo(video.name);
 
+    const videoName = path.basename(video.name, '.mp4');
+    const uniqueId = `${videoName}-disk`;
+
     return {
       ...metadata,
-      videoName: path.basename(video.name, '.mp4'),
+      videoName,
       mtime: video.mtime,
       videoSource: video.name,
       thumbnailSource,
       isProtected: Boolean(metadata.protected),
       cloud: false,
       multiPov: [],
+      uniqueId,
     };
   } catch (error) {
     // Just log it and rethrow. Want this to be diagnosable.
@@ -800,16 +808,6 @@ const markForVideoForDelete = async (videoPath: string) => {
   }
 };
 
-/**
- * Sort alphabetically by player name.
- */
-const povNameSort = (a: RendererVideo, b: RendererVideo) => {
-  const playerA = a.player?._name;
-  const playerB = b.player?._name;
-  if (!playerA || !playerB) return 0;
-  return playerA.localeCompare(playerB);
-};
-
 /**
  * Convert a RendererVideo type to a Metadata type, used when downloading
  * videos from cloud to disk.
@@ -833,6 +831,7 @@ const cloudSignedMetadataToRendererVideo = (metadata: CloudSignedMetadata) => {
   // For cloud videos, the signed URLs are the sources.
   const videoSource = metadata.signedVideoKey;
   const thumbnailSource = metadata.signedThumbnailKey;
+  const uniqueId = `${metadata.videoName}-cloud`;
 
   // We don't want the signed properties themselves.
   const mutable: any = metadata;
@@ -847,6 +846,7 @@ const cloudSignedMetadataToRendererVideo = (metadata: CloudSignedMetadata) => {
     cloud: true,
     isProtected: Boolean(mutable.protected),
     mtime: 0,
+    uniqueId,
   };
 
   return video;
@@ -1025,7 +1025,6 @@ export {
   reverseChronologicalVideoSort,
   areDatesWithinSeconds,
   markForVideoForDelete,
-  povNameSort,
   rendererVideoToMetadata,
   cloudSignedMetadataToRendererVideo,
   exists,

diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx
@@ -35,15 +35,6 @@ const WarcraftRecorder = () => {
   const upgradeNotified = useRef(false);
   const { toast } = useToast();
 
-  // The video state contains most of the frontend state, it's complex so
-  // frontend triggered modifications go through the StateManager class, which
-  // calls the React set function appropriately.
-  const [videoState, setVideoState] = useState<RendererVideo[]>([]);
-
-  const stateManager = useRef<StateManager>(
-    StateManager.getInstance(setVideoState)
-  );
-
   const [recorderStatus, setRecorderStatus] = useState<RecStatus>(
     RecStatus.WaitingForWoW
   );
@@ -84,12 +75,6 @@ const WarcraftRecorder = () => {
     page: Pages.None,
     category: getCategoryFromConfig(config),
     playingVideo: undefined,
-    selectedVideoName: undefined,
-
-    // Limit the number of videos displayed for performance. User can load more
-    // by clicking the button, but mainline case will be to watch back recent
-    // videos.
-    numVideosDisplayed: 10,
 
     // Any text applied in the filter bar gets translated into a filter here.
     videoFilterQuery: '',
@@ -98,6 +83,15 @@ const WarcraftRecorder = () => {
     videoFullScreen: false,
   });
 
+  // The video state contains most of the frontend state, it's complex so
+  // frontend triggered modifications go through the StateManager class, which
+  // calls the React set function appropriately.
+  const [videoState, setVideoState] = useState<RendererVideo[]>([]);
+
+  const stateManager = useRef<StateManager>(
+    StateManager.getInstance(setVideoState, appState, setAppState)
+  );
+
   // Used to allow for hot switching of video players when moving between POVs.
   const persistentProgress = useRef(0);
 
@@ -154,6 +148,15 @@ const WarcraftRecorder = () => {
     ipc.on('updateCrashes', updateCrashes);
   }, []);
 
+  // Debugging why we needed this hurt. I think it's because when we call setAppState, it sets 
+  // appState to undefined and reassigns it in this component. However that leaves the StateManager
+  // singleton with a reference pointing to undefined which breaks the frontend. So here we reapply
+  // the appState to the StateManager every time it updates. This is almost certainly massively
+  // overengineered but for now it works.
+  useEffect(() => {
+    stateManager.current.updateAppState(appState);
+  }, [appState]);
+
   return (
     <Box
       id="main-box"