1214352 - Allow scratchpad to be used with tab toolboxes

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Currently, Scratchpad is disabled with tab toolboxes.

I don't believe there's any reason for this, it's just a UX choice, since there's also a separate window.

Let's try enabling it.

Comment 1

[Dan Callahan [:callahad]]

Setting isTargetSupported to true in https://dxr.mozilla.org/mozilla-central/source/devtools/client/definitions.js#394 seems to work just great.

The use case I want is something like DrRacket's UI, with a text editor on top and a REPL on bottom: https://upload.wikimedia.org/wikipedia/commons/b/b5/Drracket.png

It's a poor imitation of something like Emacs + SLIME, but it's great for interactively teaching JavaScript while live coding. Planning on using it tonight to discuss ES6 promises + generators at a local meetup.

Comment 2

[Alexandre Poirot [:ochameau]]

Is it really a UX choice?

Isn't that just related to the fact that Scratchpad only supports remote (client+remote and everything) and not local tabs?
If that's the case, Scratchpad never worked in tabs, but has always been displayed in the options since bug 895180 landed, i.e. 2013/10.

That brings me to the point:
  Do we really want to keep it in toolbox at all?
  The only place where it can be used since 2013 is browser toolbox and webide toolboxes?

Do we have any metrics about scratchpad usage in toolboxes?
Do we have distinct metrics for scratched used in its own window?

Otherwise, enabling it is really easy.
I would be even tempted to enable it *and* get rid of the scratchpad window in favor of scratchpad in tabs or browser toolbox!

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Enable scratchpad in tab toolboxes

Great or dead.


The pref is there to disable scratchpad by default.
So that you have to go into options first. (existing behavior, expect that you can actually enabled it now!)
But it is enabled by default in browser toolbox. (existing behavior)

Comment 4

[Alexandre Poirot [:ochameau]]

Jeff, Any user feedback and/or product vision about Scratchpad future?

Based on Great Or Dead™ we could just get rid of it in toolbox and keep it just on its own top level window. (Given that it has always been disabled in toolbox and noone asked for it?!)
Or we could also do something completely different, still compatible with Great Or Dead™, the exact opposite. Kill with fire the Scratchpad menuitem in the menus (and its custom window mode) and instead enable and promote scratchpad in toolboxes! (Browser developers would go to the browser toolbox).

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Alexandre Poirot [:ochameau] from comment #2)
...
> I would be even tempted to enable it *and* get rid of the scratchpad window
> in favor of scratchpad in tabs or browser toolbox!

I've long been a fan of using scratchpad in webide with split console.

I'm in favour of this with three caveats:

1. we shouldn't kill the separate window initially, I would actually be interested in keeping it around as-is for a little while. A strong argument about the technical complexity of maintaining both modes might convince me otherwise, but I don't see that here so far. If we promote this new way of using scratchpad via devrel and it catches on, we should see usage of the undocked window plummet in favour of it? Yes, this implies adding telemetry to track usage of each mode.

2. as a user I will likely want to switch back and forth between scratchpad and a split console a lot, so we should have ( and document / promote ) a keybinding for that.

3. Scratchpad has pretty odd behaviour for auto indentation and a few other things. We should review how the conifguration of it and bring it as closely in line with the default behaviour in Atom and Sublime Text as possible. I logged bug 1223820 to capture this.

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #2)
> ...
> > I would be even tempted to enable it *and* get rid of the scratchpad window
> > in favor of scratchpad in tabs or browser toolbox!
> 
> I've long been a fan of using scratchpad in webide with split console.
> 
> I'm in favour of this with three caveats:
> 
> 1. we shouldn't kill the separate window initially, I would actually be
> interested in keeping it around as-is for a little while. A strong argument
> about the technical complexity of maintaining both modes might convince me
> otherwise, but I don't see that here so far. If we promote this new way of
> using scratchpad via devrel and it catches on, we should see usage of the
> undocked window plummet in favour of it? Yes, this implies adding telemetry
> to track usage of each mode.

