Closed Bug 1045660 Opened 7 years ago Closed 7 years ago

Auto-select and connect to last used runtime if available

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: paul, Assigned: ochameau)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [status:landedon])

Attachments

(2 files, 4 obsolete files)

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).
Component: Developer Tools: User Stories → Developer Tools: WebIDE
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.
Summary: UX: connecting to a device and then an app should be faster → Auto-select and connect to last used runtime if available
Attached patch Initial Attempt (obsolete) — Splinter Review
Attaching Alex's patch from bug 1055279 as initial work.
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...
Attachment #8487327 - Flags: feedback?(jryans)
Alex, I've added some comments above.  If you don't have time for this anymore, let me know.
Flags: needinfo?(poirot.alex)
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
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8487327 - Attachment is obsolete: true
Attachment #8490845 - Flags: review+
Attachment #8490847 - Flags: review?(jryans)
Flags: needinfo?(poirot.alex)
Attached patch patch v3 (obsolete) — Splinter Review
Forgot to rename lastConnectedRuntimeID to lastConnectedRuntime in webide-prefs.js

https://tbpl.mozilla.org/?tree=Try&rev=c52bf2aae49b
Attachment #8490847 - Attachment is obsolete: true
Attachment #8490847 - Flags: review?(jryans)
Attachment #8491013 - Flags: review?(jryans)
Assignee: nobody → poirot.alex
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", "adbhelper@mozilla.org");
>  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...
Attachment #8491013 - Flags: review?(jryans) → review+
Attached patch patch v4 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=30e7599a73cc
Attachment #8491013 - Attachment is obsolete: true
Attachment #8491460 - Flags: review+
Attached patch patch v5Splinter Review
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
Attachment #8491460 - Attachment is obsolete: true
Attachment #8491518 - Flags: review?(jryans)
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?
(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 :/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c10eea79e559
https://hg.mozilla.org/mozilla-central/rev/9119c622fdf3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Whiteboard: [status:landedon]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.