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

Jump to bottom

Require macOS 10.13 and add support for pause/resume #14

Merged
merged 10 commits into from
Mar 13, 2021
Merged

Require macOS 10.13 and add support for pause/resume #14

merged 10 commits into from
Mar 13, 2021

Conversation

karaggeorge
Copy link
Member

Closes #2
Closes #5
Closes #1

Also replaces #12 (re-implemented those changes here)

So, this has a bunch of changes:

  • Adds swift-argument-parser for the CLI part to manage commands, since now they have increased a lot
  • Adds a two way communication capability between the main aperture process and the node instance via:
    • Ability to send events (and get a response)
    • Ability to listen to events

Aperture sends events like onStart, onPause etc that we can listen to, and we can send events to Aperture like pause, resume to access functionality.

The communication happens using DistributedNotificationCenter and specifically mapped event names, inspired by how electron-better-ipc works.
This is sandboxed to a single aperture process using a randomly generated processId which is included in the events.

Also, the events can answer back using another unique id that is generated on the spot for each event. This way two parallel events of the same type won't get their responses confused.

I've tested the regular flow, including pause, resume and isPaused to ensure it works.

A weird observation. This code:

await recorder.startRecording(options);
console.log('Started recording');

recorder.isFileReady.then(() => console.log('File is ready'))

await recorder.pause();
console.log('Paused recording');

await delay(5000)

await recorder.resume();
console.log('Resume recording');

Outputs:

Started recording
Paused recording
Resumed recording
File is ready

And in the output the first frame is after the resume call (no time-skip). So, we could theoretically pause right after start, then play a countdown timer or do whatever and then resume, to ensure we start at a convenient time for us.

karaggeorge
karaggeorge commented Jul 1, 2020
View reviewed changes
Package.swift Outdated
@@ -1,4 +1,4 @@
// swift-tools-version:5.2
// swift-tools-version:5.1
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on Mojave, so I can't run 5.2. Had to set to 5.1 for development. We can bump again before merging

