1517728 - Export Console content to file

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

Bug 644412 adds the ability to save the content to the clipboard. We could have another context menu entry to save to a file.

We may want to group those 2 entries into one and have a sub menu

Comment 1

[Jefry Lagrange]

Attachment: Bug1517728.patch

Hi Nicolas. I thought I would ask for some feedback before I continue working on this, to make sure I'm going in the right path.

I want to leave showFilePicker general so we can use it in the style editor and not duplicate code that way. For the other parts there are some decisions that need to be made with regards of the extension of the file, whether the last file saved should be remembered so it is automatically selected a if the user wants to save the file a second time, the title of the file picker window, the default name if any of the file, etc.

Thanks!

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9044582 [details] [diff] [review]
Bug1517728.patch

Review of attachment 9044582 [details] [diff] [review]:
-----------------------------------------------------------------

Hello Jefry, 

Thanks for working on this!
For now, I think we shouldn't copy everything the StyleEditor is doing, and we should tailor showFilePicker to the console need (we can modify it later, let's keep it simple for now).

::: devtools/client/webconsole/utils/context-menu.js
@@ +203,5 @@
> +        const webconsoleOutput = parentNode.querySelector(".webconsole-output");
> +        const istream = converter.convertToInputStream(webconsoleOutput.textContent);
> +        DevToolsUtils.saveFileStream(returnFile, istream);
> +      }
> +      // TODO: Which filter is better than *? Which default name if any? Which path? 

The filter is fine I think (we want people to name it as they want).
A default name like `console-export-${YYYY-MM-DD_hh-mm-ss}.txt` would be nice.

::: devtools/shared/DevToolsUtils.js
@@ +813,5 @@
> + *        Optional nsIFile or string representing the filename to auto-select.
> + * @param nsIWindow parentWindow
> + *        Optional parent window. If null the parent window of the file picker
> + *        will be the window of the attached input element.
> + * @param callback

It would be more consistent with the rest of the codebase if the function would return a Promise (or be an async function).

@@ +823,5 @@
> + *        Optional name shown to the user that describes the file filter.
> + * @param AString fileFilter
> + *        Optional the filter used to specify file type.
> + */
> +// TODO: use this as the filepicker for the style editor.

we should file a Bug to do it, and reference it from here.

@@ +824,5 @@
> + * @param AString fileFilter
> + *        Optional the filter used to specify file type.
> + */
> +// TODO: use this as the filepicker for the style editor.
> +exports.showFilePicker = function(path, toSave, parentWindow, callback,

since it's a new file, and we are only calling it from one place, I think we can remove some of those params. If we need them in the future, we'll update the function, but for now, let's only make it match what we need in the console.

- `path` could be removed, since we pass `null` from the console (and it will simplify the code a lot)
- `toSave`  can be removed since we pass `true` from the console

@@ +871,5 @@
> +  }
> +
> +  // TODO: getString is used to get the l10 string. Which string
> +  // would be appropiate?
> +  fp.init(parentWindow, fileFilterName + ".title", mode);

I'm not sure what this is doing?
Also, shouldn't we check that filterFilterName and fileFilter are defined before adding this filter?

Comment 3

[Jefry Lagrange]

Attachment: Bug 1517728 - Export Console content to file. r=nchevobbe.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

How is it going Jefry?

Comment 5

[Jefry Lagrange]

Testing the filepicker is the most challenging part. I'm still stuck on it.

Comment 6

[Jefry Lagrange]

Hello Nicolas, so now I have managed to open the file picker in mochitest and save the file. My problem now is that when the file is saved, it appears it isn't flush immediately to disk so when I try to open it to verify that it has all of the output of the console, it gives me an error that it can't find the file.

Do you have an idea what could be causing this timing issue?

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Hello Jefry.
Could you post your patch somewhere so I can give it a try and check what is going on? Thanks!

Comment 8

[Jefry Lagrange]

Attachment: Find out why file is read before it is written

Hi Nicolas, I have attached the mochitest as I'm reworking it. You will find that in the exportAllToFile function, the issue is that the file is being read before it is written. How do I make sure it is flushed?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Hello Jefry, thanks for posting your patch, that was helpful!

So, I was able to get the text of the file with those 2 functions:

