Closed Bug 777377 Opened 9 years ago Closed 4 years ago

Recently Opened File should remember the Environment linked with the file (Browser or Content)

Categories

(DevTools Graveyard :: Scratchpad, defect, P3)

16 Branch
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: Optimizer, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Scratchpad now maintains a list of recently opened files. It would be awesome if it also remembers the environment associated with the files in the menu.
we could implement mode lines!

Filter on BLACKEAGLE?!
Priority: -- → P3
mode lines ?
(In reply to Girish Sharma [:Optimizer] from comment #2)
> mode lines ?

modelines are comments in your code that gives instruction to your editor.
Like pragmas for compilers.
(In reply to Paul Rouget [:paul] from comment #3)
> (In reply to Girish Sharma [:Optimizer] from comment #2)
> > mode lines ?
> 
> modelines are comments in your code that gives instruction to your editor.
> Like pragmas for compilers.

Yes, I talked to Mike after this comment, and adding mode lines support is very less discoverable, and user would equire to add extra lines, or the other way, user would be surprised to see extra lines added if we add them automatically.

So this is an alternative to be silent as well as discoverable: in the prefs that store the file locations, append each file name with environment type like 
"c://scratchpad.js|chrome"
Attached patch Patch (v1) (obsolete) — Splinter Review
This looked interesting so I came up with this patch ...

Files loaded from the file picker still default to environment of "content".

Files in the "recent files list" are tagged with |chrome if appropriate. When "recent files" are loaded, the proper environment (|chrome or not) is set.

The Notification box behaves properly, openning / closing as needed, even if the user closes it manually / etc.

Files when saved, update the "recent files list" to note changes in environment (|chrome or not), avoiding duplicate entries in the list.

Changes to the "recent files list" are reflected in multiple open scratchpad windows still.

Could you take a look? I'm still playing with it / looking for bugs, but would feel better with an independent UI review.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #653311 - Flags: review?(rcampbell)
Attached patch Patch (v2)Splinter Review
Small code tweak and a test push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=604c4a55949d
Attachment #653311 - Attachment is obsolete: true
Attachment #653311 - Flags: review?(rcampbell)
Attachment #653339 - Flags: review?(rcampbell)
Comment on attachment 653339 [details] [diff] [review]
Patch (v2)

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

This would also need a test. I think I'd prefer to see a different implementation altogether. Canceling review.

::: browser/devtools/scratchpad/scratchpad.js
@@ +704,5 @@
>                     createInstance(Components.interfaces.nsILocalFile);
>              let filePath = this.getRecentFiles()[aIndex];
> +            if (filePath.lastIndexOf("|") > -1) {
> +              fileContext = SCRATCHPAD_CONTEXT_BROWSER_SUFFIX;
> +              filePath = filePath.substr(0, filePath.lastIndexOf("|"));

hm. ok. This feels a little hacky. Do you need a full string like "chrome" on the end of that? if we're just encoding a bit on these paths you could use something like |c to denote chrome and save 5 characters * n paths.

@@ +767,5 @@
> +    let pathIndex = filePaths.indexOf(filePath);
> +    if (pathIndex == -1) {
> +      pathIndex =
> +        filePaths.indexOf(filePath + "|" + SCRATCHPAD_CONTEXT_BROWSER_SUFFIX);
> +    }

and the hack comes back to bite us here.

I think I'd prefer to see a different implementation. Either encode the context in the file itself somehow or use a separate collection to denote which files are chrome context or browser context.

@@ +772,4 @@
>  
>      // We are already storing this file in the list of recent files.
>      if (pathIndex > -1) {
> +      // Remove it from the list, we add it as the most recent farther down.

why are you changing this here?

@@ +776,4 @@
>        filePaths.splice(pathIndex, 1);
>      }
> +    // If we are not already storing the file and the 'recent files'-list
> +    // Is full, remove the oldest file from the list.

don't need to make this change either.

@@ +981,5 @@
>      let browser = document.getElementById("sp-menu-browser");
>      document.getElementById("sp-menu-content").removeAttribute("checked");
>      browser.setAttribute("checked", true);
>      this.executionContext = SCRATCHPAD_CONTEXT_BROWSER;
> +    this.notificationBox.removeAllNotifications(false);

why?
Attachment #653339 - Flags: review?(rcampbell)
Assignee: markcapella → nobody
Dropping back to observer status until the approach is finalized :) (I thought after comment #4 the group had decided.)

Replying to the rest:

>>>>> hm. ok. This feels a little hacky. Do you need a full string like "chrome" on the end of that? if we're just encoding a bit on these paths you could use something like |c to denote chrome and save 5 characters * n paths.

Ah well, just a UI issue ... no strong feelings here. (How fast would you expect n*5 to become prohibitive, storage / performance wise?)

@@ +767,5 @@
> +    let pathIndex = filePaths.indexOf(filePath);
> +    if (pathIndex == -1) {
> +      pathIndex =
> +        filePaths.indexOf(filePath + "|" + SCRATCHPAD_CONTEXT_BROWSER_SUFFIX);
> +    }
>>>>> and the hack comes back to bite us here.
>>>>> I think I'd prefer to see a different implementation. Either encode the context in the file itself somehow or use >>>>> a separate collection to denote which files are chrome context or browser context.

Yah, new to the group and maybe I got ahead of the discussion by jumping in with this version.

@@ +772,4 @@
>  
>      // We are already storing this file in the list of recent files.
>      if (pathIndex > -1) {
> +      // Remove it from the list, we add it as the most recent farther down.
>>>>> why are you changing this here?

Comments are good? ... If you're asking about the code change proper in that block, it keeps the "recent" list un-cluttered while users switch / save the same file but under different contexts. We'll keep the most recent file / context combination in the list versus keeping both.

@@ +776,4 @@
>        filePaths.splice(pathIndex, 1);
>      }
> +    // If we are not already storing the file and the 'recent files'-list
> +    // Is full, remove the oldest file from the list.
>>>>> don't need to make this change either.

Nah, don't need to. Thought it tightened things up at the time in regard to the above.

@@ +981,5 @@
>      let browser = document.getElementById("sp-menu-browser");
>      document.getElementById("sp-menu-content").removeAttribute("checked");
>      browser.setAttribute("checked", true);
>      this.executionContext = SCRATCHPAD_CONTEXT_BROWSER;
> +    this.notificationBox.removeAllNotifications(false);
>>>>> why?

There was a timing issue I noticed, either a) while opening / switching between files in different contexts, b) while using / saving the same file in different multiple windows, or c) a combination of both. (Further complicated where the user gets the notification and clicks the X button to close, opens another file, etc.)

I'd have to re-test to recall for sure.

Thanks for the attention! I'll keep an eye on this while finishing up some other small items -- mark
Duplicate of this bug: 789629
Status: ASSIGNED → NEW
How about having a separate preference to store an array of the urls for which the context was chrome (or for browser). That way, the preference could store even more than the recent history list number for a better user experience.

This will solve the issue of solving this bug in a hacky way and without touching any existing pref, but will introduce a new preference with a comparatively big size.
Blocks: 651942
Comment on attachment 653339 [details] [diff] [review]
Patch (v2)

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

Drive-by feedback:

::: browser/devtools/scratchpad/scratchpad.js
@@ -759,5 @@
> -        return;
> -      }
> -
> -      // It is not the most recent file. Remove it from the list, we add it as
> -      // the most recent farther down.

Is this bug no longer a problem?

@@ +789,5 @@
>  
>      let branch = Services.prefs.
>                   getBranch("devtools.scratchpad.");
>      branch.setCharPref("recentFilePaths", JSON.stringify(filePaths));
> +    this.populateRecentFilesMenu();

Why are you rebuilding the menu here? That's taken care of in the preference-listener.
Comment on attachment 653339 [details] [diff] [review]
Patch (v2)

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

::: browser/devtools/scratchpad/scratchpad.js
@@ -754,5 @@
> -      if (pathIndex === (filesCount - 1)) {
> -        // Updating the menu to clear the disabled state from the wrong menuitem
> -        // in rare cases when two or more Scratchpad windows are open and the
> -        // same file has been opened in two or more windows.
> -        this.populateRecentFilesMenu();

* this bug
Not touched in the past 5 years. No user complaints. Closing.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.