Bug 354026

Comment 1 Pawel Piech 2011-08-05 13:21:38 EDT

CQ:WIND00279939

Comment 2 Pawel Piech 2011-08-05 14:59:13 EDT

In addition to the fact that activated views can obscure non-debug views that user placed on top, this logic can also bring a view on top of another view that's in the to-be-activated list.

This is due to the bug in the logic that brings the view to the top: the views are activated based on view-context bindings.  However the contexts are activated one by one though the context service, and a view associated with one context will be opened on top of another view which is associated with another context.  I don't see any way to solve this problem since IContextService does not have a method to set activate multiple contexts at once.

Comment 3 Pawel Piech 2011-08-05 15:52:14 EDT

I'd like to target this for 3.8, though not 3.7.x since it's technically a change of behavior.  Any objections?

Comment 4 Pawel Piech 2011-08-05 16:44:14 EDT

Created attachment 201024 [details]
Patch with fix.

This patch simply removes the bring-to-top logic.

Comment 5 Michael Rennie 2011-08-08 09:12:34 EDT

My worry with this fix is that people / products that do like / require the view activation will now be broken - the proposed fix effectively reverts the fixes for bug 51635 and bug 57164.

If you really want to give users the ability to stop the auto-activation, than I think a preference is the only way to go.

Comment 6 Pawel Piech 2011-08-08 12:17:17 EDT

(In reply to comment #5)
> My worry with this fix is that people / products that do like / require the
> view activation will now be broken - the proposed fix effectively reverts the
> fixes for bug 51635 and bug 57164.

The debug view behavior uses a different mechanism, but the precedent from 57164 is more relevant.  After debugging this problem a while, I think the biggest problem for our product is actually the bug related to activating views bound to multiple different contexts.  I'll play around with it some more and see if I can work around this problem behavior, rather than nix the whole feature.

Comment 7 Pawel Piech 2011-08-08 18:48:16 EDT

Created attachment 201112 [details]
More narrow fix for checkZOrder().

I wrote a more narrow fix for the view activation problem.  It only addresses the bug where a view is brought to top even when another view that is used for debugging is already on top.  My initial analysis was wrong and I did not need to worry about activating multiple contexts through context service.

Comment 8 Pawel Piech 2011-08-08 18:49:53 EDT

Created attachment 201113 [details]
PDA patch for testing.

I used the following patch to reproduce and test the bug.  It causes the outline view to be activated for the PDA debugger through a secondary debug model ID.

Comment 9 Pawel Piech 2011-08-08 18:50:24 EDT

Hi Mike,
I'd like to target this fix for 3.7.1 if it's not deemed too risky.  What do you think?

Comment 10 Pawel Piech 2011-08-17 13:37:42 EDT

Hi Mike,  
If you have time to review this fix before 3.7.1 RC2 next week can please commit it?  This bug fell off my radar last week but it's important to our product so I'd like to get the fix into the maintenance release.

Comment 11 Michael Rennie 2011-08-22 16:54:31 EDT

A couple of things:
1. the patch does not apply to 3.7.1
2. in doActivation we have:

 // activate the view bindings
 for (int i = 0; i < fAllViewBindingIds.length; i++) {
    String viewId = fAllViewBindingIds[i];
    ViewBinding binding = (ViewBinding) fAllViewIdToBindings.get(viewId);
    binding.activated(page, perspective);
  }
  // bring most relevant views to top
  for (int i = 0; i < fAllViewBindingIds.length; i++) {
    String viewId = fAllViewBindingIds[i];
    ViewBinding binding = (ViewBinding) fAllViewIdToBindings.get(viewId);
    binding.checkZOrder(page, allViewIds);
  }

could we not condense that to be:

for (int i = 0; i < fAllViewBindingIds.length; i++) {
  String viewId = fAllViewBindingIds[i];
  ViewBinding binding = (ViewBinding) fAllViewIdToBindings.get(viewId);
  binding.activated(page, perspective);
  binding.checkZOrder(page, allViewIds);
}
3. other than 1. and 2. the patch seems to work Ok.

Comment 12 Pawel Piech 2011-08-23 12:09:54 EDT

(In reply to comment #11)
> A couple of things:
> 1. the patch does not apply to 3.7.1
> 2. in doActivation we have:
I can rework the patch for 3.7 and combine the loops later today.  BTW, I thought the 3.7 RC2 build was this week, but it's actually next week according to calendar so I guess I have a little more time.

Comment 13 Michael Rennie 2011-08-23 15:13:43 EDT

This will have to wait for 3.7.2, the RC is tomorrow:
http://www.eclipse.org/eclipse/development/plans/freeze_plan_3_7_1.php

Comment 14 Michael Rennie 2011-08-29 16:37:34 EDT

(In reply to comment #13)
> This will have to wait for 3.7.2, the RC is tomorrow:
> http://www.eclipse.org/eclipse/development/plans/freeze_plan_3_7_1.php

I should have mentioned that you could still ask for PMC approval if you wanted it in 3.7.1.

Comment 15 Pawel Piech 2011-08-29 18:40:25 EDT

(In reply to comment #13)
> This will have to wait for 3.7.2, the RC is tomorrow:
> http://www.eclipse.org/eclipse/development/plans/freeze_plan_3_7_1.php

The calendar has let me down before, I should know better than to trust it.  Not a big deal I think since we can pick up an interim maintenance build.

I committed the fix with the suggested modification and I got an NPE during more testing, so I fixed that too (so I'm glad I hadn't committed for 3.7.1 and to have a little extra time testing in 3.7.2).

Thanks for reviewing and creating the dup bug.  If forgot again we were doing that.

Comment 16 Michael Rennie 2011-08-31 11:39:47 EDT

marking fixed.

Comment 17 Curtis Windatt 2011-09-13 17:07:04 EDT

Verified using I20110912-2126 and Pawel's example