This isn't only technical. We don't maintain our tools very well. We keep piling stuff instead of integrating nicely what we already have. The scratchpad window for web developers sucks. You may be on Mac and be fine with having multiple windows but it is much much better in the toolbox, next to your document and *all* other tools. Imagine having a resource tree in toolboxes, with a "new JS resource" button, which opens a blank scratchpad. Then scratchpad doesn't need a name to make sense.
I'm totally fine with a intermediate plan as soon as we are able to make strong decisions about our products.

> 2. as a user I will likely want to switch back and forth between scratchpad
> and a split console a lot, so we should have ( and document / promote ) a
> keybinding for that.

We have ESC that works for all but scratchpad. I imagine it is incompatible with the VIM mode.
But it would be weird to have another keybinding just for scratchpad??
I could look into ensuring ESC also works in scratchpad?
I'm fine adding a key binding, but lazy to figure out which one to add...

> 
> 3. Scratchpad has pretty odd behaviour for auto indentation and a few other
> things. We should review how the conifguration of it and bring it as closely
> in line with the default behaviour in Atom and Sublime Text as possible. I
> logged bug 1223820 to capture this.

Would be great to ping editor experts about that.
Is WebIDE/style editor any better? Are we using same editor components between all of them?

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #5)
> > (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > ...
> > > I would be even tempted to enable it *and* get rid of the scratchpad window
> > > in favor of scratchpad in tabs or browser toolbox!
> > 
> > I've long been a fan of using scratchpad in webide with split console.
> > 
> > I'm in favour of this with three caveats:
> > 
> > 1. we shouldn't kill the separate window initially, I would actually be
> > interested in keeping it around as-is for a little while. A strong argument
> > about the technical complexity of maintaining both modes might convince me
> > otherwise, but I don't see that here so far. If we promote this new way of
> > using scratchpad via devrel and it catches on, we should see usage of the
> > undocked window plummet in favour of it? Yes, this implies adding telemetry
> > to track usage of each mode.
> 
> This isn't only technical. We don't maintain our tools very well. We keep
> piling stuff instead of integrating nicely what we already have. The
> scratchpad window for web developers sucks. You may be on Mac and be fine
> with having multiple windows but it is much much better in the toolbox, next
> to your document and *all* other tools. Imagine having a resource tree in
> toolboxes, with a "new JS resource" button, which opens a blank scratchpad.
> Then scratchpad doesn't need a name to make sense.
> I'm totally fine with a intermediate plan as soon as we are able to make
> strong decisions about our products.

I would instead prefer to make a data-driven decision about our products. Already the data we have is that almost no-one uses scratchpad. Our hypothesis with this change is that, by allowing it in the toolbox, we will improve the usage. Instead of acting on the hypothesis and hoping that we are right, we should A/B both modes, promote the new feature, and see what the data tells us.

There is an angle here where many people will be exposed to scratchpad for the first time via outreach then surprise us by preferring the windowed version instead of the docked version.

> > 2. as a user I will likely want to switch back and forth between scratchpad
> > and a split console a lot, so we should have ( and document / promote ) a
> > keybinding for that.
> 
> We have ESC that works for all but scratchpad. I imagine it is incompatible
> with the VIM mode.
> But it would be weird to have another keybinding just for scratchpad??
> I could look into ensuring ESC also works in scratchpad?
> I'm fine adding a key binding, but lazy to figure out which one to add...

Having thought about this more, what I'm proposing is a keybinding that will switch focus between the split console and whatever tool is open. It shouldn't be specific to scratchpad but it would certainly be very useful when using scratchpad.

> > 
> > 3. Scratchpad has pretty odd behaviour for auto indentation and a few other
> > things. We should review how the conifguration of it and bring it as closely
> > in line with the default behaviour in Atom and Sublime Text as possible. I
> > logged bug 1223820 to capture this.
> 
> Would be great to ping editor experts about that.
> Is WebIDE/style editor any better? Are we using same editor components
> between all of them?

I just reviewed this, I think scratchpad must be now using the same defaults as webIDE now, it's much better than it used to be. I still have nitpicks about a few things but I don't think it's a blocker.

Comment 8

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a68c6daf255

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Enable scratchpad in tab toolboxes - v2

Updated tests to ack the presence of scratchpad.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Add telemetry for Scratchpad as top level window - v1

I don't know if that's the best way to add this telemetry...
I was wondering if we had to split the existing "scratchpad" into two different keys like scratchpad-toolbox/scratchpad-window.
Or if, with this, we could just substract scratchpad-window to scratchpad in order to have the toolbox usage.

Comment 11

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94451dfd2923

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Enable scratchpad in tab toolboxes - v3

Fixed worker test. (WorkerTarget.hasActor was wrong)

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Add telemetry for Scratchpad as top level window - v2

Forgot to modify Histograms.json...

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8703827 [details] [diff] [review]
Enable scratchpad in tab toolboxes - v3

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

Seems good overall, but a few questions left to resolve.

::: devtools/client/framework/target.js
@@ +752,5 @@
>  
>    destroy: function() {},
>  
>    hasActor: function (name) {
> +    if (name == "console") {

Add a comment explaining why this is a special case.

::: devtools/client/preferences/devtools.js
@@ +208,5 @@
>  // Enable the Web Audio Editor
>  pref("devtools.webaudioeditor.enabled", false);
>  
> +// Enable Scratchpad
> +pref("devtools.scratchpad.enabled", false);

Will this cause it to become disabled for someone who previously had it enabled in a remote context?

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8703828 [details] [diff] [review]
Add telemetry for Scratchpad as top level window - v2

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

Code-wise, it seems fine.  But the privacy team won't accept `"expires_in_version": "never"` for new probes.  Also, you need a review from them.

Check the red box at https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe.

They will likely also suggest adding alert_emails.  Check [1] as an example.

[1]: https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/toolkit/components/telemetry/Histograms.json#5859

::: devtools/client/scratchpad/scratchpad-manager.jsm
@@ +17,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
> +const {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +
> +const Telemetry = require("devtools/client/shared/telemetry");
> +const telemetry = new Telemetry();

Maybe make lowercase `telemetry` a property of the `ScratchpadManager` object?

@@ +130,5 @@
>      let win = Services.ww.openWindow(null, SCRATCHPAD_WINDOW_URL, "_blank",
>                                       SCRATCHPAD_WINDOW_FEATURES, params);
>  
> +    telemetry.toolOpened("scratchpad-window");
> +    let onClose = function () {

No space, against lint rules.  Please use ESLint!

Comment 16

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b9b6c6ccbc2

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Enable scratchpad in tab toolboxes - v4

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> ::: devtools/client/preferences/devtools.js
> > +// Enable Scratchpad
> > +pref("devtools.scratchpad.enabled", false);
> 
> Will this cause it to become disabled for someone who previously had it
> enabled in a remote context?

Yes. But if we don't introduce this pref, scratchpad will be enable everywhere, which seems to be also wrong.
I tend to think Scratchpad has been enabled by default on remote by mistake.
Scratchpad will now be automatically turned on in the browser toolbox and for addon.
And will be optionaly displayed on local tabs and in webide.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Add telemetry for Scratchpad as top level window - v3

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> @@ +130,5 @@
> >      let win = Services.ww.openWindow(null, SCRATCHPAD_WINDOW_URL, "_blank",
> >                                       SCRATCHPAD_WINDOW_FEATURES, params);
> >  
> > +    telemetry.toolOpened("scratchpad-window");
> > +    let onClose = function () {
> 
> No space, against lint rules.  Please use ESLint!

I tried but hit bug 1237251.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8704616 [details] [diff] [review]
Enable scratchpad in tab toolboxes - v4

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

::: devtools/client/framework/target.js
@@ +752,5 @@
>  
>    destroy: function() {},
>  
>    hasActor: function (name) {
> +    // console is the only one actor implemented

Nit: Seems like this can be on one line and still fit under 80 chars

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8704617 [details] [diff] [review]
Add telemetry for Scratchpad as top level window - v3

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

Any chance I could get the telemetry review?

Comment 21

[Vladan Djeric (:vladan)]

Comment on attachment 8704617 [details] [diff] [review]
Add telemetry for Scratchpad as top level window - v3

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

::: toolkit/components/telemetry/Histograms.json
@@ +6821,5 @@
>      "description": "How many times has the devtool's Scratchpad been opened via the toolbox button?"
>    },
> +  "DEVTOOLS_SCRATCHPAD_WINDOW_OPENED_BOOLEAN": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "50",

add a bug_number field

@@ +6823,5 @@
> +  "DEVTOOLS_SCRATCHPAD_WINDOW_OPENED_BOOLEAN": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "50",
> +    "kind": "boolean",
> +    "description": "How many times has the devtool's Scratchpad standalone window been opened?"

all these histograms never record a False value, they should all be count histograms :/ Can you declare this new one as a count histogram?

@@ +7003,5 @@
>      "description": "How many users have opened the devtool's Scratchpad been opened via the toolbox button?"
>    },
> +  "DEVTOOLS_SCRATCHPAD_WINDOW_OPENED_PER_USER_FLAG": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "50",

add a bug_number field

@@ +7230,5 @@
>      "n_buckets": 100,
>      "description": "How long has Scratchpad been active (seconds)"
>    },
> +  "DEVTOOLS_SCRATCHPAD_WINDOW_TIME_ACTIVE_SECONDS": {
> +    "alert_emails": ["[e-mail]"],

add a bug_number field

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: telemetry interdiff

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Add telemetry for Scratchpad as top level window - v4

Addressed telemetry review.
Ryan, I had to tweak telemetry.js to support type:"count".
Do you think we should convert all existing histrogram to 'count'?
I have no idea about the outcome of that...
(I provided an interdiff)

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8709071 [details] [diff] [review]
Add telemetry for Scratchpad as top level window - v4

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

I think we need to keep using boolean for now, as our existing tool dashboards use the boolean type.  Converting the old tools to count would mean we'd lose the historical data, since the probe would be different.

We had this same discussion with Vladan for about:debugging in bug 1204601 comment 13.  I hope it is still okay to continue with boolean for now.

Converting to count should be a larger, separate project for our dashboards.

You should also add "p=vladan" to your commit message.

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: Add telemetry for Scratchpad as top level window - v5

Vladan, See comment 24. Are you ok with keeping it boolean?
I addressed all other comments.

Comment 26

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4903bad4a01c

Comment 27

[Alexandre Poirot [:ochameau]]

Comment on attachment 8710466 [details] [diff] [review]
Add telemetry for Scratchpad as top level window - v5

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

Benjamin, Could you validate this new Telemetry probe?
Note that we already had a discussion with Vladan from comment 21.
I addressed everything but the probe type as it would mess up with existing dashboard.
Using the right type would require fixing all existing probes and tweaking the dashboard to understand past data with old type and new data with new type.

Also, note that this page is outdated (the red box to suggest request for review):
  https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
Vladan just left, and :ally doesn't seem to be active (last bz activity in November).

Comment 28

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/d3cb55ade8dcf1df14cf365f3773c2592f76ac36
Bug 1214352 - Enable scratchpad in tab toolboxes. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/66ae50b60b1c327cf72761dc225bc2c5d8286315
Bug 1214352 - Add telemetry for Scratchpad as top level window. r=jryans,p=bsmedberg

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d3cb55ade8dc
https://hg.mozilla.org/mozilla-central/rev/66ae50b60b1c

Comment 30

[Will Bamberg [:wbamberg]]

docs -> https://developer.mozilla.org/en-US/docs/Tools/Scratchpad#Opening_Scratchpad_in_the_Toolbox

Let me know if this covers it.

(Also, do you know if code completion still works in Scratchpad? I tried out the example (https://developer.mozilla.org/en-US/docs/Tools/Scratchpad#Code_completion_and_inline_documentation), and it did not work for me.)

Comment 31

[Alexandre Poirot [:ochameau]]

(In reply to Will Bamberg [:wbamberg] from comment #30)
> docs ->
> https://developer.mozilla.org/en-US/docs/Tools/
> Scratchpad#Opening_Scratchpad_in_the_Toolbox
> 
> Let me know if this covers it.

Yes. Thanks. Nice addition about the split console!

> (Also, do you know if code completion still works in Scratchpad? I tried out
> the example
> (https://developer.mozilla.org/en-US/docs/Tools/
> Scratchpad#Code_completion_and_inline_documentation), and it did not work
> for me.)

Hum. It works here on nightly/windows.

Comment 32

[Alexandre Poirot [:ochameau]]

Completion also works on my custom build on linux.