[IDE] Opening a duplicate file should not open a second or more instances

VERIFIED FIXED

Status

--
critical
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: aaronmt, Assigned: harth)

Tracking

({dataloss})

Details

(Whiteboard: [mozmill-1.4.2+])

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
In 1.4.2b3, the IDE will allow one to open a duplicate file more than once and create a new reference to it in the list of open files. We should be smarter here and just switch to the already open file.
(Reporter)

Updated

8 years ago
Whiteboard: [mozmill-1.4.2?]
(Assignee)

Comment 1

8 years ago
Don't think this should be blocking.

I actually disagree with this, except for bug 487588 (we save the test file to disk everytime we run it, could lead to unexpected results if you have two open). Switching to the open file would only be okay if we gave the user an option to open another reference to the same file, like the switch-to-tab functionality in the awesomebar of Firefox.
(Assignee)

Updated

8 years ago
Whiteboard: [mozmill-1.4.2?] → [mozmill-1.4.2-]
(In reply to comment #1)
> open). Switching to the open file would only be okay if we gave the user an
> option to open another reference to the same file, like the switch-to-tab
> functionality in the awesomebar of Firefox.

We shouldn't allow opening the same file in another tab at all. What Aaron said, instead of creating a new tab with the same test file as content, we should switch to an already open but hidden tab, which contains the to load test file already.

Right now, this bug can cause data loss if we don't watch your steps closely enough.
Severity: normal → critical
Keywords: dataloss
(Reporter)

Comment 3

8 years ago
Tabs?

I'm talking about the single drop-down list of open files.
(Assignee)

Comment 4

8 years ago
(In reply to comment #2)
> We shouldn't allow opening the same file in another tab at all. What Aaron
> said, instead of creating a new tab with the same test file as content, we
> should switch to an already open but hidden tab, which contains the to load
> test file already.

Why? I can see wanting to try out two different forks of the same test, that's like saying you don't want to have two Gmails open in Firefox, most of the time you don't, but maybe you want to have two different contexts.

> Right now, this bug can cause data loss if we don't watch your steps closely
> enough.

No more data loss than before though.
(Reporter)

Comment 5

8 years ago
(In reply to comment #4)
> (In reply to comment #2)
> > We shouldn't allow opening the same file in another tab at all. What Aaron
> > said, instead of creating a new tab with the same test file as content, we
> > should switch to an already open but hidden tab, which contains the to load
> > test file already.
> 
> Why? I can see wanting to try out two different forks of the same test, that's
> like saying you don't want to have two Gmails open in Firefox, most of the time
> you don't, but maybe you want to have two different contexts.
> 
> > Right now, this bug can cause data loss if we don't watch your steps closely
> > enough.
> 
> No more data loss than before though.

Yes but it's an editor and we should duplicate that behaviour that is expected in editors. For example, I am testing TextMate and TextEdit, a single instance of the running application. I open a file 'a.js' it opens. I opt-in again to open a.js, and rather than two, the program will focus on the already present and currently open file.
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> Yes but it's an editor and we should duplicate that behaviour that is expected
> in editors. For example, I am testing TextMate and TextEdit, a single instance
> of the running application. I open a file 'a.js' it opens. I opt-in again to
> open a.js, and rather than two, the program will focus on the already present
> and currently open file.

I see, good call. I don't know if I would say its blocking, but I could probably get a fix for this before next beta.
(Assignee)

Comment 7

8 years ago
Created attachment 465002 [details] [diff] [review]
switch to file if it is already open
Assignee: nobody → fayearthur+bugs
Attachment #465002 - Flags: review?(jhammel)

Comment 8

8 years ago
Comment on attachment 465002 [details] [diff] [review]
switch to file if it is already open

>diff --git a/mozmill/mozmill/extension/content/editor/editor.js b/mozmill/mozmill/extension/content/editor/editor.js
>index f9c9c54..c0c18b7 100644
>--- a/mozmill/mozmill/extension/content/editor/editor.js
>+++ b/mozmill/mozmill/extension/content/editor/editor.js
>@@ -49,6 +49,16 @@ var editor = {
>     this.switchTab();
>   },
> 
>+  switchToFile : function(filename) {
>+    for(var i = 0; i < this.tabs.length; i++) {
>+      if(this.tabs[i].filename == filename) {
>+        this.switchTab(i);
>+        return true;
>+      }
>+    }
>+    return false;
>+  },
>+
>   openNew : function(content, filename) {
>     if(!filename) {
>       this.tempCount++;
>@@ -85,7 +95,7 @@ var editor = {
> 
>   changeFilename : function(filename) {
>     this.currentTab.filename = filename;
>-    this.showFilename(getBasename(filename));	
>+    this.showFilename(getBasename(filename)); 
>   },
> 
>   onFileChanged : function() {
>diff --git a/mozmill/mozmill/extension/content/menus.js b/mozmill/mozmill/extension/content/menus.js
>index 9307995..b0e46f8 100644
>--- a/mozmill/mozmill/extension/content/menus.js
>+++ b/mozmill/mozmill/extension/content/menus.js
>@@ -52,7 +52,8 @@ function openFile(){
>   var openObj = utils.openFile(window);
>   if (openObj) {
>     $("#tabs").tabs("select", 0);
>-    editor.openNew(openObj.data, openObj.path);
>+    if(!editor.switchToFile(openObj.path))
>+      editor.openNew(openObj.data, openObj.path);
>   }
> }
> 

swithToFile conflates an informational function (is the file already open?) with a side-effect (switching the tab).  Is there any way that it can just check what tab it is, maybe returning -1 if it doesn't exist?  Then have the switchTab call in onFileChanged?

Or can you explain to me why it should be this way?
Attachment #465002 - Flags: review?(jhammel) → review-
(Assignee)

Comment 9

8 years ago
Created attachment 465016 [details] [diff] [review]
seperate switching and checking logic per review comments
Attachment #465016 - Flags: review?(jhammel)

Comment 10

8 years ago
Comment on attachment 465016 [details] [diff] [review]
seperate switching and checking logic per review comments

looks good
Attachment #465016 - Flags: review?(jhammel) → review+
(Assignee)

Comment 11

8 years ago
marking blocking since we have a fix.
Whiteboard: [mozmill-1.4.2-] → [mozmill-1.4.2+]
(Reporter)

Comment 13

8 years ago
Verified Fixed, opening a duplicate file will focus on the already open file now and not open a second. Thanks!
Status: RESOLVED → VERIFIED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.