Add screenshot command #17

WasabiFan · 2017-08-18T05:43:57Z

This implementation drops screenshots in a folder called ev3dev-screenshots and opens the newly-captured image in the viewer. We should discuss if we instead want to adopt a different behavior, e.g. saving it to a temporary file. The reason I chose this approach is that you can't easily retrieve the file with just an open preview window.

Also, the current implementation will get really unhappy if you don't have any workspace open. We need to figure out what to do there.

Fixes #4

WasabiFan · 2017-08-18T05:46:12Z

p.s.: force-pushing is bad! 👎

dlech · 2017-08-18T17:53:08Z

p.s.: force-pushing is bad!

Yes, I should have mentioned that to you, sorry. There is a good reason for it in this case.

dlech · 2017-08-18T17:59:36Z

Personally, I would like to be presented with a "save as" dialog when uploading a screenshot. I don't want to pollute my project directory with screenshots. It would also fix the problem of not having a workspace open.

dlech · 2017-08-18T18:01:34Z

src/extension.ts

@@ -508,6 +516,39 @@ class Device extends vscode.TreeItem {
            [this.shellPort.toString()]);
        term.show();
    }
+
+    captureScreenshot(): void {


Can we make this async?

I suppose we could... why? We don't wait for completion regardless because this captures the screenshot and invokes a user action without returning any data.

I prefer it over .then() for readability and easier handling of exceptions

dlech · 2017-08-18T18:06:22Z

src/extension.ts

+                    // VSCode will only recognize the resource properly if using the "file://" scheme
+                    // Forward slashes are required, even on Windows
+                    const parsedPath = path.parse(fileName);
+                    vscode.commands.executeCommand('vscode.open', vscode.Uri.parse(`file:///${parsedPath.dir.replace(/\\/g, '/')}/${parsedPath.base}`));


Can we use vscode.Uri.file() instead?

Huh, that seems like what I was looking for.

WasabiFan · 2017-08-18T19:00:52Z

Personally, I would like to be presented with a "save as" dialog when uploading a screenshot. I don't want to pollute my project directory with screenshots. It would also fix the problem of not having a workspace open.

I agree, however I wasn't able to find "save as" functionality. Do you know of such an option?

dlech · 2017-08-18T19:08:30Z

One interesting approach I saw was to us the HTML preview feature of VS Code.

Basically, you implement a web server in your extension and display a page using the HTML preview that talks to your extension/server. They you can do anything a web page can do.

You could also use an input box to get the path name. Maybe there is some nodejs package out there that can get us a OS native Save As... dialog that we could hook up to a button on the input box?

dlech · 2017-08-18T19:40:12Z

Here's another thing you could try:

Save to a temporary file.
Open the temporary file.
vscode.commands.executeCommand('workbench.action.files.saveAs');

WasabiFan · 2017-08-18T20:19:26Z

One interesting approach I saw was to us the HTML preview feature of VS Code.

The frustrating part is that as soon as you start using an HTML page you are forced to implement all your UI to make it not look terrible. I'm also doubtful that they'd let us open dialogs from there.

You could also use an input box to get the path name. Maybe there is some nodejs package out there that can get us a OS native Save As... dialog that we could hook up to a button on the input box?

Hmmm... it's worth a shot! I'm worried that the sandbox they host extensions in will block this or make the behavior weird.

Here's another thing you could try:

I like that approach. Where'd you see that command documented?

WasabiFan · 2017-08-18T20:24:25Z

Yes, I should have mentioned that to you, sorry. There is a good reason for it in this case.

OK, sounds good. If there's a security issue (e.g. passwords accidentally included) let me know and I can strip that out of my branch.

dlech · 2017-08-18T20:28:43Z

I messed up the git LFS and I didn't want to have an extra >10MB in history forever for no reason. 😉

dlech · 2017-08-18T20:30:32Z

Where'd you see that command documented?

In the vscode source. Basically it is the same as clicking File > Save As...

WasabiFan · 2017-08-18T20:34:14Z

I messed up the git LFS and I didn't want to have an extra >10MB in history forever for no reason. 😉

That'd certainly explain why I had to manually reconcile merge issues with the OSX binary... I just downloaded the one from master and clobbered my local copy during the merge 😆

In the vscode source. Basically it is the same as clicking File > Save As...

Ooh, sneaky...

WasabiFan · 2017-08-19T03:07:38Z

Updated. That undocumented command worked (surprisingly 😆) so I think this should behave pretty well. I'd appreciate some testing on other OSes. And of course, the new functionality hasn't been released in fbgrab yet.

WasabiFan · 2017-08-19T04:55:43Z

Rebased on master to remove the broken history.

dlech · 2017-08-19T19:36:02Z

src/extension.ts

+                vscode.commands.executeCommand('vscode.open', vscode.Uri.file(screenshotFile));
+
+                // Delay to allow viewer to open
+                setTimeout(() => vscode.commands.executeCommand('workbench.action.files.saveAs'), 100);


Hmmm... delays like this are never good. If someone is on a really slow computer it won't work right.

I'm thinking maybe we just display the file and users can click File > Save As... if they want to save it somewhere else.

I also found that I can click and drag the image to other programs, so I probably won't use the File > Save As... myself, so it would just be getting in my way for me.

Also, what happens to the temporary file? It kind of looks like we might be tracking them, but it isn't quite obvious. Are the files being deleted when the extension unloads?

I'm thinking maybe we just display the file and users can click File > Save As... if they want to save it somewhere else.

Now that we know the "save as" option works, I think this would be pretty nice.

Also, what happens to the temporary file? It kind of looks like we might be tracking them, but it isn't quite obvious. Are the files being deleted when the extension unloads?

Yeah, the theory behind node-temp is that once you call track any files that were created will be deleted once the process exits (IDE reloads or closes). I haven't validated that this happens properly in the VSCode environment yet.

dlech · 2017-08-19T19:38:16Z

src/utils.ts

+    return `${d.getFullYear()}-${pad(d.getMonth())}-${pad(d.getDay())}-${pad(d.getHours())}-${pad(d.getMinutes())}-${pad(d.getSeconds())}`;
+}
+
+let tempDirs: { [sharedKey: string]: string } = {};