Package.swift Outdated
@@ -15,13 +15,15 @@ let package = Package(
)
],
dependencies: [
.package(url: "https://github.com/wulkano/Aperture", from: "0.2.0")
.package(url: "https://github.com/wulkano/Aperture", from: "0.2.0"),
.package(url: "https://github.com/apple/swift-argument-parser", from: "0.1.0")
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, used 0.1.0 because I can only run on Swift 5.1, but it had everything I needed for this

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a TODO comment about upgrading this dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

After I'm done with the rest of the comments if you want to pull down and migrate, that'd work too. I don't know if any breaking changes happened. I just can't even fetch 0.2.0 without using Swift tools 5.2, so I couldn't test it

Copy link
Contributor

Choose a reason for hiding this comment

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

We can keep it 5.1 until you can upgrade.

@karaggeorge
Copy link
Member Author

@sindresorhus I'm sure my swift code needs cleanup, feel free to push any updates needed on the branch if you don't want to comment on everything 😄

Also if you can test it locally as well that'd be great

@karaggeorge karaggeorge requested a review from sindresorhus July 1, 2020 23:50
@karaggeorge karaggeorge changed the title A lot of improvements Rewrite using SwiftArgumentParser and DistributedNotificationCenter Jul 1, 2020
@karaggeorge
Copy link
Member Author

I haven't added the 1 sec delay we discussed in the other PR. I didn't test it here. I'll test it locally in a bit, and update with the results

@karaggeorge
Copy link
Member Author

Looks like there's still that 1s delay. Both after just resolving startRecording() and if you start/pause/resume, there's 1s delay after resume() resolves.
Running it a few times, I've gotten anywhere between 0.6 and 0.9s delay

However, from the moment before you call .resume() and it resolving it's about .1s

So, if we add the 1s delay on both promises, we could call .resume() and know we have about 1s to perform all animations before the recording starts

@sindresorhus
Copy link
Contributor

So, if we add the 1s delay on both promises, we could call .resume() and know we have about 1s to perform all animations before the recording starts

👍

@karaggeorge
Copy link
Member Author

Added the 1s delay to both the resume and startRecording calls

sindresorhus
sindresorhus reviewed Jul 4, 2020
View reviewed changes
index.js Outdated
@@ -43,7 +44,7 @@ class Aperture {
videoCodec = undefined
} = {}) {
this.processId = getRandomId();
return new Promise((resolve, reject) => {
return new Promise(async (resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's an anti-pattern to use async in new Promise as if the async function rejects, it causes an unhandled rejection. It's better then to use an async-IIFE and try/catch inside it.

sindresorhus
sindresorhus suggested changes Jul 5, 2020
View reviewed changes
Package.swift Outdated
@@ -15,13 +15,15 @@ let package = Package(
)
],
dependencies: [
.package(url: "https://github.com/wulkano/Aperture", from: "0.2.0")
.package(url: "https://github.com/wulkano/Aperture", from: "0.2.0"),
.package(url: "https://github.com/apple/swift-argument-parser", from: "0.1.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a TODO comment about upgrading this dependency?

Sources/ApertureCLI/notifications.swift Outdated
return notification.userInfo?[name] as? T
}

func getData() -> String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func getData() -> String? {
var data: String? {

Sources/ApertureCLI/notifications.swift Outdated
answer(nil)
}

func answer(_ data: Any?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func answer(_ data: Any?) {
func answer(_ data: Any? = nil) {

and you can drop func answer()

Sources/ApertureCLI/notifications.swift Outdated
var payload = [AnyHashable: Any]()

if data != nil {
payload["data"] = "\(data!)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't force-unwrap. Use guard or if-let. Applies in other places too.

Sources/ApertureCLI/notifications.swift Outdated
data: Any?,
using callback: @escaping (ApertureNotification) -> Void
) {
let responseName = "aperture.\(processId).\(event).response.\(UUID().uuidString)"
Copy link
Contributor

Choose a reason for hiding this comment

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

responseIdentifier would be more correct, I think.

Sources/ApertureCLI/notifications.swift Outdated
}

DistributedNotificationCenter.default().postNotificationName(
.Name("aperture.\(processId).\(event)"),
Copy link
Contributor

Choose a reason for hiding this comment

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

"aperture.\(processId).\(event) is used in three places. I think it could benefit from being moved into a function.

Sources/ApertureCLI/notifications.swift Outdated
sendEvent(processId: processId, event: event, data: nil) { _ in }
}

func sendEvent(processId: String, event: String, data: Any?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func sendEvent(processId: String, event: String, data: Any?) {
func sendEvent(processId: String, event: String, data: Any? = nil) {

and you can drop the above method.

Sources/ApertureCLI/notifications.swift Outdated
payload["data"] = "\(data!)"
}

var observer: Any?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var observer: Any?
var observer: AnyObject?

Sources/ApertureCLI/notifications.swift Outdated
}
}

func sendEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you namespace these methods? (enum with static members)

sindresorhus
sindresorhus reviewed Jul 10, 2020
View reviewed changes
Sources/ApertureCLI/notifications.swift Outdated
)
}

func sendEvent(processId: String, event: String, using callback: @escaping (ApertureNotification) -> Void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

processId should be a Int.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we default it to "main"
Should it be a required arg? Or should it default to some kind of random Int?

@sindresorhus
Copy link
Contributor

Bump

@karaggeorge
Copy link
Member Author

Will be working on this next, when I have some downtime

@sindresorhus
Copy link
Contributor

@karaggeorge Bump :)

Base automatically changed from master to main January 23, 2021 13:46
@YoucefHQ
Copy link

+1, this would be amazing to have for users. Thank you 🙏🏽

@karaggeorge
Copy link
Member Author

Sorry for all the delay 😅
@sindresorhus I updated the dependency versions (since I'm on 10.15 now 🎉 ) and updated based on your comments.

I also updated the js code a bit to match the PR that went in since this was opened, so now the startRecording resolves with nothing, but isFileReady resolves with the file path.

Also, bumped the minimum version to 10.13 since new Aperture is also 10.13

Because of the above two, we probably need a major version bump when publishing.

I'll also work on adding some typings in a separate PR when I get some time

@YoucefHQ
Copy link

@karaggeorge Thank you so much for doing this. I'm a Product Manager, and so many of our users asked us for the ability to pause/stop their recordings. We were able to build it on Windows, but we were waiting for Aperture-Node to support it. So thank you on behalf of our users. 🙏🏽

@karaggeorge
Copy link
Member Author

@YoucefHQ no problem. If you don't mind me asking, what are you using as a windows alternative for aperture-node?

@YoucefHQ
Copy link

YoucefHQ commented Mar 11, 2021 •
edited
Loading

@karaggeorge We use a native library on Windows called ScreenRecorderLib https://github.com/sskodje/ScreenRecorderLib

@sindresorhus sindresorhus changed the title Rewrite using SwiftArgumentParser and DistributedNotificationCenter Require macOS 10.13 and add support for pause/resume Mar 13, 2021
@sindresorhus sindresorhus merged commit 5320ff1 into main Mar 13, 2021
@sindresorhus sindresorhus deleted the add-pause-resume branch March 13, 2021 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@karaggeorge @sindresorhus @YoucefHQ