Add recording controls enhancement #733

p-malecki · 2024-11-15T16:58:24Z

This PR introduces recording controls enhancement:

introduces a toggle button to start/stop recordings
opens a save dialog immediately after pressing stop (in future, we could reuse replayOverlay for video previews and edits)
ignores and does not save recordings under 1 second
adds a maximum limit of 5 minutes for recordings, after which it automatically pauses and opens the save dialog
adds timestamps to the filenames of video recordings
eliminates text labels from the Replay and Logs buttons to create more space in the top button group
updates the Replay button icon

Test plan:

Start a recording, interact with the test app, then stop the recording after a few seconds and save the file to a selected location. Open the saved recording to verify that the app's screen was correctly captured.
Start a recording and stop it immediately to confirm that the save dialog does not appear for recordings under 1 second.
Test the maximum recording time to ensure that it automatically stops and opens the save dialog after the 5-minute limit is reached.
Ensure that the Replay button is disabled during recording.
After capturing a recording, test the Replay functionality to ensure it still works correctly.
Verify that the timestamps on the recorded video filenames reflect the correct date and time, matching the system clock.

Before:

After:

Screen.Recording.2024-11-18.at.18.53.39.mov

resolves: #708

vercel · 2024-11-15T16:58:28Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
radon-ide	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 19, 2024 3:58pm

kmagiera

I like the changes UI-wise, one suggestion I'd have is to maybe try moving the red dot and timer into the recording button. That is, when we hit record, the red dot displays there instead of "stop" button. This way it should be cleaner that the recording is in progress as the top bar is much more visible than the recording progress component that suddenly appears in top left edge when no one expects it.

kmagiera · 2024-11-18T11:26:44Z

packages/vscode-extension/src/devices/preview.ts

@@ -102,23 +116,23 @@ export class Preview implements Disposable {
    this.subprocess?.stdin?.write("pointer show false\n");
  }

