Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 356252

Summary:

[Compatibility] Closing all the views in a perspective without a shared area closes the perspective

Product:

[Eclipse Project] Platform

Reporter:

Remy Suen <remy.suen>

Component:

Assignee:

Platform UI Triaged <platform-ui-triaged>

Status:

VERIFIED FIXED

QA Contact:

Eric Moffatt <emoffatt>

Severity:

major

Priority:

CC:

emoffatt, rollsisu

Version:

4.2

Target Milestone:

4.2 M3

Hardware:

OS:

Windows XP

Whiteboard:

Bug Depends on:

Bug Blocks:

350080

Attachments:

Description	Flags
workaround patch	none

Description Remy Suen

2011-08-30 16:57:06 EDT

1. Make a perspective that doesn't have a shared area.

pageLayout.setEditorAreaVisible(false);

2. Open it.
3. Close all the views.
4. The perspective gets closed too.

Eric, I thought you added code for this case? I certainly remember you mentioning this problem to me before.

Comment 1 Eric Moffatt

2011-08-31 10:42:16 EDT

Yes, I had thought so myself...I may use this defect to implement a more general tag-based approach that Tom suggested (he wants specific control over whether a stack collapses (TBR => false) and/or is removed when it's 'empty'.

I'll take a look for this specific case first though...

Comment 2 Eric Moffatt

2011-09-15 12:52:55 EDT

M2 is done...

Comment 3 Ben Roling

2011-09-15 16:16:04 EDT

Created attachment 203451 [details]
workaround patch

This issue has been biting me in an application I am porting to E4.  This is a patch I have used as a workaround.

Comment 4 Remy Suen

2011-09-29 10:31:37 EDT

(In reply to comment #3)
> This issue has been biting me in an application I am porting to E4.

Jubula, from the Juno simultaneous release train, has a 'Functional Test Execution' perspective that also has this problem as it doesn't have a shared area.

Comment 5 Eric Moffatt

2011-09-29 15:52:23 EDT

Ben, thanks for the patch !

This is indeed a case where we're being over-zealous in cascading the TBR flag.

I think the correct fix may be a little higher up in the code (line 255) where we skip all the processing based on type.

Aaargh, I'm trying to check in the code but I'm fighting GIT...I'll attach a patch for now so you can make sure this change works for you.

Comment 6 Ben Roling

2011-09-30 09:43:26 EDT

Thanks Eric.  Line 255 seems like its probably a better place to make the correction.  I didn't place my change there because I wasn't sure if it was important to flip TBR true if a child of the perspective has TBR flipped true, but that is likely just due to my lack of experience with the platform code.

Comment 7 Eric Moffatt

2011-09-30 14:56:02 EDT

Pushed in >20110930.

commit d49acf4e4a216ba93d0f21bf08f09cef5ef84dbd

This uses the check outside of the 'isToBeRendered' if...

Ben, this works for me but if you would like to check in your scenarios please do and re-open if you find issues.

Comment 8 Eric Moffatt

2011-09-30 14:59:23 EDT

Ben, I understand (the motto 'safety first' comes to mind...;-), the main reason to put the code outside the if is that it makes the handling more symmetrical...we will *neither* hide nor show something we were ignoring.

Otherwise we could end up in a situation where we *were* hiding something automagically when it went 'empty' but not re-rendering it when a child became visible...

Comment 9 Ben Roling

2011-09-30 17:02:12 EDT

I agree that it makes more sense to have the code behavior symmetrical.  I was just explaining the rationale for why I coded my patch the way I did.

I wasn't trying to convince you the change should be done as in the patch.  I just wanted to make sure you were aware in case it might trip you to think of some other place that may end up needing to be changed as a result of this.  It doesn't seem like anything else should be relying on this but you would probably know better than I.

Comment 10 Eric Moffatt

2011-10-26 13:39:59 EDT

Verified in I20111025-2000.