TypeScript rewrite: Exports window #1016

karaggeorge · 2021-05-23T05:20:45Z

Rewrites main/renderer code for exports window. Also revisited some logic in the Conversion/Export classes created in the Editor rewrite. Fixed some small issues and added a few things:

Retry button on Editor window for failed/canceled exports
Retry option in the export menu in the Exports window
Copy option in the export menu in the Exports window
Better error/cancelation handling in export process

karaggeorge · 2021-05-23T05:22:10Z

renderer/components/editor/conversion/conversion-details.tsx

@@ -1,10 +1,10 @@
 import {UseConversionState} from 'hooks/editor/use-conversion';

 const ConversionDetails = ({conversion}: {conversion: UseConversionState}) => {
-  const message = conversion?.message ?? 'Loading…';
+  const message = conversion?.message || 'Loading…';


This is because message is initialized with '' in the main process.

I would add a code comment so we don't try to revert to ?? in the future.

But wouldn't it be better to not initialize it with ''?

Yeah that's fair. Mostly I just didn't want to make it nullable. But I could also initialize it with 'Loading' since that's the value it's set to in the UI when it's empty

karaggeorge · 2021-05-23T05:23:40Z

main/conversion.ts

+    public readonly format: Format,
+    public readonly options: ConversionOptions
+  ) {
+    // eslint-disable-next-line constructor-super


This is to silence a warning because of the TypedEmitter annotation. Couldn't find a way around it.

Warning said this doesn't need a super() call, but obviously without one we get a compilation error

karaggeorge · 2021-05-23T05:25:28Z

renderer/components/exports/export.tsx

+      label: 'Retry',
+      click: () => retry()
+    }].filter(Boolean));
+  }, [canRetry, retry, openInEditor]);


Not sure about text/order/dividers for this. It's the context menu in the Exports window. At the moment it can have one of three states:

Open Original

Open Original Copy

Open Original Retry

I think it should be:

Open Original

Copy ------ Open Original

Retry ------ Open Original

sindresorhus · 2021-05-23T07:43:14Z

Probably not related to this PR, but I noticed there's no "Copy" button when converting to MP4, while there is when converting to GIF.

sindresorhus · 2021-05-23T07:47:55Z

I got this when choosing "Open original" in the export history:

Call Stack
tryCatch
../node_modules/regenerator-runtime/runtime.js (45:14)
Generator.invoke [as _invoke]
../node_modules/regenerator-runtime/runtime.js (274:0)
Generator.prototype.<computed> [as next]
../node_modules/regenerator-runtime/runtime.js (97:0)
asyncGeneratorStep
../node_modules/@babel/runtime/helpers/esm/asyncToGenerator.js (3:0)
_next
../node_modules/@babel/runtime/helpers/esm/asyncToGenerator.js (25:0)
eval
../node_modules/@babel/runtime/helpers/esm/asyncToGenerator.js (32:0)
new Promise
<anonymous>
eval
../node_modules/@babel/runtime/helpers/esm/asyncToGenerator.js (21:0)
CallbacksRegistry.apply
electron/js2c/renderer_init.js (107:792)
<unknown>
electron/js2c/renderer_init.js (83:4787)
<unknown>
electron/js2c/renderer_init.js (83:4528)
EventEmitter.<anonymous>
electron/js2c/renderer_init.js (103:872)
EventEmitter.emit
events.js (310:20)
Object.onMessage
electron/js2c/renderer_init.js (91:818)

sindresorhus · 2021-05-23T07:49:15Z

../node_modules/@babel/runtime/helpers/esm/asyncToGenerator.js (21:0)

Why is Babel transpiling async functions? Electron supports them natively.

sindresorhus · 2021-05-23T07:53:08Z

The "export completed" screen needs a way to show the file in Finder. In the export history, you can click the list item to do so. I also think the "export completed" screen should have a way to "Open original".

The "export completed" screen should indicate what file format I just exported. I didn't remember whether it was GIF or MP4.

The "export completed" screen should show the actual GIF instead of a static thumbnail.

Drag and drop should also work from the "export completed" screen.

sindresorhus · 2021-05-23T07:38:44Z

main/windows/kap-window.ts

+  //   if (!this.browserWindow.isDestroyed()) {
+  //     callback();
+  //   }
+  // };


Intention to leave it like this?

This was throwing an error that it was unused but I wasn't sure if it'd get used in the future

sindresorhus · 2021-05-23T07:54:21Z

renderer/components/exports/export.tsx

+      label: 'Retry',
+      click: () => retry()
+    }].filter(Boolean));
+  }, [canRetry, retry, openInEditor]);


I think it should be:

Open Original

Copy ------ Open Original

Retry ------ Open Original

karaggeorge · 2021-05-23T14:40:23Z

Can we copy MP4? I thought you can only have gif and apng in your clipboard

I'll look into that error and Babel

The conversion preview is actually just the video element that plays in the editor state, but we just resize and pause it. Should I replace it with the actual result of the conversion (whatever format that may be) and have it play?

sindresorhus · 2021-05-23T15:11:59Z

Can we copy MP4? I thought you can only have gif and apng in your clipboard

Yes. We don't copy the animated GIF directly since the pasteboard doesn't support that. We copy a file URL to it. We can do the same with MP4.

Kap/main/plugins/built-in/copy-to-clipboard-plugin.ts

Lines 6 to 8 in eaf8193

    
           const copyFileReferencesToClipboard = (filePaths: string[]) => { 
        
             clipboard.writeBuffer('NSFilenamesPboardType', Buffer.from(plist.build(filePaths))); 
        
           };

sindresorhus · 2021-05-23T15:13:00Z

Should I replace it with the actual result of the conversion (whatever format that may be) and have it play?

Only if it's GIF, I would say. As we cannot play AV1, for example. It's GIF that is interesting to see a preview of, as you can then determine whether you need to re-export using different settings.

karaggeorge · 2021-05-23T20:31:10Z

@sindresorhus I think I addressed almost everything here. Only question, regarding:

I also think the "export completed" screen should have a way to "Open original".

In Exports, Open Original is opening the file back in the editor. What do you mean by this in the editor screen? We added showing the file in Editor, when you click the preview (dragging should also work now). Does that cover that request? The location is the temp file for most plugins, and the actual file for Save to Disk plugin. But in either case it's the converted file, not the original

sindresorhus · 2021-05-24T09:45:05Z

In Exports, Open Original is opening the file back in the editor. What do you mean by this in the editor screen? We added showing the file in Editor, when you click the preview (dragging should also work now). Does that cover that request? The location is the temp file for most plugins, and the actual file for Save to Disk plugin. But in either case it's the converted file, not the original

Yeah. You're right. It doesn't make sense there since you can just click the "Back" button to get the original in the editor.

sindresorhus · 2021-05-24T09:46:20Z

The new changes look good.

sindresorhus · 2021-05-24T09:49:16Z

I don't think it's clear enough to the user that they can drag and drop the thumbnail or that they can click it to show in Finder.

Maybe we should have a popover the first time the export completed view is shown with a tip about drag and drop and click the thumbnail?

Maybe we should also make the filename clickable (with underline) and make it "open in finder"?

sindresorhus · 2021-05-24T09:51:56Z

In the export history, I noticed that new entries are added to the end. Shouldn't the most recent export be first?

sindresorhus · 2021-05-24T09:52:16Z

.apng doesn't fit in the export completed screen:

sindresorhus · 2021-05-24T09:53:12Z

There's something weird with the aspect ratio in the preview: (Notice how everything is wider)

sindresorhus · 2021-05-24T09:59:37Z

While converting, the circular progress is not that visible. I the overlay we show when hovering over should be there permanently while converting.

karaggeorge · 2021-05-24T16:51:33Z

Maybe we should have a popover the first time the export completed view is shown with a tip about drag and drop and click the thumbnail?

Native dialog with some text? Or do we have a better idea for design?

Maybe we should also make the filename clickable (with underline) and make it "open in finder"?

Is the underline always there, or on hover?

.apng doesn't fit in the export completed screen:

We discussed this with @skllcrn , it was either ellipsis from the start or adding the full name on hover. Ended up going with the latter. So if you hover over you should see the extension.

One more thing we could do is add it to the description:
888 x 722 at 15 FPS -> 888 x 722 GIF at 15 FPS (using the format pretty name)

sindresorhus · 2021-05-24T18:03:21Z

Native dialog with some text? Or do we have a better idea for design?

I don't think it should be a blocking dialog. It's just a tip, not important information. A tooltip or toast would be best. It should only be shown once. This is how I do it in Gifski:

Maybe @skllcrn has opinions about this.

Is the underline always there, or on hover?

I would have it always there, but ask @skllcrn.

We discussed this with @skllcrn , it was either ellipsis from the start or adding the full name on hover. Ended up going with the latter. So if you hover over you should see the extension.

I would still make the default name template fit for all extensions. Just make the window a tiny bit wider.

One more thing we could do is add it to the description:
888 x 722 at 15 FPS -> 888 x 722 GIF at 15 FPS (using the format pretty name)

I think the extension is better.

skllcrn · 2021-05-24T20:05:52Z

Love the drag and drop tooltip!

I don't think underlines are a common pattern on macOS, maybe we could add a way to open the directory from the File menu too?

Regarding seeing the full file name including the extension, making the window slightly larger so that default filenames always fit seems reasonable. That said, a quick search tells me that macOS filenames can be up to 255 characters, so truncation is needed in any case, and hovering to see the full value is a nice affordance.

sindresorhus · 2021-05-24T20:35:20Z

I don't think underlines are a common pattern on macOS, maybe we could add a way to open the directory from the File menu too?

You're right, it's not a common pattern, but I have seen it done sometimes. I cannot find an example right now.

How about this instead; A hamburger menu next to the "Copy" button which would have a "Show in Finder" menu item? It would let us add other things there in the future too.

That said, a quick search tells me that macOS filenames can be up to 255 characters, so truncation is needed in any case, and hovering to see the full value is a nice affordance.

Sure, but most people just use the default template.

skllcrn · 2021-05-24T21:08:45Z

A hamburger menu next to the "Copy" button which would have a "Show in Finder" menu item? It would let us add other things there in the future too.

Sounds good to me!

karaggeorge · 2021-05-25T00:14:17Z

I'll add the menu for those. And in the future, we can add the native share menu on that, and actions from the export plugins. What icon should I use for that?

Also, does that mean we are removing the show in finder when you click on the preview? Or are we doing both

sindresorhus · 2021-05-25T00:19:15Z

Also, does that mean we are removing the show in finder when you click on the preview? Or are we doing both

Both

karaggeorge · 2021-05-27T04:57:23Z

@sindresorhus I implemented the things we mentioned. Not sure about the tooltip styling, just used react-tooltip for it

sindresorhus · 2021-05-27T08:59:36Z

I think the tooltip needs some work. It was not clear to me at first it was even a tooltip as it blended into the background.

Here's what I would do:

Add more shadow to it and lower the intensity of the border.
Make its background darker.
More vertical padding and slightly less horizontal padding inside it.
A tiny bit more border radius on it.

And application => app.

sindresorhus · 2021-05-27T09:01:14Z

I also thought of another thing. I think we should follow the Big Sur style for toolbar buttons:

Meaning, just icons.

sindresorhus · 2021-08-11T23:07:33Z

@karaggeorge Maybe we should just get this merged and then open an issue about the remaining stuff? The remaining stuff is not essential.

karaggeorge · 2021-08-26T16:45:36Z

It's been a while and I honestly don't remember what was left on this. If the state of the PR is working, and no major issues are there, we can merge/ maybe release?

I keep seeing the AV1 issues which were fixed in the previous PR

sindresorhus · 2021-08-26T19:19:12Z

Works well for me.

Rewrite Conversion/Export logic and Exports window

7a8bbd1

karaggeorge commented May 23, 2021

View reviewed changes

karaggeorge and others added 3 commits May 23, 2021 02:23

Fix failing build

c92c059

Rebuild

ab64937

Electron v10 (#1017)

f1b22ae

sindresorhus reviewed May 23, 2021

View reviewed changes

Add GIF preview, format in the title and open on click functionality

3bded86

karaggeorge added 2 commits May 27, 2021 00:56

Add background, icon menu and other small improvements

74a3691

Add tooltip

df43885

sindresorhus changed the title ~~TypeScript Rewrite: Exports Window~~ TypeScript rewrite: Exports Window Aug 26, 2021

sindresorhus changed the title ~~TypeScript rewrite: Exports Window~~ TypeScript rewrite: Exports window Aug 26, 2021

sindresorhus merged commit 777d780 into main Aug 26, 2021

sindresorhus deleted the rewrite-exports branch August 26, 2021 19:19

This was referenced Aug 26, 2021

Improve tip tooltip in exports window #1032

Open

Use macOS 11 style toolbar buttons in export window #1033

Open

TypeScript rewrite: Exports window #1016

TypeScript rewrite: Exports window #1016

Conversation

karaggeorge commented May 23, 2021

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

sindresorhus commented May 23, 2021

sindresorhus commented May 23, 2021 • edited

sindresorhus commented May 23, 2021

sindresorhus commented May 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karaggeorge commented May 23, 2021

sindresorhus commented May 23, 2021

sindresorhus commented May 23, 2021

karaggeorge commented May 23, 2021 • edited

sindresorhus commented May 24, 2021

sindresorhus commented May 24, 2021

sindresorhus commented May 24, 2021

sindresorhus commented May 24, 2021

sindresorhus commented May 24, 2021

sindresorhus commented May 24, 2021

sindresorhus commented May 24, 2021

karaggeorge commented May 24, 2021

sindresorhus commented May 24, 2021

skllcrn commented May 24, 2021 • edited

sindresorhus commented May 24, 2021

skllcrn commented May 24, 2021

karaggeorge commented May 25, 2021

sindresorhus commented May 25, 2021

karaggeorge commented May 27, 2021

sindresorhus commented May 27, 2021 • edited

sindresorhus commented May 27, 2021

sindresorhus commented Aug 11, 2021

karaggeorge commented Aug 26, 2021

sindresorhus commented Aug 26, 2021

sindresorhus commented May 23, 2021 •

edited

sindresorhus commented May 23, 2021 •

edited

karaggeorge commented May 23, 2021 •

edited

skllcrn commented May 24, 2021 •

edited

sindresorhus commented May 27, 2021 •

edited