-  public startReplays() {
+  public startVideoRecording(videoId: string) {


It is a wrong level of abstraction here. We assume in the callback that IDs can be "recording" and "replay" so we shouldn't allow any string as videoID, as if anything else is passed, the callback would never trigger.

One way would be to specify allowed values in typescript, but I think that this method shouldn't expose videoID as a parameter and just make it an implementation detail.

I've made the changes. Let me know if they look as you expected. Additionally, I've added sendCommandOrThrow and handleVideoRecordingPromise to reduce code duplication.

kmagiera · 2024-11-18T11:33:15Z

packages/vscode-extension/src/common/Project.ts

+  stopRecording(): void;
+  captureRecording(): Promise<RecordingData>;


can we only have one method for stopping that returns the data? What is the point of having both?

You're right, one is enough.

…nd captureRecording

kmagiera · 2024-11-19T08:13:57Z

packages/vscode-extension/src/common/Project.ts

@@ -145,6 +145,8 @@ export interface ProjectInterface {
  getDeepLinksHistory(): Promise<string[]>;
  openDeepLink(link: string): Promise<void>;

+  startRecording(): void;
+  captureAndStopRecording(): Promise<RecordingData>;


For symmetry we could just call it "stopRecording" – I believe there's no way for sim server to stop recording without producing the recorded file anyways, is there?

Actually, simserver allows starting, stopping, and saving video recordings without stopping them. CaptureReplay is responsible only for saving, unlike captureAndStopRecording, which saves and stops recordings, so name captureAndStopRecording is more appropriate

what if we change it to stopRecording(capture: boolean)

kmagiera · 2024-11-19T08:14:34Z

packages/vscode-extension/src/devices/DeviceBase.ts

+  public stopRecording() {
+    return this.preview?.stopRecording();
+  }
+
+  public async captureRecording() {
+    if (!this.preview) {
+      throw new Error("Preview not started");
+    }
+    return this.preview.captureRecording();


If we only have one method in the Project interface, I don't think there's a lot of value in having separate methods at this level given we only forward the call down

I removed the stopRecording calls after reloading and changing device settings, as the first was causing a bug with recordings not stopping in UI, and the second was unwanted behaviour (we don't want to stop recording on eg. font change), so there is no need for the stopRecording method, and we now use captureAndStopRecording instead.

kmagiera · 2024-11-19T08:16:08Z

packages/vscode-extension/src/devices/DeviceBase.ts

+  public stopReplays() {
+    return this.preview?.stopReplays();
+  }


I think I understand what was the confusion with stop / start vs capture. With replays we have option to enable / disable replays and then we use "capture" when we want to save replay. Maybe we should rename startReplay and stopReplay to enableReplay / disableReplay to avoid this confusion?

I agree, names enableReplayand disableReplay seem more accurate.

kmagiera · 2024-11-19T08:21:52Z

packages/vscode-extension/src/devices/preview.ts

+
+    const lastPromiseKey =
+      promiseType === "recording" ? "lastRecordingPromise" : "lastReplayPromise";
+    const lastPromise = this[lastPromiseKey];


I don't like using this like that. If we access it this way, I'd rather have a single map of promises that we remember. This will be a bit over-abstracted but more clear to read:

So instead of haveing lastRecordingPromise and lastReplayPromise we'd have:

private videoRecordingPromises: Map<string, VideoRecordingPromiseHandlers>

Then we'd use the map to lookup the last promise.

This way we wouldn't need to match videoId agains constants in section where we wait for video_ready and instead just lookup the ID in that map

Right, this mapping could be useful in the future if we decide to introduce additional recordings.

It also removes stopRecording calls after reload or device settings change.

kmagiera · 2024-11-19T11:00:37Z

packages/vscode-extension/src/devices/preview.ts

+    VideoRecordingPromiseType,
+    VideoRecordingPromiseHandlers | undefined


I think we can just use string here as key, the keys that we use are not important here because we don't iterate over the map anywhere and we also don't have any special handling based on the key now.

We should not allow undefined value. Just remove entry from map when we want to reset it.

kmagiera · 2024-11-19T11:01:38Z

packages/vscode-extension/src/devices/preview.ts

+    this.videoRecordingPromises = new Map<
+      VideoRecordingPromiseType,
+      VideoRecordingPromiseHandlers | undefined
+    >();


this is an empty map, we can construct it in the line where we define it

kmagiera · 2024-11-19T11:02:25Z

packages/vscode-extension/src/devices/preview.ts

+    this.videoRecordingPromises.set("recording", undefined);
+    this.videoRecordingPromises.set("replay", undefined);


we don't need undefined value there. If its not there map.get(key) will return undefined

kmagiera · 2024-11-19T11:03:10Z

packages/vscode-extension/src/devices/preview.ts

+            ? videoErrorMatch[1]
+            : "";
+
+          if (!this.videoRecordingPromises.has(videoId as VideoRecordingPromiseType)) {


if we just use string as key, we wouldn't need as here

kmagiera · 2024-11-19T11:03:47Z

packages/vscode-extension/src/devices/preview.ts


+          const handlers = this.videoRecordingPromises.get(videoId as VideoRecordingPromiseType);


same comment about as. We should just get it earlier and throw when handlers is undefined rather than using .has

kmagiera · 2024-11-19T11:04:25Z

packages/vscode-extension/src/devices/preview.ts


+          const handlers = this.videoRecordingPromises.get(videoId as VideoRecordingPromiseType);
+          this.videoRecordingPromises.set(videoId as VideoRecordingPromiseType, undefined);


.delete(videoId)

kmagiera · 2024-11-19T11:06:40Z

packages/vscode-extension/src/devices/preview.ts

-    this.lastReplayPromise = { resolve: resolvePromise!, reject: rejectPromise! };
-    stdin.write(`video replay save\n`);
-    return promise;
+    return this.handleVideoRecordingPromise("replay");


handleVideoRecordingPromise is a weird name. There's no promise at this point, so what is this method "handling"??

I'd just name it saveVideo or saveVideoWithID

kmagiera · 2024-11-19T11:09:22Z

packages/vscode-extension/src/webview/views/PreviewView.tsx

@@ -126,6 +130,46 @@ function PreviewView() {
    }
  };

+  const MAX_RECORDING_TIME = 300;


append units to this constant such that it is clear, e.g. MAX_RECORDING_TIME_SEC

kmagiera · 2024-11-19T11:10:11Z

packages/vscode-extension/src/webview/views/PreviewView.tsx

+    const interval = setInterval(() => {
+      setRecordingTime((prevRecordingTime) => {
+        if (prevRecordingTime >= MAX_RECORDING_TIME - 1) {
+          clearInterval(interval);
+          handleRecording();
+          return MAX_RECORDING_TIME;
+        }
+        return prevRecordingTime + 1;
+      });
+    }, 1000);


why there's an interval here, can't we just use setTimeout here?

kmagiera · 2024-11-19T15:14:16Z

packages/vscode-extension/src/devices/DeviceBase.ts

+    const recordingDataPromise = this.preview.captureRecording();
+    this.preview?.stopRecording();
+    return recordingDataPromise;


for symmetry, it'd be better if this method just proxied the call down to preview instead of making two calls

kmagiera · 2024-11-19T15:15:42Z

packages/vscode-extension/src/devices/preview.ts

+          if (!this.videoRecordingPromises.has(videoId)) {
+            throw new Error(`Invalid video ID: ${videoId}`);
+          }

+          const handlers = this.videoRecordingPromises.get(videoId);


Suggested change

if (!this.videoRecordingPromises.has(videoId)) {

throw new Error(`Invalid video ID: ${videoId}`);

}

const handlers = this.videoRecordingPromises.get(videoId);

const handlers = this.videoRecordingPromises.get(videoId);

if (!handlers) {

throw new Error(`Invalid video ID: ${videoId}`);

}

After this we don't need to check if handlers is set as we do in the lines below

kmagiera · 2024-11-19T15:19:40Z

packages/vscode-extension/src/webview/views/PreviewView.tsx

+    setTimeout(() => {
+      setRecordingTime((prevRecordingTime) => {
+        if (prevRecordingTime >= MAX_RECORDING_TIME_SEC - 1) {
+          handleRecording();
+          return MAX_RECORDING_TIME_SEC;
+        }
+        return prevRecordingTime + 1;
+      });
+    }, 1000);


I don't understand this bit. Maybe it should use interval actually. Now it doesn't do effect cleanup which may result in this timer running indefinitely until the timeout is hit regardless of whether the video is stopped or not.

I've tested it and it behaves weird. After stopping recording before the time limit, setTimeout runs once more with prevRecordingTime set to 0 by handleRecording.
Adding a cleanup function would be a better solution.

p-malecki added 17 commits November 12, 2024 10:32

WIP Add btn to PreviewView

46e31d8

Add recoding to DeviceSettingsDropdown

176488a

Make logs btn round, remove btns labels

966399a

Update icons

cc311ee

Remove recording from DeviceSettingsDropdown

60b3576

Update recording btn style

2110684

Add startRecording and captureRecording

0b9f997

Update PreviewView

4b77af6

Rename functions to use more general VideoRecording instead of Replays

cd2140a

Use directly saveVideoRecording

4d7dcb4

Add timestamp to saveVideoRecording

f0868de

Stop recording after save

7ce0a4f

Update rec indicator css

d5ddbb9

Rm

60a0c86

Update IconButton css

5704638

Update IconButton css

1809870

Fix timestamp

c03cda5

p-malecki requested a review from kmagiera November 15, 2024 16:58

Merge branch 'main' into @p-malecki/add-recording-enhancement

b7f6a40

vercel bot deployed to Preview November 15, 2024 18:04 View deployment

fix lint

5831986

vercel bot deployed to Preview November 15, 2024 18:07 View deployment

fix lint

7aa47cb

vercel bot deployed to Preview November 15, 2024 18:09 View deployment

kmagiera requested changes Nov 18, 2024

View reviewed changes

Merge branch 'main' into @p-malecki/add-recording-enhancement

c3780f1

vercel bot deployed to Preview November 18, 2024 15:47 View deployment

Use one method stopAndCaptureRecording instead of two stopRecording a…

fb88a61

…nd captureRecording

vercel bot deployed to Preview November 18, 2024 16:26 View deployment

vercel bot deployed to Preview November 18, 2024 17:20 View deployment

Move rec indicator and timer to recording button

ff937f9

vercel bot deployed to Preview November 18, 2024 17:39 View deployment

p-malecki added 2 commits November 18, 2024 18:44

Format css

899f74b

Remove videoId from methods in preview.ts

f3f9647

vercel bot deployed to Preview November 18, 2024 18:05 View deployment

p-malecki requested a review from kmagiera November 18, 2024 18:10

Refactor by adding handleVideoRecordingPromise

7af6ec9

vercel bot deployed to Preview November 18, 2024 18:23 View deployment

kmagiera requested changes Nov 19, 2024

View reviewed changes

Rename startReplays to enableReplay

e23b6f3

vercel bot deployed to Preview November 19, 2024 08:54 View deployment

Use videoRecordingPromises map

598c678

vercel bot deployed to Preview November 19, 2024 09:16 View deployment

p-malecki added 2 commits November 19, 2024 10:29

Refactor captureAndStopRecording

c01b50c

Rm test recording

1612f80

vercel bot deployed to Preview November 19, 2024 09:29 View deployment

Remove stopRecording from DeviceBase

8e86486

It also removes stopRecording calls after reload or device settings change.

vercel bot deployed to Preview November 19, 2024 09:37 View deployment

p-malecki requested a review from kmagiera November 19, 2024 09:53

Merge branch 'main' into @p-malecki/add-recording-enhancement

1d9cc95

vercel bot deployed to Preview November 19, 2024 10:58 View deployment

kmagiera requested changes Nov 19, 2024

View reviewed changes

Apply changes from review comments

1126d84

vercel bot deployed to Preview November 19, 2024 11:39 View deployment

p-malecki requested a review from kmagiera November 19, 2024 11:41

kmagiera requested changes Nov 19, 2024

View reviewed changes

Apply changes from review comments

94e7f28

p-malecki requested a review from kmagiera November 19, 2024 15:58

vercel bot deployed to Preview November 19, 2024 15:58 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add recording controls enhancement #733

Add recording controls enhancement #733

p-malecki commented Nov 15, 2024 •

edited

Loading

vercel bot commented Nov 15, 2024 •

edited

Loading

kmagiera left a comment

kmagiera Nov 18, 2024

p-malecki Nov 18, 2024 •

edited

Loading

kmagiera Nov 18, 2024

p-malecki Nov 18, 2024

kmagiera Nov 19, 2024

p-malecki Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

p-malecki Nov 19, 2024

kmagiera Nov 19, 2024

p-malecki Nov 19, 2024

kmagiera Nov 19, 2024

p-malecki Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

kmagiera Nov 19, 2024

p-malecki Nov 19, 2024

		stopRecording(): void;
		captureRecording(): Promise<RecordingData>;

		VideoRecordingPromiseType,
		VideoRecordingPromiseHandlers \| undefined

		this.videoRecordingPromises.set("recording", undefined);
		this.videoRecordingPromises.set("replay", undefined);


		const handlers = this.videoRecordingPromises.get(videoId as VideoRecordingPromiseType);


		const handlers = this.videoRecordingPromises.get(videoId as VideoRecordingPromiseType);
		this.videoRecordingPromises.set(videoId as VideoRecordingPromiseType, undefined);

Add recording controls enhancement #733

Are you sure you want to change the base?

Add recording controls enhancement #733

Conversation

p-malecki commented Nov 15, 2024 • edited Loading

Test plan:

vercel bot commented Nov 15, 2024 • edited Loading

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-malecki Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-malecki commented Nov 15, 2024 •

edited

Loading

vercel bot commented Nov 15, 2024 •

edited

Loading

p-malecki Nov 18, 2024 •

edited

Loading