Closed Bug 1517728 Opened 1 year ago Closed 9 months ago

Export Console content to file

Categories

(DevTools :: Console, enhancement, P3)

65 Branch
enhancement

Tracking

(firefox69 fixed)

RESOLVED FIXED
Firefox 69
Tracking Status
firefox69 --- fixed

People

(Reporter: nchevobbe, Assigned: jefry.reyes)

References

Details

Attachments

(3 files, 1 obsolete file)

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
Severity: normal → enhancement
Assignee: nobody → jefry.reyes
Attached patch Bug1517728.patchSplinter Review

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!

Attachment #9044582 - Flags: feedback?(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?
Attachment #9044582 - Flags: feedback?(nchevobbe)
Attachment #9048625 - Attachment description: Export Console content to file → Bug 1517728 - Export Console content to file. r=nchevobbe.
Status: NEW → ASSIGNED

How is it going Jefry?

Priority: -- → P3

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

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?

Flags: needinfo?(nchevobbe)

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

Flags: needinfo?(nchevobbe)
Flags: needinfo?(jefry.reyes)

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?

Flags: needinfo?(jefry.reyes)

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!

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

Hello Jefry,

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

Flags: needinfo?(jefry.reyes)

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.

Flags: needinfo?(jefry.reyes)

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.

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).

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

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

Flags: needinfo?(nchevobbe)
Attachment #9064690 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
You need to log in before you can comment on or make changes to this bug.