Bug 219827 - SPI replacement to enable J2SE Project extensions to correctly handle Project Properties dialog
SPI replacement to enable J2SE Project extensions to correctly handle Project...
Status: RESOLVED FIXED
Product: java
Classification: Unclassified
Component: Project
7.2
PC Windows 7
: P2 (vote)
: 7.3
Assigned To: apireviews
issues@java
: API, API_REVIEW_FAST
Depends on: 200691 208019 208112
Blocks: 221229 219746
  Show dependency treegraph
 
Reported: 2012-10-10 13:43 UTC by Petr Somol
Modified: 2012-11-11 03:17 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Proposed patch, includes changes in J2SE, WS and FX code, api change, test (48.39 KB, patch)
2012-10-10 13:43 UTC, Petr Somol
Details | Diff
Proposed patch, includes changes in Project UI, J2SE, WS and FX code, api change, test (33.75 KB, patch)
2012-11-01 15:46 UTC, Petr Somol
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Petr Somol 2012-10-10 13:43:23 UTC
Created attachment 125694 [details]
Proposed patch, includes changes in J2SE, WS and FX code, api change, test

A year ago we faced the problem of project properties' saving in non-default panels of Project Properties dialog added by J2SE project extensions (WS, FX). The solution accepted then (see issue #200691) allowed each extension to reqister a service (J2SECustomPropertySaver) that would be invoked when OK is pressed in Project Properties dialog. The solution is principally correct but in its original form now appears insufficient. A problem appeared first in form of issues #208019 and #208112; these have been fixed by a hack that has now been shown to lead to a memory leak: issue #219746.

