Implement bulk export#770
Conversation
Adding extra state for cancellation is kind of a PITA but cancellation just waits for the export to finish without it. This is mixing exceptions and empty strings--I'm coming from a C++ background and exceptions kind of scare me, so I tried to avoid them unless it looked like a big diff to avoid them the NoRecordingInProgressErrors had a precedent so I got a bit lazy.
Like the last change, this involves some error softening as we plow through an export
|
@agloo This is amazing work, I'm really impressed by how deeply you were able to understand the codebase to implement a big feature like this. I have one overall comment that would require (hopefully) a small refactor: it looks like this feature depends on the side panel being open. As a user I think I would expect the bulk export to continue regardless of whether the side panel is open or not. My suggestion is:
|
This still tests alright
This state was already in a few other messages, so we just had to add it to CardExportedMessage().
Adds DuplicateNoteError for when a card already exists Adds AudioRequestSupersededError for when the recorder gets preempted
Before this, we'd just display "card exported: x/y" as before I'm also removing the initial notify progress, which would display "card exported: x/y" before we even started exporting.
…ppening This puts the app in a weird state so this just greys out the control in the overlay while a card is recording
killergerbah
left a comment
There was a problem hiding this comment.
Sorry for being so late to review this - lately I am short on time during the weekdays. And thanks for making all the requested changes, I know it probably took a big chunk of time. I think this is really close, hopefully the remaining comments do not take as long to address.
Replace the 'bulk' export mode with a boolean flag and separate publish methods to simplify the branching logic in CardPublisher. Addresses: "Sorry for realizing this so late, but it doesn't seem like a distinct..." "Is there a reason you're writing this instead of..." "I would recommend writing a new public method specifically for bulk export..." "exportMode appears to be unused..." "As mentioned in my comment on model.ts..." "As mentioned elsewhere, I think you should branch here..."
Addresses "Nitpick: it's probably more efficient..."
Addresses "I think in this case fire-and-forget is fine..."
Addresses "Also: if CardExportedMessage.exportError is..."
Responds to: "In this case, would we be exporting a card..." "Was this change necessary? From what I can tell, re-record..."
Addresses "This if -> throw block can be moved into _executeAction" and my response to it
agloo
left a comment
There was a problem hiding this comment.
Thanks for your patience!
|
|
||
| handle(request: any, sender: any, sendResponse: any): boolean { | ||
| // Check if we already processed this cancellation to prevent duplicate processing | ||
| if (this._cardPublisher.bulkExportCancelled) { |
There was a problem hiding this comment.
Yup, that setter has the same pattern and without side effects from these functions I agree that we should just set the bool. I had this up thinking about the potential for data races (e.g. you have a cancel in flight as you start another bulk export), but these seem benign enough that I'll just make it a public bool and set it directly here for simplicity.
killergerbah
left a comment
There was a problem hiding this comment.
Hey thanks so much for continuing to iterate with me. Just some minor comments left.
Code review aside, we also get to remove some logic in the BulkExportController
killergerbah
left a comment
There was a problem hiding this comment.
Hey want to thank you again for building this and pushing it all the way through. It's definitely not easy to understand a new codebase, especially one written by me, and then make a significant change to it. Users have been asking for this for a while, so they will be very happy :). Looking forward to your future contributions, although I'm sure you are quite busy!
|
Thanks for your work; I really appreciate it! I'm also a programmer, and I’ve been one for a year. I have to say, I’m not able to do such excellent work. |
New functionality:
This adds a button to the side panel overlay between
Download Subtitles as SRTandMining Historythat starts exporting one card after another from the current subtitle to the end of the video. While it does this, it blocks out the side panel with a cancellable progress bar to avoid weird state if you try to seek around/kick off an export.Architecture:
This driven by a queue, with events being consumed by logic in SidePanel. SidePanel will kick off the export of the next card every time it gets a card-exported message back.
I found that this put the extension into some weird states, most notably that I was getting a lot of pausing-stopped-video and empty-card-created errors. I downgraded those from exceptions to a no-op and empty cards, respectively.
I tested this almost exclusively on one YouTube video--happy to look at whatever else you'd like to see covered.