1045660 - Auto-select and connect to last used runtime if available

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

Developers usually always work with the same runtime and debug the same app.

We can probably help the user by auto-selecting the latest runtime and try to connect to it, and in the case of Runtime Apps, auto select the latest app (package/hosted apps are already auto-selected).

Comment 1

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

We have bug 1055666 to focus on the "re-select last project if it's a runtime app" portion.

Let's re-focus this to just the "auto-select last used runtime" step.

Comment 2

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

Attachment: Initial Attempt

Attaching Alex's patch from bug 1055279 as initial work.

Comment 3

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

Comment on attachment 8487327 [details] [diff] [review]
Initial Attempt

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

I do think these bits would make more sense it webide.js instead of app-manager.js.

I think of app-manager as the model object, with the current state of the apps and runtimes, and emits events when these things change.

webide.js is more the view & controller object.  It asks app-manager for the current state, tells it to perform actions and make changes, and listens for events from app-manager to hear when the state changes.

So, overall it seems good, but let's move to webide.js.

::: browser/devtools/webide/modules/app-manager.js
@@ +104,5 @@
>      this.emit("app-manager-update", what, details);
> +
> +    // Automatically reconnect to the previously runtime,
> +    // if available and has an ID
> +    if (what == "runtimelist" && this.selectedRuntime == null &&

Moving this to webide.js as a new method called after "runtimelist" seems good.

With the type name as well (as I suggest below), you should know where to look too.

@@ +139,5 @@
>    onConnectionChanged: function() {
>      if (this.connection.status == Connection.Status.DISCONNECTED) {
> +      if (typeof(this.selectedRuntime.getID) === "function") {
> +        this.lastConnectedRuntimeID = this.selectedRuntime.getID();
> +        Services.prefs.setCharPref("devtools.webide.lastConnectedRuntimeID",

Rather than storing this at disconnection time, it seems clearer to me to store it right after a successful connection.

In webide.js, I'd suggest adding a new method that's called on the "runtime" event, and you'd store it then.

Also, I think we should store the type of runtime along with the ID.  In bug 916804, I added string type names to the runtimes.  That would useful here if that patch ever lands...

Comment 4

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

Alex, I've added some comments above.  If you don't have time for this anymore, let me know.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Add type attribute to webide runtimes

Part of patch already r+ by paul in bug 916804.
Hopefully, this isn't the part of the patch that makes TBPL intermittently orange!!

https://tbpl.mozilla.org/?tree=Try&rev=c548de9085ff

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

This patch depend on you work adding `type` on runtimes,
so I just pulled that and submitted it to eventually land it
if this patch lands before bug 916804.

Otherwise I tried to address all the feedback comments
and also provided a test.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Forgot to rename lastConnectedRuntimeID to lastConnectedRuntime in webide-prefs.js

https://tbpl.mozilla.org/?tree=Try&rev=c52bf2aae49b

Comment 8

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

Comment on attachment 8491013 [details] [diff] [review]
patch v3

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

A few renames and such, but overall it looks good.  Thanks!

::: browser/devtools/webide/content/webide.js
@@ +346,5 @@
>        }
>      }
>    },
>  
> +  autoSelectedRuntime: function () {

Either "autoSelectRuntime" or "autoConnectToRuntime"

@@ +347,5 @@
>      }
>    },
>  
> +  autoSelectedRuntime: function () {
> +    // Automatically reconnect to the previously runtime,

previously selected

@@ +362,5 @@
> +      type = "custom";
> +    }
> +
> +    // We support most runtimes except simulator,
> +    // that needs to be manually launched

Nit: Wrap to 80 chars

@@ +368,5 @@
> +      for (let runtime of AppManager.runtimeList[type]) {
> +        // Some runtimes do not expose getID function and don't support
> +        // autoconnect (like remote connection)
> +        if (typeof(runtime.getID) == "function" && runtime.getID() == id) {
> +          AppManager.connectToRuntime(runtime);

Use |this.connectToRuntime| (the webide.js UI function), so that the busy indicator is displayed.

@@ +390,5 @@
>        labelNode.setAttribute("value", name);
>      }
>    },
>  
> +  saveLastSelectedRuntime: function () {

The pref is "connected" and this function is "selected", pick your favorite and use it for both.

::: browser/devtools/webide/modules/app-manager.js
@@ +605,5 @@
>        this.runtimeList.usb.push(r);
>        r.updateNameFromADB().then(
> +        () => {
> +          this.update("runtimelist");
> +          // Also update the runtime button label,

Nit: Feel free to wrap to 80 chars like usual

::: browser/devtools/webide/test/test_autoconnect_runtime.html
@@ +75,5 @@
> +          is(Object.keys(DebuggerServer._connections).length, 1, "Automatically reconnected");
> +
> +          yield win.Cmds.disconnectRuntime();
> +
> +          yield closeWebIDE(win);

Move close and server destroy to a cleanup function so they still happen if the test blows up.

::: browser/devtools/webide/webide-prefs.js
@@ +15,5 @@
>  pref("devtools.webide.adbAddonURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/adb-helper/#OS#/adbhelper-#OS#-latest.xpi");
>  pref("devtools.webide.adbAddonID", "[e-mail]");
>  pref("devtools.webide.monitorWebSocketURL", "ws://localhost:9000");
> +pref("devtools.webide.lastConnectedRuntime", "");
> +pref("devtools.webide.lastSelectedProject", "");

This project pref belongs in the other bug I think...

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

https://tbpl.mozilla.org/?tree=Try&rev=30e7599a73cc

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Unfortunately, the usage of registerCleanupfunction fails...
I have no idea why but very few tests are doing this
and the only usage in webide uses synchronous calls.
Here we have some async function to call.
But the cleanup function doesn't seem to be called at all.

Here is a new patch without cleanup function...
https://tbpl.mozilla.org/?tree=Try&rev=d7fe90600344

Comment 11

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

Comment on attachment 8491460 [details] [diff] [review]
patch v4

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

::: browser/devtools/webide/test/test_autoconnect_runtime.html
@@ +25,5 @@
> +          DebuggerServer.addBrowserActors();
> +
> +          let win = yield openWebIDE();
> +
> +          SimpleTest.registerCleanupFunction(function* () {

Looks like |registerCleanupFunction| doesn't understand generators, maybe that was the issue?

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> > +          SimpleTest.registerCleanupFunction(function* () {
> 
> Looks like |registerCleanupFunction| doesn't understand generators, maybe
> that was the issue?

It was also failing with a regular function. It doesn't seem to be called at all :/

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/c10eea79e559
https://hg.mozilla.org/integration/fx-team/rev/9119c622fdf3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c10eea79e559
https://hg.mozilla.org/mozilla-central/rev/9119c622fdf3