The situation to be resolved can be summarized as follows: when Project Properties dialog is open, the J2SE Project extensions need a temporary storage of additional properties whose lifecycle should be bound to the lifecycle of the dialog. However, there is no mechanism for that in the dialog itself which is handled by J2SE project code. Each extension can provide various panels, which may or may not be constructed depending on whether the user opens them or not. Thus the reference to extension's specific properties (if they are constructed) is to be held statically. Such solution is prone to memory leaks though as shown in issue #219746 and should be handled properly.
In case the user pressed OK the properties in temporary storage (if it has been created when user opened extension's specific panels) need to be saved. But regardless whether OK or Cancel has been pressed, any temporary storage needs to be eventually erased when the dialog closes.

The proposed solution is to replace the original SPI

public interface J2SECustomPropertySaver {
   void save(Project p);
}

by 

public interface by J2SECustomPropertyHandler {
   void save(Project p);
   void cleanup(Project p);
}

and modify CustomizerProviderImpl in java.j2seproject module so that it invokes J2SECustomPropertyHandler.save() for each registered project extension on OK press, as has been the case till now with J2SECustomPropertySaver.save(), but also to invoke J2SECustomPropertyHandler.cleanup() for each registered project extension on dialog close.

This proposed change will have localized impact and is unlikely to affect any third party code because J2SEProject extension is not a publicly promoted practice. To my knowledge the only two existing extensions are our own javawebstart and javafx2.project modules. The old SPI will be marked deprecated but will remain operational. The change will streamline property handling lifecycle in Project Properties dialog, will prevent memory leaks and will enable to remove some ugly pieces of code.
Comment 1 David Konecny 2012-10-14 21:39:21 UTC
Would it make sense to define this API in Project UI infrastructure so that it can be used by other project types apart from J2SE?
Comment 2 Vladimir Voskresensky 2012-10-14 23:15:21 UTC
(In reply to comment #1)
> Would it make sense to define this API in Project UI infrastructure so that it
> can be used by other project types apart from J2SE?
Yes. It would be great if C/C++ can reuse it as well.
Comment 3 Tomas Zezula 2012-10-16 09:03:38 UTC
If the generic contract is needed I would suggest small change.
The problem with the J2SECustomPropertyHandler and the J2SECustomPropertySaver is that they were designed just for J2SE project. The J2SE Project CustomizerProvider is responsible to call them in its store listener. If we make them generic there will be still a need for the CustomizerProvider implementors to call the interface in the store listener, something like:

private class StoreListener implements ActionListener {
    
        public void actionPerformed(ActionEvent e) {
            uiProperties.save();
            for (J2SECustomPropertySaver saver : project.getLookup().lookupAll(J2SECustomPropertySaver.class)) {
                saver.save(project);
            }
        }
    }

This requires changes in all the affected project types which may even not know about panels which are injected into them. I suggest to move the responsibility to the side of the panel provider.

Currently the ProjectCustomizer.Category has setStoreListener and setOKButtonListener methods which are called by the project UI infrastructure (no action needed on the project implementor side). My suggestion is to extend the ProjectCustomizer.Category by setCloseListener(AL) which will be used to do the clean up.

The provider of the extension panel will just do the following in the implementation of  the ProjectCustomizer.CompositeCategoryProvider.createCategory() method:

ProjectCustomizer.Category c = ProjectCustomizer.Category.create(CAT_WEBSTART,
                    NbBundle.getMessage(JWSCompositeCategoryProvider.class, "LBL_Category_WebStart"), null); 
c.setStoreListener(new ActionListener() {
    @Override
    public void actionPerformed(ActionEvent e) {
         store();
    }
});
c.setCloseListener(new ActionListener() {
    @Override
    public void actionPerformed(ActionEvent e) {
         free();
    }
});
return c;
Comment 4 Petr Somol 2012-10-17 08:40:08 UTC
Comment #3 sounds good. I'll react in more detail after the return from vacation.
Comment 5 Petr Somol 2012-10-25 17:58:19 UTC
I tried to implement the suggestion from Comment #3 but hit a serious problem. Close listeners need to be called when the whole properties dialog gets closed; this however happens immediately after the invocations of Store listeners in org.netbeans.modules.project.uiapi.CustomizerDialog.OptionListener.actionPerformed where each store listener is called through RequestProcessor. This is a sync problem, Close listener can effectively destroy data before actually the RequestProcessor thread gets to it (I observed this).
Combining the proposed Close listener mechanism with the J2SECustomPropertySaver does not help either - dialog panels apparently get closed (and Close listeners in org.netbeans.modules.project.uiapi.CustomizerDialog called) before j2seproject...CustomizerProviderImpl calls J2SECustomPropertySaver.save().
This does not seem to be a problem in case of the original proposal in this Issue, as it is restricted to use with j2seproject only, where store listeners are not executed in separate threads. Will try to investigate more.
Comment 6 Petr Somol 2012-11-01 15:46:38 UTC
Created attachment 126926 [details]
Proposed patch, includes changes in Project UI, J2SE, WS and FX code, api change, test

I am attaching a different patch proposal that follows the idea from Comment #3, though due to problems described in Comment #4 its application is less straightforward than expected. Nevertheless it makes the use of J2SECustomPropertySaver services obsolete, what is an advantage in itself.

The patch introduces a Close listener to ProjectCustomizer.Category which is called on window close. The usage in WebStart and FX Project dialogs is as follows. Assuming OKButton listeners in ProjectCustomizer.Category are called before Store and Close listeners, and assuming there is no guarantee regarding call order of Store and Close listeners (Store listeners are called asynchronously), the proposed use case is to grab a temporary reference to WS or FX specific properties class in OKButton listener, to do cleanup of all other references to the property class using the Close listener, and to do the actual storage calls in Store listener which also eventually removes the temporary reference.
Comment 7 Petr Somol 2012-11-08 10:27:33 UTC
if there is no more comments I am going to integrate tomorrow.
Comment 8 Petr Somol 2012-11-10 20:10:20 UTC
fixed in jetmain
http://hg.netbeans.org/jet-main/rev/569ba1b5a7c2

Note that the changeset also includes changes in WebStart and FX Project modules (JFXProjectProperties and JWSProjectProperties lifecycle and persistence mechanism is changed considerably) in order to fix the memory leak filed as Issue #219746.
Comment 9 Quality Engineering 2012-11-11 03:17:06 UTC
Integrated into 'main-golden', will be available in build *201211110001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/569ba1b5a7c2
User: Petr Somol <psomol@netbeans.org>
Log: #219827 add Close listener to ProjectCustomizer.Category


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo