Closed
Bug 1055666
Opened 10 years ago
Closed 10 years ago
Re-select project on connect if last project is runtime app
Categories
(DevTools Graveyard :: WebIDE, defect)
DevTools Graveyard
WebIDE
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: jryans, Assigned: ochameau)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [status:landedon])
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
If you're already connected, and then pick a runtime app, we open it for you.
We should do the same if a runtime app was already selected at the time of connection.
Reporter | ||
Updated•10 years ago
|
Blocks: gaia-devtools
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
Sole, this is related to bug 1055022, so I was curious what you thought.
Instead of "deactivated" the selected runtime app, it would remain selected, unless the user changes it. Then, if there is one selected still when connecting again, it's reopened, to get you closer to the state you had before with less steps.
Does that seem good, or is it maybe too "magical"?
Flags: needinfo?(sole)
Comment 2•10 years ago
|
||
Oh that would be neat... I guess! I think it sounds better than closing/unselecting apps as I suggested on bug 1055022
Flags: needinfo?(sole)
Reporter | ||
Updated•10 years ago
|
Summary: Re-open on connect if selected project is runtime app → Re-select project on connect if last project is runtime app
Reporter | ||
Comment 3•10 years ago
|
||
Attaching Alex's patch from bug 1055279 as initial work here.
Reporter | ||
Updated•10 years ago
|
Attachment #8487324 -
Flags: feedback?(jryans)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8487324 [details] [diff] [review]
Initial Attempt
Review of attachment 8487324 [details] [diff] [review]:
-----------------------------------------------------------------
As with bug 1045660, let's move it to webide.js.
::: browser/devtools/webide/modules/app-manager.js
@@ +308,5 @@
> this.selectedProject.type == "hosted") {
> this.validateProject(this.selectedProject);
> }
> + // Remember the last selected project
> + let lastProject = "";
We can do this work in a new method when webide.js gets the "project" event.
@@ +310,5 @@
> }
> + // Remember the last selected project
> + let lastProject = "";
> + if (this.selectedProject.type == "runtimeApp") {
> + lastProject = this.selectedProject.app.manifestURL;
Let's store the type name too.
@@ +384,5 @@
> }, deferred.reject);
>
> + // Automatically reconnect to the previously selected project
> + if (!this.selectedProject) {
> + this.webAppsStore.once("store-ready", () => {
AppManager emits the "runtime-apps-found" event for this case.
webide.js doesn't do anything for that event yet, but you can add something for this purpose.
Attachment #8487324 -
Flags: feedback?(jryans)
Reporter | ||
Comment 5•10 years ago
|
||
Alex, I've added some comments above. If you don't have time for this anymore, let me know.
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 6•10 years ago
|
||
Unfortunately for this patch, I wasn't able to easily provide a test.
There is no test so far involving runtime apps
and don't have a clear picture on how to fake one in a test.
https://tbpl.mozilla.org/?tree=Try&rev=c548de9085ff
Attachment #8487324 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8490848 -
Flags: review?(jryans)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → poirot.alex
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8490848 [details] [diff] [review]
patch v2
Review of attachment 8490848 [details] [diff] [review]:
-----------------------------------------------------------------
I'd like to check it again with the adjusted logic.
::: browser/devtools/webide/content/webide.js
@@ +560,5 @@
> + } else if (AppManager.selectedProject.type == "mainProcess") {
> + type = "mainProcess";
> + }
> + }
> + Services.prefs.setCharPref("devtools.webide.lastSelectedProject",
It's a bit tricky to determine the right logic here...
If there *is* a selected project, but the |type| is not "runtimeApp" or "mainProcess", we should clear the pref I think, as this means a local project was last selected. Currently it gets set to ":", which then gives a TypeError during the match.
Also, if there is no selected project (which happens if you disconnect the runtime), I think we should leave the pref alone and do nothing. Currently we again are setting it to ":".
@@ +572,5 @@
> + let pref = Services.prefs.getCharPref("devtools.webide.lastSelectedProject");
> + if (!pref) {
> + return;
> + }
> + let [_, type, project] = pref.match(/^(\w+):(.*)$/);
Maybe try is needed? Somehow we should prevent the TypeError in case of strings like ":".
Attachment #8490848 -
Flags: review?(jryans)
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=30e7599a73cc
(In reply to J. Ryan Stinnett [:jryans] from comment #8)
> Comment on attachment 8490848 [details] [diff] [review]
>
> It's a bit tricky to determine the right logic here...
>
> If there *is* a selected project, but the |type| is not "runtimeApp" or
> "mainProcess", we should clear the pref I think, as this means a local
> project was last selected. Currently it gets set to ":", which then gives a
> TypeError during the match.
>
> Also, if there is no selected project (which happens if you disconnect the
> runtime), I think we should leave the pref alone and do nothing. Currently
> we again are setting it to ":".
I think having two distinct prefs for local and runtime is counter productive.
So I ended up merging into just one, lastSelectedProject,
which can handle runtime, mainprocess, package and hosted.
And the pref is now set only if we support saving/restoring the selected project,
otherwise we reset the pref to the default empty value.
>
> @@ +572,5 @@
> > + let pref = Services.prefs.getCharPref("devtools.webide.lastSelectedProject");
> > + if (!pref) {
> > + return;
> > + }
> > + let [_, type, project] = pref.match(/^(\w+):(.*)$/);
>
> Maybe try is needed? Somehow we should prevent the TypeError in case of
> strings like ":".
Instead of try, I first return the match object and bail out if it is empty.
Attachment #8490848 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8491463 -
Flags: review?(jryans)
Assignee | ||
Comment 10•10 years ago
|
||
New try, another patch assiociated in the run was failing:
https://tbpl.mozilla.org/?tree=Try&rev=d7fe90600344
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8491463 [details] [diff] [review]
patch v3
Review of attachment 8491463 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/devtools/webide/content/webide.js
@@ -79,5 @@
>
> this.setupDeck();
> },
>
> - openLastProject: function() {
I assume this should not be in the diff...
@@ +534,5 @@
> yield UI.createToolbox();
> });
> },
>
> + saveLastSelectedProject: function() {
I believe the logic is still not right in the case that you:
1. Connect to runtime
2. Choose runtime app
3. Disconnect manually
4. Reconnect
because it will clear the pref at disconnect time. If you need to, you could test AppManager's connection state.
@@ +538,5 @@
> + saveLastSelectedProject: function() {
> + // Remember the last selected project on the runtime
> + let project = "", type = "";
> + let selected = AppManager.selectedProject;
> + if (selected) {
I hope to make all this conversion junk less depressing with project-agnostic work...
@@ +554,5 @@
> + if (type) {
> + Services.prefs.setCharPref("devtools.webide.lastSelectedProject",
> + type + ":" + project);
> + } else {
> + Services.prefs.clearUserPref("devtools.webide.lastSelectedProject", "");
No 2nd arg.
@@ +562,5 @@
> + autoSelectProject: function() {
> + if (AppManager.selectedProject) {
> + return;
> + }
> + let shouldRestore = Services.prefs.getBoolPref("devtools.webide.restoreLastProject");
This is to expose this as a user configurable option? I think that's a good idea. But then you should add a default value for this pref, and expose a control for it in WebIDE's settings.
Also, you could check it when saving and skip that if it's disabled.
Attachment #8491463 -
Flags: review?(jryans)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> Comment on attachment 8491463 [details] [diff] [review]
> patch v3
>
> Review of attachment 8491463 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/devtools/webide/content/webide.js
> @@ -79,5 @@
> >
> > this.setupDeck();
> > },
> >
> > - openLastProject: function() {
>
> I assume this should not be in the diff...
>
No, no it is expected! See next comment.
> @@ +562,5 @@
> > + autoSelectProject: function() {
> > + if (AppManager.selectedProject) {
> > + return;
> > + }
> > + let shouldRestore = Services.prefs.getBoolPref("devtools.webide.restoreLastProject");
>
> This is to expose this as a user configurable option? I think that's a good
> idea. But then you should add a default value for this pref, and expose a
> control for it in WebIDE's settings.
>
> Also, you could check it when saving and skip that if it's disabled.
So I'm merging the existing "save last local project" with this new feature to "save last runtime project".
"save last local project" already uses "restoreLastProject" pref and also "projectlocation" one.
It is implemented within openLastProject that I removed in favor of autoSelectProject.
In this patch, I'm still using restoreLastProject pref, but depecated projectlocation one in favor of lastSelectedProject one. So when updating, users are going to loose their last selected local project once. (I'm not sure it is worth putting migration code for that?)
Assignee | ||
Comment 13•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f5d405f98414
Fixed the issue when disconnecting manually.
I also had to fix a bug within AppActorFront.
When reconnecting to a simulator more than once,
we were incorrectly caching AppActorFront.
That ended up introducing exceptions for requests
being made to the second instance of the simulator.
(Because the client was the old one, for the closed simulator)
Attachment #8491463 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8493139 -
Flags: review?(jryans)
Reporter | ||
Updated•10 years ago
|
Blocks: rm-connect
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8493139 [details] [diff] [review]
patch v4
Review of attachment 8493139 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/devtools/webide/content/webide.js
@@ -79,5 @@
>
> this.setupDeck();
> },
>
> - openLastProject: function() {
You should also remove the step to set "devtools.webide.lastprojectlocation" in |UI.openProject| and remove the pref from webide-prefs.js, since it is no longer used after this.
@@ +586,5 @@
> + return;
> + }
> + let [_, type, project] = m;
> +
> + if (type == "mainProcess" && AppManager.isMainProcessDebuggable()) {
You need to wait until a runtime is connected before setting main process or runtime app projects, since this function is also called on WebIDE init and you may not yet be connected.
Attachment #8493139 -
Flags: review?(jryans) → review+
Reporter | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
Whiteboard: [status:inflight]
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8494475 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8493139 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Comment 17•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [status:inflight] → [status:inflight][fixed-in-fx-team]
Comment 18•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [status:inflight][fixed-in-fx-team] → [status:inflight]
Target Milestone: --- → Firefox 35
Updated•10 years ago
|
Whiteboard: [status:inflight] → [status:landedon]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•