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

[Jiri Vanek]

I think we misunderstood a bit.

Attached is an patch doing what I wonted.

It is defining interface (I wish it could be an class! :) ) common to both  PolicyEditorFrame and 
PolicyEditorDialog.

It become quite verbose, but its cleaner.


It is fixing one mayor bug:
You had     public static class PolicyEditorFrame extends JDialog {
So I fixed it to    public static class PolicyEditorFrame extends JFrame {

And making the two constructors I wont private.
I really don't know where we miscommunicated:))

Also it is removing unnecessary    private final Window parentWindow by fixing the signature of 
methods which were using from incorrect window, to correct component.


Otherwise it is same. If  you can survive my Interface, You can push.
If not (and I agree it is maybe nuclear shot to kill a pigeon.. really maybe to much verbose) then 
please one last round. But keep your version :
  - with two private constructors
  - without aprent variable
  - and without duplicate code.


Sorry for my probably wrong explanations during reviews :(


J.


On 03/25/2014 09:05 PM, Andrew Azores wrote:
> 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: modalPoliciEditor.patch
Type: text/x-patch
Size: 22320 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140326/dc64d583/modalPoliciEditor-0001.patch>