651942 - Add a list of recently-opened files to the file menu of the Scratchpad

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Rob Campbell [:rc] (:robcee)]

The workspace should maintain a persistent list of 5 (to 10 or so) recently-opened files in the File menu. Accessing these items will reload the given file from its location on disk.

Comment 1

[Johan C]

Attachment: patch 0.1

* Added a 'Clear Items' menuitem for clearing the list.
* Added a pref for controlling how many recently opened files should be stored.

Comment 2

[Josh Matthews [:jdm]]

Rob, this patch has been waiting for feedback for some time now.

Comment 3

[Rob Campbell [:rc] (:robcee)]

yes. I've been traveling and have talked to Johan about it. Thanks for your help.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 602946 [details] [diff] [review]
patch 0.1

+pref("devtools.scratchpad.recentlyOpened.max-recent", 10);

that preference name is a little ungainly. How about changing that to:

devtools.scratchpad.recentfiles ?

+  setRecentFiles: function SP_setRecentFiles(aFile) {
+    let maxRecent = Services.prefs.getIntPref(PREF_RECENTLY_OPENED_MAX);
+    let aFiles = this.getRecentFiles();
+    let aFilesLength = aFiles.length;

we usually use the aParameter naming conventions for function/method parameters. I'd probably lose the "a" prefix on these variable names.

+    let branch = Services.prefs.getBranch("devtools.scratchpad.recent-files.list.");

recentfileslist.?

+    if (aFilesLength < maxRecent) {
+      branch.setComplexValue("file"+aFilesLength, Ci.nsILocalFile, aFile);
+    }

trailing space after that brace. Careful with those!

"file" + aFilesLength should have spacing around the + operator.

+    else {
+      // moves every file down, overwriting the oldest recent.
+      for (let i = 1; i < aFilesLength; i++)
+        branch.setComplexValue("file"+(i-1), Ci.nsILocalFile, aFiles[i]);
+
+      branch.setComplexValue("file"+(maxRecent-1), Ci.nsILocalFile, aFile);
+    }
+  },

I think I'd rather see you retrieve the full list of files first and then write them out in one shot rather than doing the reading and writing at the same time. A personal preference but I think it'll look cleaner.

You also get to reuse getRecentFiles()!

in +  getRecentFiles: function SP_getRecentFiles() {

+    while (!error) {
+      try {
+        let file = branch.getComplexValue("file"+i, Ci.nsILocalFile);
+        files.push(file);
+        i++;
+      }
+      catch (err) {
+        error = true;
+      }
+    }

move your try/catch outside of the while loop.

+        menuitem.setAttribute("oncommand", "Scratchpad.openRecentFile("+i+");");

spaces around operators, please.

I think the overall approach is reasonable. Just a few stylistic nits.

Writing a unittest for this is going to be a Blast! ;)

Comment 5

[Johan C]

Attachment: patch 0.2

No test yet, but I have reworked the patch a bit, could you take another look?

I found a better solution for 'SP_getRecentFiles', the try-catch method required a lot of additional code to fix the following bugs.

Fixed bugs:
* If the "recentfiles"-pref has been set to a number that is less than the number of files currently stored, the oldest files are removed and the list is reordered.
* If a "file-pref" (e.g. "...recentfileslist.3") has been reset and it's not the most recently opened file, the list is reordered.

Added:
* When opening a file from the 'Open Recent' menu, set it as the most recently opened file.
* Mark file currently opened with checked="true".

Fixed a few typos.
Fixed typo: SP_getState -> SP_setState.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 613654 [details] [diff] [review]
patch 0.2

