[CLOSED] Identify the main Brackets window the right way

[core-ai-bot]

Issue by RaymondLim
Monday Mar 18, 2013 at 20:11 GMT
Originally opened as adobe#220

---

This fixes #3047. The old code was on the wrong assumption that the first item in the window map is always the main window and therefore we were sometimes sending "close window" messages incorrectly to the main window.

---

RaymondLim included the following code: https://github.com/adobe/brackets-shell/pull/220/commits

[core-ai-bot]

Comment by redmunds
Tuesday Mar 19, 2013 at 01:30 GMT

---

This code is much better than the "main window is the first in list" code that I wrote, and I verified that the bug is fixed (but I had to update recipe to repro).

I was going to suggest that m_Browser and m_BrowserId be renamed to something like m_MainBrowser and m_MainBrowserId to make this code easier to understand. Then I realized that this code is shared between Windows and Mac, so simply renaming those variables may cause confusion on Mac.

Since this code is shared on both platforms, I also did some testing on Mac and I found this bug related to those variables:

1. Start Brackets on Mac
2. Open a second window with: Debug > New Brackets Window
3. Close first window

Results
On Mac the second window stays open when you close the first window, but the Brackets > About Brackets menu item is disabled.

Expected
If second window is kept open, then it has complete functionality as found in first window.

I searched through the menus and couldn't find any other problems, but I also looked through the Mac-specific code and it seems to check GetBrowserId() (which is only set for first window on both Mac and Win) in many places, so there may be some other problems.

First, I think this code should be fixed on the Mac. After that's done, then the variables may need to be renamed and better documented to make this code easier to understand.

Done with initial review.

[core-ai-bot]

Comment by RaymondLim
Tuesday Mar 19, 2013 at 20:55 GMT

---

@redmunds

The issue you found on Mac can also be reproduced in sprint 15 build. And the menu item is from Mac resource file and its corresponding command may not be implemented for popup windows. So please log a new issue for that since it has nothing to do with the "closing window in the wrong order" issue. You can skip step 3, just mention that switching to the second window will disable Brackets > About Brackets menu items.

[core-ai-bot]

Comment by redmunds
Tuesday Mar 19, 2013 at 21:16 GMT

---

Yes, I know that bug was not caused by your change. But the code needs to be cleaned up to make it easier to understand. It the variable names were more clear, this bug may not have been introduced. And since the code cleanup effects the other bug, it's easiest to do it all at once. If you are busy with other tasks, then I can fix it.

[core-ai-bot]

Comment by RaymondLim
Tuesday Mar 19, 2013 at 21:22 GMT

---

You're welcome to fix the new issue. I did look into it this morning, but still don't know how to get it working w/o a crash when the main window is closed. And fixing this doesn't really clean up any code, it is just adding the missing command for the popup window.

Not quite sure you want me to rename the two variables with this pull request. Let me know if you want to.

[core-ai-bot]

Comment by redmunds
Wednesday Mar 20, 2013 at 00:23 GMT

---

I think it would be relatively easy to fix the Mac using ClientHandler::GetBrowserForNativeWindow(). It needs to be wrapped in a separate method called something like GetFrontmostBrowser(). Unfortunately, this touches a lot of code, so it's too much effort and risk for now.

Usage of m_Browser and m_BrowserId didn't change, but I added documentation of how they are used.

@RaymondLim You're code is ready to merge. So, please review my changes and give me comments, or merge.

[core-ai-bot]

Comment by RaymondLim
Wednesday Mar 20, 2013 at 19:00 GMT

---

It looks fine with your comments on m_Browser and m_BrowserId. You can merge it now.

[core-ai-bot]

Comment by redmunds
Wednesday Mar 20, 2013 at 19:40 GMT

---

Merging.