Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 361153 - Dispose Page and PageRec in PageBookView.doDestroyPage(IWorkbenchPart, PageRec)
Summary: Dispose Page and PageRec in PageBookView.doDestroyPage(IWorkbenchPart, PageRec)
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.8   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Platform-UI-Inbox CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords:
Depends on:
Blocks: 367663
  Show dependency tree
 
Reported: 2011-10-17 10:54 EDT by Tomasz Zarna CLA
Modified: 2019-11-14 03:11 EST (History)
2 users (show)

See Also:


Attachments
Fix for eclipse.platform.ui v01 (4.02 KB, patch)
2011-12-30 12:26 EST, Tomasz Zarna CLA
no flags Details | Diff
mylyn/context/zip (18.58 KB, application/octet-stream)
2011-12-30 12:26 EST, Tomasz Zarna CLA
no flags Details
Fix for eclipse.platform.text v01 (910 bytes, patch)
2011-12-30 12:26 EST, Tomasz Zarna CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomasz Zarna CLA 2011-10-17 10:54:37 EDT
Majority if not all subclasses of PageBookView do pretty much the same thing in their implementation #doDestroyPage:

protected void doDestroyPage(IWorkbenchPart part, PageRec pageRecord) {
	IPage page = pageRecord.page;
	page.dispose();
	pageRecord.dispose();
	// ... other stuff
}

I was wondering if we could pull up these 3 lines to PageBookView.doDestroyPage (making it non-abstract) so all subclasses can just call super.doDestroyPage(...) first and proceed with their own stuff.

It would be an API change though, wouldn't it?
Comment 1 Remy Suen CLA 2011-10-17 13:45:36 EDT
(In reply to comment #0)
> It would be an API change though, wouldn't it?

According to the wiki page, making abstract methods non-abstract is safe.
http://wiki.eclipse.org/Evolving_Java-based_APIs_2
Comment 2 Paul Webster CLA 2011-11-02 10:25:23 EDT
Because it was abstract we've basically forced clients to override that method.  Changing it would provide a method no current client calls.


Although arguably if we did provide  a default and allowed it to be overridden or extended, we would offer a default for any new consumers of this class.

Remy, how would this fit in with the e4 MPart pattern?  Is this something that's easily translated to @PreDestroy?

PW
Comment 3 Paul Webster CLA 2011-11-02 12:23:56 EDT
Asked for a clarification :-)

1) it won't effect anything currently written.  Given that, is it worth it?

2) When Eclipse4 starts to expose true nesting (MPart that contains multiple MParts managed by the framework) will this cause any problems, as the nested parts still care about @PreDestroy not a dispose() method?

PW
Comment 4 Tomasz Zarna CLA 2011-11-03 07:32:09 EDT
(In reply to comment #3)
> 1) it won't effect anything currently written.  

This may be considered as a good thing, right? :)

> Given that, is it worth it?

Well, it's an enhancement request to avoid code duplication in any new PageBookView subclasses (like the new HistoryView). It's definitely not a must-have, but I can provide patches to all current implementations of the PBV, no big deal.
Comment 5 Remy Suen CLA 2011-11-03 08:00:04 EDT
(In reply to comment #3)
> 2) When Eclipse4 starts to expose true nesting (MPart that contains multiple
> MParts managed by the framework) will this cause any problems, as the nested
> parts still care about @PreDestroy not a dispose() method?

In 3.x, the programming model is to write some dispose() method that the framework will call when your object is no longer needed.

In 4.x, you would just tag some method with @PreDestroy. I don't feel that this PBV request is related to 4.x. Even if you're a part in a part, you would still have a @PreDestroy method. In 3.x we call dispose(), in 4.x we call @PreDestroy, I don't think the introduction of this API would change how people perceive the two different methods of cleaning up their resources.
Comment 6 Paul Webster CLA 2011-11-03 16:01:17 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > 1) it won't effect anything currently written.  
> 
> This may be considered as a good thing, right? :)

OK, if that's the point (make it easier going forward) then we can consider this for 4.2

PW
Comment 7 Tomasz Zarna CLA 2011-12-30 12:26:06 EST
Created attachment 208875 [details]
Fix for eclipse.platform.ui v01
Comment 8 Tomasz Zarna CLA 2011-12-30 12:26:12 EST
Created attachment 208876 [details]
mylyn/context/zip
Comment 9 Tomasz Zarna CLA 2011-12-30 12:26:50 EST
Created attachment 208877 [details]
Fix for eclipse.platform.text v01
Comment 10 Lars Vogel CLA 2019-11-14 03:11:54 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

If the bug is still relevant, please remove the "stalebug" whiteboard tag.