this can be const

dlech · 2017-08-19T19:45:46Z

src/extension.ts

+            });
+
+            writeStream.on('close', () => {
+                vscode.window.setStatusBarMessage(`Screenshot successfully captured`);


I don't think we need a message in the status bar. When the file opens, it will be obvious that it was successful.

And when it doesn't? I want the status bar message there to help us debug if/when the image doesn't actually open. That's the great thing about the status bar: it doesn't need to be added/removed and it doesn't take up additional space!

When it doesn't, we should be showing a prominent error message.

We should be, yes. And if either we make a mistake or VSCode has an issue, leaving the user without any sort of feedback as to what went on is a Bad Experience (TM).

Well, just don't make mistakes. 😈

(just kidding, of course)

dlech · 2017-08-19T19:48:48Z

src/utils.ts

+    return new Promise((resolve, reject) => {
+        temp.track();
+        temp.mkdir(sharedKey, (err, dirPath) => {
+            if(err) {


nit: space after if. here and above

dlech · 2017-08-19T19:51:40Z

src/extension.ts

@@ -508,6 +517,34 @@ class Device extends vscode.TreeItem {
            [this.shellPort.toString()]);
        term.show();
    }
+
+    async captureScreenshot() {
+        const error = (message) => {


I see this is only used once. Why assign it to a variable?

Because it used to be used twice. 😉

dlech · 2017-08-19T19:54:29Z

src/extension.ts

+            writeStream.on('close', () => {
+                vscode.window.setStatusBarMessage(`Screenshot successfully captured`);
+                vscode.commands.executeCommand('vscode.open', vscode.Uri.file(screenshotFile));
+


We should also handle writeStream.on('error', ....

dlech · 2017-08-19T21:11:03Z

src/extension.ts

            writeStream.on('open', () => {
                conn.stdout.pipe(writeStream);
            });

            writeStream.on('close', () => {
-                vscode.window.setStatusBarMessage(`Screenshot successfully captured`);
-                vscode.commands.executeCommand('vscode.open', vscode.Uri.file(screenshotFile));
+                if(!anyErrors) {


can we use 'finish' instead of 'close' for success so we don't have to have the anyErrors variable?

🤔 It's been a while since the last time I worked with streams in detail and they've changed a bit since then, so my recollection is a bit fuzzy. I believe that you can still flush all data and close the stream even if errors were raised, as the error event can be raised arbitrarily by the stream source. I couldn't find any documentation which made me confident that there was an event for "finished and no errors were thrown", but I'd be happy to switch and remove the ugly flag if we can find such a thing.

I'm pretty sure the 'finish' event will do this (only fire when done and no errors). Why don't you try it out and see what happens?

How would you suggest I test it? I've been experimenting with unplugging the Wi-Fi dongle which lets me cause an initial failure to connect, but the screen is so small that timing it to interrupt the transfer is tough.

I wouldn't worry about simulating the error. If the 'finish' event fires at the right time, let's just use it.

OK, what about destroying the stream or removing the 'finish' event handler when there is an error?

Also, I'm wondering... even if there is an error and we get to 'finish', then don't we have the entire file and it can be displayed?

OK, what about destroying the stream or removing the 'finish' event handler when there is an error?

I can look into these alternatives.

Also, I'm wondering... even if there is an error and we get to 'finish', then don't we have the entire file and it can be displayed?

If the client disconnects part-way through the download, we have half of a PNG file. That half of the PNG file is all of the data sent through the channel, but not all the data that fbgrab had to send.

If the client disconnects part-way through the download, we will never get a 'finish' event because it never sees EOF

If the client disconnects part-way through the download, we will never get a 'finish' event because it never sees EOF

What makes you think that?

Relevant: nodejs/node#13812

The linked issue highlights that the expected (and current) behavior of streams is that finish is called despite errors. It also acknowledges that errors will be raised before finish (after that issue was patched) so we certainly can react to the error event to prevent functionality in finish.

dlech · 2017-08-19T21:12:36Z

src/utils.ts

+
+const tempDirs: { [sharedKey: string]: string } = {};
+export function getSharedTempDir(sharedKey: string): Promise<string> {
+    if(tempDirs[sharedKey]) {


nit: space after if here too

dlech · 2017-08-20T00:40:19Z

src/utils.ts

+    }
+
+    return new Promise((resolve, reject) => {
+        temp.track();


for style points, we could call temp.cleanupSync() when the extension exits (via deactivate())

It is possible to unload and reload the extension without the process actually exiting.

Earlier I confirmed that it cleared the temp file properly when closing the window or reloading the session/switching folders.

It is possible to unload and reload the extension without the process actually exiting.

The main VSCode process doesn't exit, but the extension host does. Extensions are run in an independent sandboxed process which is restarted when you reload the window.

dlech · 2017-08-20T00:41:58Z

package.json

@@ -182,6 +192,7 @@
        "dbus-native": "^0.2.2",
        "dnode": "^1.2.2",
        "ssh2": "~0.5.5",
-        "ssh2-streams": "~0.1.19"
+        "ssh2-streams": "~0.1.19",
+        "temp":"^0.8.3"


missing space after :

dlech · 2017-08-20T00:42:34Z

package.json

    }
-}
+}


don't delete newline at the end of the file

npm install automatically formats this file and adds it back in.

dlech · 2017-08-20T00:52:43Z

src/extension.ts

+            writeStream.on('close', () => {
+                if (!anyErrors) {
+                    statusBarMessage.text = `Screenshot "${screenshotBaseName}" successfully captured`;
+                    vscode.commands.executeCommand('vscode.open', vscode.Uri.file(screenshotFile));


when testing this out, I forgot to update fbgrab, so I got an empty text file displayed instead of a screenshot.

If you want to be fancy, you can check for writeStream.bytesWritten == 0 here and not show the file.

Huh, good idea... I could probably even get fancier and check for the initial bytes of the PNG header (IIRC it literally starts with the ASCII text PNG). If it isn't a PNG, a warning could be displayed suggesting you may need to upgrade fbcat.

dlech · 2017-08-20T01:59:24Z

BTW, there is a fbcat 0.5.0 package now.

dlech · 2017-08-20T02:59:17Z

Just thought of one more thing. Please add an entry to the changelog.

WasabiFan · 2017-08-20T06:04:59Z

Updated. We now remove the finish handler when an error occurs. Also, it validates that the file was saved correctly and complains if it wasn't instead of showing the file.

WasabiFan · 2017-08-20T07:04:54Z

I just updated this PR to add a status bar helper. My hope is that we can implement this pattern for other actions. Here's the flow:

As an action is in-progress, this new helper lets us signal that things are happening in the background and make it clear that the action wasn't a no-op. If the action fails, you hide the message and present an error dialog; if it succeeds, you set the message to a success indication and this helper displays the message for a certain delay before removing it.

I like this because it doesn't clutter the interface, yet it gives you something concrete to tell you that the action is in-progress. It goes away once things are finished so that you aren't left with annoying messages in the UI. I was a bit reluctant to implement any status message previously because the dispose logic and timeout were ugly, but I think the consuming syntax is nice now 😃 If you like it, I think I'll open a PR to update other asynchronous actions to this pattern so that the same feeling is present everywhere.

WasabiFan · 2017-08-23T01:58:35Z

@dlech Is there anything else you were looking to change here? I'd like to get this merged so I can use the utilities I added in this PR for further work.

dlech · 2017-08-23T14:17:47Z

You have made significant changes since the last round of review and I have not had a chance to look at it again.

Conventional wisdoms says: please don't add significant changes to patch series (or pull requests), especially after several rounds of review, as it makes more work for the reviewer and slows down the review process (paraphrasing common advice from the Linux kernel mailing list).

dlech · 2017-08-23T14:19:15Z

This is probably OK, but I would like to test your changes again before merging. In the mean time, if you want to squash/rebase this into a couple of logical commits, that would be helpful.

WasabiFan · 2017-08-23T15:13:58Z

OK, sounds good. I would certainly like to get some testing on other platforms.

Conventional wisdoms says: please don't add significant changes to patch series (or pull requests), especially after several rounds of review, as it makes more work for the reviewer and slows down the review process (paraphrasing common advice from the Linux kernel mailing list).

As I was making changes in response to your feedback, it's hard to say that I was doing something out-of-the-ordinary... after all, we don't want a half-implemented feature merged just so that we can limit the number of reviews. Personally, I like it when contributors/teammates make significant changes after I give them feedback, because it means that my thoughts helped them to improve their code or think about the problem in a way they hadn't previously.

dlech · 2017-08-23T15:43:41Z

In this particular case, I was referring to 1686e0a. We had already reached a point where we had something that worked and we were both happy with it. Then you added that commit, which is a pretty significant addition and not something that I requested. It's no big deal, but now you have to just wait until I have time to look at the whole thing again. It would have been easier on me if you saved that commit for a separate PR since it is a feature that can stand on it's own.

dlech

Looking good. There are mainly just some style issues that I would like to get cleaned up.

dlech · 2017-08-24T14:58:04Z

package.json

@@ -75,6 +75,11 @@
                "category": "ev3dev"
            },
            {
+                "command": "ev3devBrowser.captureScreenshot",


Running from the command palette is actually broken here because there is no parameter passed as d in

vscode.commands.registerCommand('ev3devBrowser.captureScreenshot', d => ev3devBrowserProvider.captureScreenshot(d))

But some of the other commands are currently broken for the same reason, so we can leave this as is and fix them all later (I have a plan...).

Just looked at this again and this command should not be showing up in the command palette. Not sure what's up. Perhaps a vscode bug.

dlech · 2017-08-24T15:00:19Z

src/extension.ts

@@ -1,11 +1,21 @@
-import * as dnode from 'dnode';
+import * as dnode from 'dnode'


Actually, I would prefer to have a semicolon at the end of each line here. I'm not sure why it was missing from so many of the import lines.

dlech · 2017-08-24T15:04:28Z

src/extension.ts

+                }
+                else {
+                    handleCaptureError("The captured screenshot was not in the expected format."
+                        + " This may be caused by running an old version of 'fbcat' on the remote device; try updating the fbcat package.");


This works. 😄

It could be more specific though and say that version that we need fbcat 0.5.0.

Also, on my 13" macbook, this message gets cut off because it is so long. The full message is visible in a tooltip, but it is a bit hard to read.

Suggestion: you could make the message much shorter and then include a "More Info..." button that links to a wiki page or displays a second info box with the details.

Another thing to consider is that 6 months from now, it is unlikely that anyone will still be using an image with an outdated fbcat package.

How about this: "The screenshot was not in the correct format. You may need to upgrade to fbcat 0.5.0."

Sounds good to me.

dlech · 2017-08-24T15:04:47Z

src/extension.ts

+
+            writeStream.on('finish', async () => {
+                const pngHeader = [ 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A ];
+                if(await verifyFileHeader(screenshotFile, pngHeader)) {


style: space after if

dlech · 2017-08-24T15:05:35Z

src/utils.ts

@@ -0,0 +1,111 @@
+import * as vscode from 'vscode'


nit: semicolons at the end of every statement

dlech · 2017-08-24T15:10:31Z

src/utils.ts

+     * @param newMessage The new message to display
+     */
+    public update(newMessage: string) {
+        if(!this._statusBarItem) {


style: space after if

dlech · 2017-08-24T15:10:37Z

src/utils.ts

+
+    constructor(initialMessage?: string) {
+        this._statusBarItem = vscode.window.createStatusBarItem();
+        if(initialMessage) {


style: space after if

dlech · 2017-08-24T15:13:39Z

src/extension.ts

+    async captureScreenshot() {
+        const statusBarMessage = new StatusBarProgressionMessage("Attempting to capture screenshot...");
+
+        const handleCaptureError = (e) => {


style: no () around single lambda parameter

dlech · 2017-08-24T15:20:14Z

src/utils.ts

+        }
+    }
+
+    private _dispose() {


style: don't really need the leading underscore in typescript since we have the access modifier to tell us this is private.

dlech · 2017-08-24T15:20:56Z

src/utils.ts

+}
+
+export class StatusBarProgressionMessage {
+    private _statusBarItem: vscode.StatusBarItem;


style: don't really need the leading underscore in typescript since we have the access modifier to tell us this is private.

dlech · 2017-08-24T15:36:48Z

package.json

@@ -75,6 +75,11 @@
                "category": "ev3dev"
            },
            {
+                "command": "ev3devBrowser.captureScreenshot",
+                "title": "Take Screenshot",
+                "category": "ev3dev"


OK, it looks like assigning "category" here causes this command to show up in the command palette. Let's omit this line for now.

WasabiFan · 2017-08-26T05:51:00Z

@dlech Updated and rebased.

WasabiFan · 2017-08-26T06:14:27Z

Err, modified the wrong package.json entry. Will update.

WasabiFan · 2017-08-26T06:45:04Z

Fixed.

dlech · 2017-08-26T14:48:57Z

Woo hoo. I'm now taking bets on how many merge conflicts I will have in my local branch. Any guesses?

WasabiFan · 2017-08-26T16:30:01Z

Haha, I was wondering about that... I'm betting on 14 individual conflicts 😛

dlech · 2017-08-26T17:03:06Z

I didn't count exactly, but I think you are pretty darn close.

dlech reviewed Aug 18, 2017

View reviewed changes

WasabiFan force-pushed the screenshot-command branch from b5a7a83 to 0f0760d Compare August 19, 2017 04:55

dlech requested changes Aug 19, 2017

View reviewed changes

dlech reviewed Aug 19, 2017

View reviewed changes

dlech reviewed Aug 20, 2017

View reviewed changes

Implement "screenshot" command

89c36d8

WasabiFan force-pushed the screenshot-command branch from ad0ca11 to 1e1173b Compare August 24, 2017 04:22

dlech requested changes Aug 24, 2017

View reviewed changes

dlech reviewed Aug 24, 2017

View reviewed changes

WasabiFan force-pushed the screenshot-command branch from 1e1173b to bbef09f Compare August 26, 2017 05:50

WasabiFan force-pushed the screenshot-command branch from bbef09f to 78101e2 Compare August 26, 2017 06:44

WasabiFan added 2 commits August 25, 2017 23:46

Implement+use helper for friendly status bar progressions

194c549

Add changelog entry for screenshot command

0098a88

WasabiFan force-pushed the screenshot-command branch from 78101e2 to 0098a88 Compare August 26, 2017 06:46

dlech approved these changes Aug 26, 2017

View reviewed changes

dlech merged commit 4dbb822 into ev3dev:master Aug 26, 2017

WasabiFan mentioned this pull request Aug 29, 2017

Add Travis CI config #28

Merged

		@@ -1,11 +1,21 @@
		import * as dnode from 'dnode';
		import * as dnode from 'dnode'

Add screenshot command #17

Add screenshot command #17

Conversation

WasabiFan commented Aug 18, 2017

WasabiFan commented Aug 18, 2017

dlech commented Aug 18, 2017

dlech commented Aug 18, 2017

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

WasabiFan commented Aug 18, 2017

dlech commented Aug 18, 2017

dlech commented Aug 18, 2017 • edited Loading

WasabiFan commented Aug 18, 2017

WasabiFan commented Aug 18, 2017

dlech commented Aug 18, 2017

dlech commented Aug 18, 2017

WasabiFan commented Aug 18, 2017

WasabiFan commented Aug 19, 2017

WasabiFan commented Aug 19, 2017

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

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

dlech commented Aug 20, 2017

dlech commented Aug 20, 2017

WasabiFan commented Aug 20, 2017

WasabiFan commented Aug 20, 2017

WasabiFan commented Aug 23, 2017

dlech commented Aug 23, 2017

dlech commented Aug 23, 2017

WasabiFan commented Aug 23, 2017

dlech commented Aug 23, 2017

dlech left a 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

WasabiFan commented Aug 26, 2017

dlech commented Aug 18, 2017 •

edited

Loading

WasabiFan commented Aug 26, 2017 •

edited

Loading