Community
Participate
Working Groups
I20140408-1645 and I20140415-0800 Seems to work fine for me up to I20140402-0100, but others have had it earlier (bug 394505, bug 397064). - new install of SDK, new workspace (important, since E4 seems to cache stuff across workspaces) - Help > Install New Software..., use http://download.eclipse.org/egit/updates or Add... > Archive... and use a local copy of http://www.eclipse.org/downloads/download.php?file=/egit/updates/org.eclipse.egit.repository-3.3.1.201403241930-r.zip - install the "Eclipse Git Team Provider" feature - click Yes to restart - close Welcome - Alt+Shift+Q, Q > open Git Repositories view - add an existing local Git repo => Git Repositories view sometimes has most of its toolbar buttons duplicated (similar to bug 402561) - select repo, open context menu => Context menu is often OK, but sometimes almost empty (see below) - choose context menu > Show In > History - click the "Git Repositories" view tab - open context menu => menu is empty except for "Show In" and "Paste Repository Path" items (and sometimes "Compare With"). This is similar to bug 397064. After a restart, the context menu is usually there again, but it disappears again after a while. Closing and reopening the view also helps sometimes, but it's unclear when (bug 430306).
Nothing in the log. I've also seen this in I20140408-1645 on GTK with an old version of EGit (2.3.1.201302201838-r), so I doubt it's an EGit issue.
This is a blocker, a regression, and must be fixed for M7.
I'm working on this at the moment.
Thanks Wojciech.
With regards to the duplicated toolbar buttons problem, I can no longer reproduce it (N20140416-2200).
(In reply to Wojciech Sudol from comment #5) > With regards to the duplicated toolbar buttons problem, I can no longer > reproduce it (N20140416-2200). Yup, I also can't reproduce that reliably. BTW: The missing menus problem also shows up in the History view after a while. I can't compare commits any more or compare an old version with the workspace. Interesting fact: In the Quick Diff menu, I can still reset the baseline to HEAD or HEAD^, but I can't set a specific commit as baseline any more. Looks like the problem is that the commands don't get the current selection any more or are no longer updated on selection changes.
The problem with empty menu started to occur after the commit 0b8935d41933c2c0237d88e7fc8ab4374a2fb818 with a fix for bug 354538 - "[Contributions] [Compatibility] Window menus and toolbars leaked after having been closed". General scenario to reproduce the bug: 1. Open Git Repositories view and add an existing repository 2. Close and reopen the view 3. Open context menu on the repository item The only workaround I found is to restart Eclipse with the Git Repositories view open.
(In reply to Wojciech Sudol from comment #7) > The problem with empty menu started to occur after the commit > 0b8935d41933c2c0237d88e7fc8ab4374a2fb818 with a fix for bug 354538 - > "[Contributions] [Compatibility] Window menus and toolbars leaked after > having been closed". > General scenario to reproduce the bug: > 1. Open Git Repositories view and add an existing repository > 2. Close and reopen the view > 3. Open context menu on the repository item > The only workaround I found is to restart Eclipse with the Git Repositories > view open. Since we have some workaround for that let me decrease the severity of the bug a bit Daniel
It seems that the bug 432637 is another effect of this change. I can reproduce the bug only when I run Eclipse with this patch.
(In reply to Wojciech Sudol from comment #9) > It seems that the bug 432637 is another effect of this change. I can > reproduce the bug only when I run Eclipse with this patch. I've made the initial investigation and both issues are not caused by the changes introduces in the bug 354538 Basically the context menu disappears when particular view (i.e. Git) either becomes inactive or gets closed. I will continue investigation of the issue Daniel
*** Bug 432637 has been marked as a duplicate of this bug. ***
OK, I've found the cause of the issue with disappearing context menu of the Git view. The regression has been introduced with the Bug 431714 - the changes in the ModelServiceImpl class. Paul, Do you want to fix it now or we can just revert the commit and return to it at the beginning of the Mars? Daniel
(In reply to Daniel Rolka from comment #12) > OK, I've found the cause of the issue with disappearing context menu of the > Git view. The regression has been introduced with the Bug 431714 - the > changes in the ModelServiceImpl class. What did your investigation reveal? Were you just checking which commit did it, or did you trace the code? The commit from bug 431714 is http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=cf7be5c8f93e003afa54c4928cda1d57130ce10f Maybe in changing some of the conditions we broke part menus: - if (searchRoot instanceof MPart && (searchFlags & IN_PART) != 0) { + if (searchRoot instanceof MPart) { ... + if (searchFlags != IN_MAIN_MENU) {
(In reply to Paul Webster from comment #13) > (In reply to Daniel Rolka from comment #12) > > OK, I've found the cause of the issue with disappearing context menu of the > > Git view. The regression has been introduced with the Bug 431714 - the > > changes in the ModelServiceImpl class. > > What did your investigation reveal? Were you just checking which commit did > it, or did you trace the code? > > The commit from bug 431714 is > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=cf7be5c8f93e003afa54c4928cda1d57130ce10f > > Maybe in changing some of the conditions we broke part menus: > > - if (searchRoot instanceof MPart && (searchFlags & IN_PART) != 0) { > + if (searchRoot instanceof MPart) { > > ... > > + if (searchFlags != IN_MAIN_MENU) { I was looking for commits that could break it and I found this one. After reverting it, the context menu works fine. Yes, changes in the condition look suspicious. I was asking if we are going to fix it or revert the change since we can expect other regressions in other places introduced by the change that we are not aware at this moment, Daniel
(In reply to Daniel Rolka from comment #14) > I was asking if we are going to fix it or revert the change since we can > expect other regressions in other places introduced by the change that we > are not aware at this moment, I would prefer if we fix this issue, instead of reverting the new search API. If we revert, customers need to write this code themself and will have similar issues.
(In reply to Lars Vogel from comment #15) Where is the popup menu created? I tried to write a JUnit test for it but if I add the popup menu directly to the application model I get the correct result via the model search service. It only looks as if the IN_PARTS flag is not directly supported without another flag. Not sure if that is intended, I created Bug 433232 for clarification.
(In reply to Lars Vogel from comment #16) > (In reply to Lars Vogel from comment #15) > > Where is the popup menu created? I tried to write a JUnit test for it but if > I add the popup menu directly to the application model I get the correct > result via the model search service. Popup menus from the workbench are created in PopupMenuExtender. What search is being called that then fails? Do we know that yet? PW
Adding back the "&& (searchFlags & IN_PART) != 0" on line 271 fixes the problem from comment #7. Louis-Michel, why was it removed? Somehow returning the part's menus in this code block is causing them to not be rendered. I wasn't able to find which findRecursiveElements(*) call was involved, and it is important to know what is going on here. PW
When you add back the "old" flag test If you execute the test: org.eclipse.e4.ui.tests.application.EModelServiceFindTest in the method: testFindElementsCombinations elements = modelService.findElements(application, null, EModelService.IN_PART, getSelector("singleValidId")); assertEquals(0, elements.size()); you will get an assertion fail because it will retrieve a TrimmedWindow. In this test my expectation is to retrieve 0 element, because there is not element named "singleValidId" in a part (IN_PART) If my understanding of the model is correct, MWindows can't be in a part, and this is why I added a test for this. Hope this is clear. LMM
I added comments on Bug 433232. The proposed patch will retrieve model elements with the flag IN_PART (when use without another flag)
(In reply to Louis-Michel Mathurin from comment #20) > I added comments on Bug 433232. The patch on that bug did not solve the missing menus. I'm going to have to revert the entire ModelServiceImpl change if this can't be fixed. M7 wraps up on Friday. PW
IS there a test that exist to reproduce the bug?
(In reply to Louis-Michel Mathurin from comment #22) > IS there a test that exist to reproduce the bug? No, just the steps in comment #7 PW
Created attachment 242264 [details] The XMI
(In reply to Louis-Michel Mathurin from comment #24) > Created attachment 242264 [details] > The XMI I change the code to make the Git RepositoryView work again by adding the flag again...but I can't identify the reason why traversing MMenu children make the menu disappear ! After running the EModelServiceFindTest.class the only 3 tests that are not passing are the one I added myself. I ran the same tests with the "old" code and they are failing too. I included a file in comment number 24, so you can take a look at it and browse the model element and see if you agree with the assertion in the test. Here are three cases where the findElement is not working as expected(from my perspective) when adding back the flag (searchFlags & IN_PART) != 0" clazz = MMenuElement.class; elements = modelService.findElements(application, clazz, EModelService.IN_ANY_PERSPECTIVE, getSelector(clazz)); assertEquals(4, elements.size()); elements = modelService.findElements(application, clazz, EModelService.IN_ACTIVE_PERSPECTIVE, getSelector(clazz)); assertEquals(3, elements.size()); elements = modelService.findElements(application, clazz, EModelService.IN_ANY_PERSPECTIVE | EModelService.IN_MAIN_MENU, getSelector(clazz)); assertEquals(13, elements.size());
(In reply to Louis-Michel Mathurin from comment #25) > (In reply to Louis-Michel Mathurin from comment #24) > > Created attachment 242264 [details] > > The XMI > > I change the code to make the Git RepositoryView work again by adding the > flag again...but I can't identify the reason why traversing MMenu children > make the menu disappear ! > > After running the EModelServiceFindTest.class the only 3 tests that are not > passing are the one I added myself. I ran the same tests with the "old" > code and they are failing too. > > > I included a file in comment number 24, so you can take a look at it and > browse the model element and see if you agree with the assertion in the test. > > Here are three cases where the findElement is not working as expected(from > my perspective) when adding back the flag (searchFlags & IN_PART) != 0" > > clazz = MMenuElement.class; > elements = modelService.findElements(application, clazz, > EModelService.IN_ANY_PERSPECTIVE, getSelector(clazz)); > assertEquals(4, elements.size()); > > elements = modelService.findElements(application, clazz, > EModelService.IN_ACTIVE_PERSPECTIVE, getSelector(clazz)); > assertEquals(3, elements.size()); > > elements = modelService.findElements(application, clazz, > EModelService.IN_ANY_PERSPECTIVE | EModelService.IN_MAIN_MENU, > getSelector(clazz)); > assertEquals(13, elements.size()); I think the related Gerrit review from Louis https://git.eclipse.org/r/#/c/25456/ With this patch, the Git repository menu works again and the three failing tests are commented out.
(In reply to Louis-Michel Mathurin from comment #25) > Here are three cases where the findElement is not working as expected(from > my perspective) when adding back the flag (searchFlags & IN_PART) != 0" > > clazz = MMenuElement.class; > elements = modelService.findElements(application, clazz, > EModelService.IN_ANY_PERSPECTIVE, getSelector(clazz)); > assertEquals(4, elements.size()); If I get the code right, the current logic requires also the IN_PART flag. I think that is to return zero. > elements = modelService.findElements(application, clazz, > EModelService.IN_ACTIVE_PERSPECTIVE, getSelector(clazz)); > assertEquals(3, elements.size()); Same here. I think zero is also OK. > > elements = modelService.findElements(application, clazz, > EModelService.IN_ANY_PERSPECTIVE | EModelService.IN_MAIN_MENU, > getSelector(clazz)); > assertEquals(13, elements.size()); Same here, to get 13 you have to rewrite the test to the following: elements = modelService.findElements(application, clazz, EModelService.IN_ANY_PERSPECTIVE | EModelService.IN_MAIN_MENU | EModelService.IN_PART, getSelector(clazz)); assertEquals(13, elements.size()); This is also in line with the Javadoc of the findMethod <li><b>IN_PART</b> Include MMenu and MToolbar elements owned by parts</;i>. In summary, I think we should apply the Gerrit review from Louis, it fixes the Git repository issue and the above tests are incorrect. ----------- Note for myself: Maybe ANYWHERE needs to include IN_PART and IN_MAIN_MENU?
Released latest fix with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=1993074e222415f655d3782c7f5c051056d7d58c That appears to have fixed the usecase in comment #7 PW
(In reply to Paul Webster from comment #28) > Released latest fix with > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=1993074e222415f655d3782c7f5c051056d7d58c > > That appears to have fixed the usecase in comment #7 > > PW Yes, it fixed the problem but I'm almost sure this fix is causing IAEs on Mac, see bug 433492.
(In reply to Paul Webster from comment #21) > (In reply to Louis-Michel Mathurin from comment #20) > > I added comments on Bug 433232. > > The patch on that bug did not solve the missing menus. I'm going to have to > revert the entire ModelServiceImpl change if this can't be fixed. M7 wraps > up on Friday. > > PW This might be the right thing to do at this point, given all the pain it caused (so far).
I've reverted the optimizations from bug 431714 with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=bb6e9df4cf3e36074fcbd15e4cb1d51fa87232b1 PW
(In reply to Dani Megert from comment #29) > (In reply to Paul Webster from comment #28) > > Released latest fix with > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=1993074e222415f655d3782c7f5c051056d7d58c > > > > That appears to have fixed the usecase in comment #7 > > > > PW > > Yes, it fixed the problem but I'm almost sure this fix is causing IAEs on > Mac, see bug 433492. That was not the case. (In reply to Paul Webster from comment #31) > I've reverted the optimizations from bug 431714 with > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=bb6e9df4cf3e36074fcbd15e4cb1d51fa87232b1 > > PW Shouldn't we reopen bug 431714 then?
(In reply to Dani Megert from comment #32) > > Shouldn't we reopen bug 431714 then? The basic code to find handlers and commands is still there. My commits have just removed the optimizations to other areas not related to handlers. PW
(In reply to Paul Webster from comment #33) > (In reply to Dani Megert from comment #32) > > > > Shouldn't we reopen bug 431714 then? > > The basic code to find handlers and commands is still there. My commits > have just removed the optimizations to other areas not related to handlers. > > PW Cool, thanks Paul!
In 4.4.0.I20140501-0200 PW