+  /**
+   * Save a recently opened file as a complexValue.
+   *
+   * @param nsILocalFile file
+   *        The file we want to store as a recently opened file.
+   *
+   * @return bool conditional
+   *         Returns false if the file already exists in the list of
+   *         recently opened files.

nit: don't usually have extra linebreaks between @param and @return

in setRecentFile:

+  setRecentFile: function SP_setRecentFile(file) {

parameter names should have an "a" prefix (for "argument"). It's a silly convention we use.

+    // If the file is already in the list, set it as the most recent.
+    for (let i = 0; i < filesCount; i++) {
+      if (files[i].path === file.path) {
+        for (let j = i + 1; j < filesCount; j++) {
+          branch.setComplexValue(j - 1, Ci.nsILocalFile, files[j]);
+        }
+
+        branch.setComplexValue(filesCount - 1, Ci.nsILocalFile, file);
+        return false;
+      }
+    }

not loving this nested loop.

For one, it's doing more than just "setting this file as the most recent". It's also renumbering the rest of the branch.

what about,

let paths = files.map(function(file) { return file.path; });
let pathIndex = paths.indexOf(aFile.path);

if (pathIndex < 0) { // path not found
  if (files.length == maxRecent)
    files.pop(); // remove last file on the array
} else // path found, remove it
  files.splice(pathIndex, 1);

files.unshift(aFile); // add the file to the front of the array

files.forEach(function(file, index) {
  branch.setComplexValue(index, Ci.nsILocalFile, file);
});

I think that covers the whole thing. What do you think?

Comment 7

[Johan C]

Attachment: patch 0.3 tests wip 1

Updated per your feedback, thanks!

+ Add the file to our list of recent files on "Save" and "Save as".
+ First version with tests.

What do you think? \o/ tests \o/

Comment 8

[Johan C]

Attachment: patch 0.3 tests wip 1

Silly me, forgot to add the test file.

Se comment above for notes on patch.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 616648 [details] [diff] [review]
patch 0.3 tests wip 1

The test is a good start. It tests the new methods you've created to setRecentFile and queries the contents of the recent files menu. That's good stuff. Do we have any file open and save tests anywhere you could query the contents of the recent files menu as well?

good progress!

Comment 10

[Johan C]

Attachment: patch 0.4

+ Test for menuitems in menu.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 617672 [details] [diff] [review]
patch 0.4

+  /**
+   * Open recently opened file.
+   * 
+   * @param integer index
+   *        Clicked menuitem in the 'Open Recent' menu.
+   */
+  openRecentFile: function SP_openRecentFile(index) {
+    let files = this.getRecentFiles();
+    this.setFilename(files[index].path);
+    this.importFromFile(files[index], false);
+    this.setRecentFile(this.getRecentFiles()[index]);
+  },

+    this.setRecentFile(this.getRecentFiles()[index]);

is there a reason you don't just setRecentFile(files[index])?

You could also dereference the file in question on your first call to getRecentFiles() and get:

+  openRecentFile: function SP_openRecentFile(index) {
+    let file = this.getRecentFiles()[index];
+    this.setFilename(file.path);
+    this.importFromFile(file, false);
+    this.setRecentFile(file);
+  },

in setRecentFile():

could you add some comments around the if... else and before the forEach to explain what you're doing? (if not found, remove the last file, otherwise, remove it from the list, etc)

in getRecentFiles():

+    let diff = (files.length - maxRecent >= 0) ? files.length - maxRecent : 0;

add a comment for this please.

(number of files is greater than max recent files? how does that happen?)

+    for (let i = 0; i < files.length; i++) {

explain what this is for, please.

This seems to be constructing a list of files. Starting with getting the childList from the branch, and then sorting it.

Then iterating through all the files, getting each one, removing it from the branch, but if the index is greater than the diff value, reinserting it and adding it to the sorted list.

Very confusing. I think this method should be clearer and not do any restructuring of the preference branch at all since you'd expect it to retrieve values. It's generally a bad idea to modify storage when you're retrieving something.

+    } else {
+      recentFilesMenu.setAttribute("disabled", true);
+    }

Is this redundant? Menu's disabled by default, right? If there are no items in the recently opened list, it won't be enabled?

+  clearRecent: function SP_clearRecent()

This doesn't update the menu does it?

in scratchpad.xul:

+        <menupopup id="sp-menu-open_recentPopup">
+        </menupopup>