async function exportAllToFile(hud, message) {
  const menuPopup = await openContextMenu(hud, message);
  const exportMenu = menuPopup.querySelector("#console-menu-export");

  const view = exportMenu.ownerDocument.defaultView;
  EventUtils.synthesizeMouseAtCenter(exportMenu, {type: "mousemove"}, view);
  const exportFile = menuPopup.querySelector("#console-menu-export-file");
  ok(exportFile, "copy menu item is enabled");

  var nsifile   = new FileUtils.File('/tmp/test_me.log');
  MockFilePicker.setFiles([nsifile]);
  EventUtils.synthesizeMouseAtCenter(exportFile, {}, view);
  return getFileContents('file:///tmp/test_me.log');
}

function getFileContents(file) {
  return new Promise((resolve, reject) => {
    const channel = NetUtil.newChannel({
      uri: NetUtil.newURI(file),
      loadUsingSystemPrincipal: true,
    });
    NetUtil.asyncFetch(channel, function(inputStream, status) {
      if (Components.isSuccessCode(status)) {
        info("Fetched downloaded contents.");
        resolve(NetUtil.readInputStreamToString(inputStream, inputStream.available()));
      } else {
        reject();
      }
    });
  });
}

it gives me the following in the terminal:

 console.log: ====================================================================================================
 console.log: hello test.js:4:17
 myObject:
 Object { a: 1 }
  myArray:
 Array [ "b", "c" ]
 test.js:5:17
 Error: "error object"
     wrapper http://localhost:52054/test.js:6
     logStuff http://localhost:52054/test.js:10
 test.js:6:17
 console.trace() myConsoleTrace test.js:7:17
     wrapper http://localhost:52054/test.js:7
     logStuff http://localhost:52054/test.js:10
 world ! test.js:8:17
 console.log: ====================================================================================================

(looks good to me :) )

---

So what was wrong:

• in context-menu.js, we were using webconsoleOutput.textContent instead of getElementText(webconsoleOutput) (hence losing all the line ending)
• in the test, we were checking that the submenu item was in the DOM tree, and then calling click on it. But it wasn't visible on screen, so the click call wasn't doing anything. In my version, I hover the Export to item, so display the sub item, and then I click on the item with synthesizeMouseAtCenter
• the read function you had was throwing for some reason. I copied the getFileContents from a test in the JSONViewer, and it looks good now.

Hope this helps!

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

(BTW, the export to clipboard test was also failing, I guess because we didn't actually display the sub menu)

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Hello Jefry,

Can I help you push this over the finish line? It'd be awesome to have this feature in the console :)

Comment 12

[Jefry Lagrange]

I will try to finish it up this week or the week next. As far as I can see it is just the integration test that need fixing. I will try to get it done.

Comment 13

[Jefry Lagrange]

hmmm.... .Ok so I think there's a confusion with the code. I think this was what happened.

You ran the code, got the error of the read() function (by this time the file was created)

Replaced it getFileContents function (it reads correctly the previously created file)

In order to correctly test this, you must delete the file in the tmp directory each time you run the tests otherwise you won't get the error.

Testing it further, I think I still have the same error. By the time firefox writes the file, I can't read it. Please check it on your end to see if it happens.

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1517728 - Export console output to file.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

I think you're right Jefry.
So I took another look, and this seems to work https://phabricator.services.mozilla.com/D31040 .
Feel free to copy the interesting bits into your patch :)

Also, we may have a different test file for the export to file, and not reuse the one for the clipboard (or rename the test to reflect what it does).

Comment 16

[Jefry Lagrange]

Thanks Nicolas!, I have updated the phab diff with all the changes. I think everything works now.

Comment 17

[Jefry Lagrange]

Hi, Nicolas, I think everything is fine. I'm having problems pushing to the try server. Could you push the patch for me?

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Jefry, I still see a windows error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=929ac6169d845ff5fcf54aacc830a41a72345dc9&selectedJob=249670580

I think that's because how we split to get the lines.
I tried to fix this and pushed to TRY again https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffed474d7579aef673cf9e6e8f9a1017ae8e6245 (here's the fix: https://hg.mozilla.org/try/rev/8254ff06c2faf5eb0cf5d8f7f9db3ad583ebb3b6)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0883abbe6a6f
Export Console content to file. .

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0883abbe6a6f