[rfc][icedtea-web][policyeditor] Modal PolicyEditor

[Andrew Azores]

On 03/25/2014 03:07 PM, Jiri Vanek wrote:
>
>>>> - }
>>>> - });
>>>> + public static class PolicyEditorFrame extends JFrame {
>>>> + public final PolicyEditor editor;
>>>> +
>>>> + public PolicyEditorFrame(final PolicyEditor editor) {
>>>
>>> I doubt this will be ever used - please do it private.
>>
>> Private, but return the specialized PolicyEditorFrame type anyway 
>> from the public getters? Seems a bit odd. I'm not just returning 
>> JFrame/JDialog because then there's no easy way to get the actual 
>> PolicyEditor instance out of it, to do things like programmatically 
>> add codebases. I did have to give the PolicyEditor a reference to its 
>> parent frame/dialog in this revision, but I'd rather not use this as 
>> a way to hold the reference to the frame/dialog and have eg the 
>> security dialogs access the frame/dialog through the editor's 
>> reference. That just seems upside down and backward.
>
>
> I must insists. Please do both "do the actuall work" constructors 
> (public PolicyEditorFrame(final PolicyEditor editor) and   public 
> PolicyEditorDialog(final PolicyEditor editor) )private.

Okay, so then if they're private, CertWarningPane and 
PartiallySignedAppTrustWarningPanel can't use them, and will be stuck 
with only JDialog? Then how do they get to the actual PolicyEditor 
instance to do addNewCodebase?

>
>>
>>>> + super();
>>>> + this.editor = editor;
>>>> + add(editor);
>>>> + this.pack();
>>>> + editor.setVisible(true);
>>>> + this.setJMenuBar(createMenuBar(this, editor));
>>>> +
>>>> + setTitle(R("PETitle"));
>>>> +
>>>> + setDefaultCloseOperation(WindowConstants.DISPOSE_ON_CLOSE);
>>>> +
>>>> + addWindowListener(new WindowAdapter() {
>>>> + @Override
>>>> + public void windowClosing(final WindowEvent e) {
>>>> + editor.quit();
>>>> + dispose();
>>>> + }
>>>> + });
>>>> +
>>>> + editor.closeButton.addActionListener(new ActionListener() {
>>>> + @Override
>>>> + public void actionPerformed(final ActionEvent e) {
>>>> + dispose();
>>>> + }
>>>> + });
>>>> + }
>>>> + }
>>>> +
>>>> + public static PolicyEditorFrame getPolicyEditorFrame(final String 
>>>> filepath) {
>>>> + return new PolicyEditorFrame(new PolicyEditor(filepath));
>>>> + }
>>>> +
>>>
>>> Tehre is really a lot of shared code in this two "methods".
>>>
>>> PolicyEditorFrame and PolicyEditorDialog.
>>>
>>> They both have general ancestor -Window. All the operations behind 
>>> super() call may be done in generalised ancestor.
>>
>> Window doesn't have setTitle, or setJMenuBar, or 
>> setDefaultCloseOperation...
> hmm.. I was found with patns down it seams.... The java.awt.Window 
> really do not have it?? Strange...
> Anyway - there is really to much duplicate code. Please
>
>
> nvm - the duplicate code is really awwefull:
>
> +           this.editor = editor;
>    private artificial forefather?
> +            add(editor);
> +            this.pack();
>
> window
>
>   editor.setVisible(true);
> +
> +            setTitle(R("PETitle"));
> +
>
> this is stupid :)
>
> + setDefaultCloseOperation(WindowConstants.DISPOSE_ON_CLOSE);
> +
> this too:-/

Sorry but none of the above five comments are useful. I don't know what 
you want.

>
> Maybe add interfcae for this? :D feel free to elaborate as you wish.

What? An interface for what? I don't understand how adding an interface 
is going to help.

>
> +            addWindowListener(new WindowAdapter() {
> +                @Override
> +                public void windowClosing(final WindowEvent e) {
> +                    editor.quit();
> +                    dispose();
> +                }
> +            });
> +
> +            editor.closeButton.addActionListener(new ActionListener() {
> +                @Override
> +                public void actionPerformed(final ActionEvent e) {
> +                    dispose();
> +                }
> +            });
>
> thso eshould be in Window or comon artifical forefather  too.
>
>

I still don't know how I'm supposed to be subclassing Window here. It is 
missing required functionality that is implemented independently by both 
JFrame and JDialog. Yes, if I made a Window subclass, then most of the 
logic could be moved into it just fine, but some important things eg 
window title and menu bar would be missing. Then if I subclass this new 
Window subclass, I still don't have the JMenuBar, etc. I don't know what 
"artificial forefather" you're suggesting otherwise.

Anyway, the attached patch slightly reduces the amount of duplicated 
code by extracting it into a private static helper method that operates 
on Window. The only work now done in the JDialog and JFrame subclassed 
constructors are the parts that cannot be done on a Window, which 
happens to be the exact same between the two of them.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: modal-policyeditor-4.patch
Type: text/x-patch
Size: 22131 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140325/666e54a4/modal-policyeditor-4-0001.patch>