you can just use a /> to close the menupopup and ditch </menupopup>.

canceling review.

Comment 12

[Johan C]

Reworking the patch to populate the menu when Scratchpad is being opened, and to make use of an observer for preference changes.

Comment 13

[Johan C]

Attachment: patch 0.5

Reworked patch according to your feedback. It's now using a preference observer to repopulate the menu and for clearing items from the menu, as well as hiding/unhiding/disabling/enabling the menu. Furthermore, the menu is now populated when Scratchpad is opened.

Modified the tests to work with the reworked methods, and added new tests to further test the observer. I'm not sure about the tests however, setTimeouts are necessary to make sure the preference observer has fired before running the appropriate test, but I'm not sure if this is the best solution.

In test06 for example I have to call the populateRecentFilesMenu method even though I have a 100ms delay between clearing all files and running the tests in test06. Also, for some reason I have to call the removeRecentFilesMenuitem method to hide the 'Open Recent'-menu, even though it should already be hidden, as the menu is empty.

Any feedback is greatly appreciated.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 624632 [details] [diff] [review]
patch 0.5

I think this is good, Johan. I'm going to ask Heather to have a pass through this.

Comment 15

[Heather Arthur [:harth]]

Comment on attachment 624632 [details] [diff] [review]
patch 0.5

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

Thanks for the patch for this much-needed feature! It looks good. I just have some nits and a couple suggestions. As for the setTimeout()s in the test, they should be avoided. Maybe you could observe the pref change in the test.

::: browser/devtools/scratchpad/scratchpad-manager.jsm
@@ +205,5 @@
> +    if (aData == "recentFilesMax") {
> +      let enumerator = Services.wm.getEnumerator("devtools:scratchpad");
> +      while (enumerator.hasMoreElements()) {
> +        let win = enumerator.getNext();
> +        win.Scratchpad.handleRecentFileMaxChange();

I'm more for observing the preference change from each scratchpad window, e.g. having the PreferenceObserver live in scratchpad.js. Less micromanaging for the scratchpad manager.

@@ +209,5 @@
> +        win.Scratchpad.handleRecentFileMaxChange();
> +      }
> +    }
> +    else if (aData.match(/^recentfileslist.([0-9]+)$/)) {
> +      let prefName = aData.match(/^recentfileslist.([0-9]+)$/)[1];

put `aData.match(...)` in a var so you don't have to change it in two places, and to be more descriptive.

::: browser/devtools/scratchpad/scratchpad.js
@@ +789,5 @@
> +      let file = branch.getComplexValue(prefNames[i], Ci.nsILocalFile);
> +      files.push(file);
> +    }
> +
> +    return [files, prefNames];

It seems like a lot of this code could be avoided by just storing the list as an JSON-parseable array of file paths in a string pref. 

To update the list you could parse the preference, update the array and write it back to the preference.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_651942_recent_files.js
@@ +29,5 @@
> +let gFileContent02 = "hello.world.02('bug651942');";
> +let gFileContent03 = "hello.world.03('bug651942');";
> +let gFileContent04 = "hello.world.04('bug651942');";
> +
> +function p(objects) {

Use a more descriptive function name here than just "p". Is this an unused function? If so, take it out.

@@ +41,5 @@
> +function runTest01()
> +{
> +  gScratchpad = gScratchpadWindow.Scratchpad;
> +
> +  gFile01 = createTemporaryFile(gFile01, gFileName01, gFileContent01);

The name "createTemporaryFile" only describes part of what that function does, I think the more important part to describe is that the function loads the file into a scratchpad.

@@ +45,5 @@
> +  gFile01 = createTemporaryFile(gFile01, gFileName01, gFileContent01);
> +  gFile02 = createTemporaryFile(gFile02, gFileName02, gFileContent02);
> +  gFile03 = createTemporaryFile(gFile03, gFileName03, gFileContent03);
> +
> +  setTimeout(runTest02, 100);

Don't use `setTimeout()`. The test could fail on slower systems. Add a callback argument to `createTemporaryFile()` to be called when the file has been saved.

You'll have to chain the opening, so you could either just test opening two files to keep it readable (probably enough anyways), or use a kind of async utility function like the one here: https://github.com/caolan/async#series.

@@ +48,5 @@
> +
> +  setTimeout(runTest02, 100);
> +}
> +
> +function runTest02()

you could call this something more descriptive, like "testOverwriteRecent".

@@ +63,5 @@
> +    runTest03(recentFiles);
> +  }, 100);
> +}
> +
> +function runTest03(recentFiles)

More descriptive function name.

@@ +67,5 @@
> +function runTest03(recentFiles)
> +{
> +  let [recentFiles02, prefNames02] = gScratchpad.getRecentFiles();
> +
> +  is(recentFiles02[0].path, recentFiles[1].path, "File01: Success");

Something more telling than "Success". I can't tell what this is testing, maybe "File01 path is correct".

@@ +75,5 @@
> +  gScratchpad.openRecentFile(prefNames02[0]);
> +
> +  setTimeout(function() {
> +    runTest04(recentFiles02);
> +  }, 100);

Same here, no `setTimeout()`, use callbacks.

@@ +78,5 @@
> +    runTest04(recentFiles02);
> +  }, 100);
> +}
> +
> +function runTest04(recentFiles02)

More descriptive function name.

@@ +94,5 @@
> +  gScratchpad.clearRecentFiles(prefNames03[1]);
> +
> +  setTimeout(function() {
> +    runTest05([recentFiles03[0].path, recentFiles03[1]])
> +  }, 100);

no `setTimeout()`

@@ +97,5 @@
> +    runTest05([recentFiles03[0].path, recentFiles03[1]])
> +  }, 100);
> +}
> +
> +function runTest05(pathsForTheFilesWeCleared)

More descriptive function name.

"pathsForTheFilesWeCleared" is too long, maybe "clearedFiles".

@@ +118,5 @@
> +  isnot(matchFound, true,
> +        "The files were successfully removed from the 'Open Recent'-menu");
> +  gScratchpad.clearRecentFiles();
> +
> +  setTimeout(runTest06, 100);

No `setTimeout()`

@@ +121,5 @@
> +
> +  setTimeout(runTest06, 100);
> +}
> +
> +function runTest06()

More descriptive name.

@@ +136,5 @@
> +  ok(menu.hasAttribute("disabled"), "No files in the menu, it was disabled successfully.");
> +
> +  Services.prefs.setIntPref("devtools.scratchpad.recentFilesMax", 0);
> +
> +  setTimeout(runTest07, 100);

No `setTimeout()`.

@@ +167,5 @@
> +
> +  NetUtil.asyncCopy(fileContentStream, fout);
> +
> +  gScratchpad.setFilename(aFile.path);
> +  gScratchpad.importFromFile(aFile.QueryInterface(Ci.nsILocalFile),  true,

It looks like we're not waiting for the copying to finish before opening the file here. Pass a callback to asyncCopy and open the file in the callback.

@@ +193,5 @@
> +{
> +  let doc = gScratchpadWindow.document;
> +
> +  is(doc.getElementById("sp-menu-open_recentPopup").children.length,
> +     gScratchpad.getRecentFiles()[0].length + 2,

Might want to make vars for these things you're comparing and explain the "+ 2" part in a comment.

::: browser/locales/en-US/chrome/browser/devtools/scratchpad.dtd
@@ +30,5 @@
>  <!ENTITY openFileCmd.commandkey       "o">
>  
> +<!ENTITY open_recentMenu.label        "Open Recent">
> +<!ENTITY open_recentMenu.accesskey    "R">
> +<!ENTITY clear_recentMenu.label       "Clear Items">

Nit: Looks like the other entities are camel cased without "_", might want to stick to that to be consistent.

Comment 16

[Johan C]

Attachment: patch 0.6

Thanks Heather for the feedback and help.

Changes:
1. Moved the preference observer to scratchpad.js.
2. Removed the 'removeRecentFilesMenuitem'-method.
3. Updated and improved the test, it is now utilising a preference observer.
4. Reworked the patch to save file-paths in a JSON parsable string instead of a complexValue.
5. Simplified the preference observer a bit.
6. Fixed other nits and bits per your feedback.

Comment 17

[Johan C]

^ In addition I added my patch for bug 756501.

Comment 18

[Heather Arthur [:harth]]

Comment on attachment 629531 [details] [diff] [review]
patch 0.6

Looks great to me, sorry for the delay!

Comment 19

[Johan C]

Attachment: patch 0.6 rebased

(In reply to Heather Arthur [:harth] from comment #18)
> Comment on attachment 629531 [details] [diff] [review]
> patch 0.6
> 
> Looks great to me, sorry for the delay!

No problem, cheers!

Rebased the patch.

Comment 20

[Heather Arthur [:harth]]

Comment on attachment 634429 [details] [diff] [review]
patch 0.6 rebased

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

Thanks for rebasing, I think you forgot to add the test file though (: Can you upload a new patch with the test and also take out the code for bug 756501, I actually think we should do that functionality separately. Just a couple more small things while we're at it:

::: browser/devtools/scratchpad/scratchpad.js
@@ +662,5 @@
> +        let filePath = this.getRecentFiles()[aIndex];
> +        file.initWithPath(filePath);
> +        this.importFromFile(file, false);
> +        this.setFilename(file.path);
> +        this.setRecentFile(file);

It looks like you might end up duplicating many of these steps in both openRecentFile() and openFile(). Use your judgement, but a function with any shared code would be great.

@@ +738,5 @@
> +    filePaths.push(aFile.path);
> +
> +    let branch = Services.prefs.
> +                 getBranch("devtools.scratchpad.");
> +    branch.setCharPref("recentFilePaths", JSON.stringify(filePaths));

Maybe return true here if you've specified the return is a boolean up there.

@@ +751,5 @@
> +    let recentFilesMenu = document.getElementById("sp-open_recent-menu");
> +
> +    if (maxRecent < 1) {
> +      recentFilesMenu.setAttribute("hidden", true);
> +      return false;

just "return;" here if you won't be using the return value.

Comment 21

[Heather Arthur [:harth]]

(In reply to Heather Arthur [:harth] from comment #20)
> 
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +662,5 @@
> > +        let filePath = this.getRecentFiles()[aIndex];
> > +        file.initWithPath(filePath);
> > +        this.importFromFile(file, false);
> > +        this.setFilename(file.path);
> > +        this.setRecentFile(file);
> 
> It looks like you might end up duplicating many of these steps in both
> openRecentFile() and openFile(). Use your judgement, but a function with any
> shared code would be great.

Actually, if you do this shared function, you could probably add in the "savePrompt" code that fixes bug 756501 too, since it seems more of a necessity than functionality addition.

Comment 22

[Johan C]

Attachment: patch 0.6.1

(In reply to Heather Arthur [:harth] from comment #20)
> Comment on attachment 634429 [details] [diff] [review]
> patch 0.6 rebased
> 
> Review of attachment 634429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for rebasing, I think you forgot to add the test file though (:

Sorry, I'm not sure how that happened.

> @@ +738,5 @@
> > +    filePaths.push(aFile.path);
> > +
> > +    let branch = Services.prefs.
> > +                 getBranch("devtools.scratchpad.");
> > +    branch.setCharPref("recentFilePaths", JSON.stringify(filePaths));
> 
> Maybe return true here if you've specified the return is a boolean up there.

I removed the specification and changed all returns in this method to "return;".

> @@ +751,5 @@
> > +    let recentFilesMenu = document.getElementById("sp-open_recent-menu");
> > +
> > +    if (maxRecent < 1) {
> > +      recentFilesMenu.setAttribute("hidden", true);
> > +      return false;
> 
> just "return;" here if you won't be using the return value.

Done.

(In reply to Heather Arthur [:harth] from comment #21)
> (In reply to Heather Arthur [:harth] from comment #20)
> > 
> > ::: browser/devtools/scratchpad/scratchpad.js
> > @@ +662,5 @@
> > > +        let filePath = this.getRecentFiles()[aIndex];
> > > +        file.initWithPath(filePath);
> > > +        this.importFromFile(file, false);
> > > +        this.setFilename(file.path);
> > > +        this.setRecentFile(file);
> > 
> > It looks like you might end up duplicating many of these steps in both
> > openRecentFile() and openFile(). Use your judgement, but a function with any
> > shared code would be great.
> 
> Actually, if you do this shared function, you could probably add in the
> "savePrompt" code that fixes bug 756501 too, since it seems more of a
> necessity than functionality addition.

I went with this. If you could take a closer look at the "openFile"-method, that would be great.

Cheers :)

Comment 23

[Heather Arthur [:harth]]

Comment on attachment 635084 [details] [diff] [review]
patch 0.6.1

Looks good. This is awesome, thanks for the patch and updates!

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 635084 [details] [diff] [review]
patch 0.6.1

>diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js

>+  setRecentFile: function SP_setRecentFile(aFile)

>+    let paths = filePaths.map(function(file) { return file; });

Drive-by super-nit that I just happened to notice: this map call seems to be a no-op?

Also a thought that occurred to me when thinking about the feature in general: should we avoid saving recently opened files while in private browsing mode? Kind of an edge case, but also relatively easy to fix. Can do that in a followup though.

Comment 25

[Rob Campbell [:rc] (:robcee)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #24)
> Comment on attachment 635084 [details] [diff] [review]
> patch 0.6.1
> 
> >diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js
> 
> >+  setRecentFile: function SP_setRecentFile(aFile)
> 
> >+    let paths = filePaths.map(function(file) { return file; });
> 
> Drive-by super-nit that I just happened to notice: this map call seems to be
> a no-op?

good catch! paths == filePaths.

> Also a thought that occurred to me when thinking about the feature in
> general: should we avoid saving recently opened files while in private
> browsing mode? Kind of an edge case, but also relatively easy to fix. Can do
> that in a followup though.

does this really apply? PrivateBrowsing doesn't necessarily imply PrivateScratchpads.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

But what if I'm scratchpadding the page where I'm buying a wedding ring?!1

We just generally seem to avoid persisting any state while in PB mode, particularly if it can be site-related, so it seemed like something we may want to do here. But certainly no need to block on it now; can discuss it with private browsing pros in a separate bug.

Comment 27

[Johan C]

Attachment: patch 0.6.2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #24)
> Comment on attachment 635084 [details] [diff] [review]
> patch 0.6.1
> 
> >diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js
> 
> >+  setRecentFile: function SP_setRecentFile(aFile)
> 
> >+    let paths = filePaths.map(function(file) { return file; });
> 
> Drive-by super-nit that I just happened to notice: this map call seems to be
> a no-op?

Thanks for catching that, fixed.

Rebased and addressed gavin's keen eye, so carrying forward the r+ from the previous attachment.

Thank you Rob, Heather and Gavin for your feedback and patience on this one. :)

This patch includes the fix for bug 756501.

Comment 28

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/1464fc01cd17

Comment 29

[Paul Rouget [:paul]]

Backed out: https://hg.mozilla.org/integration/fx-team/rev/a28eac0f3690
(leaks)

See: https://tbpl.mozilla.org/php/getParsedLog.php?id=13077771&tree=Fx-Team

Comment 30

[Johan C]

Attachment: patch 0.7

* Plugged leaks.
* Fixed some thing I missed.

Thank you again harth and also dcamp for all the help. :)

Comment 31

[Heather Arthur [:harth]]

Comment on attachment 638490 [details] [diff] [review]
patch 0.7

looks good, thanks for the fix.

Comment 32

[Heather Arthur [:harth]]

http://hg.mozilla.org/integration/fx-team/rev/40e232a4acf3

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/40e232a4acf3