[rfc][icedtea-web] Security dialogs wait for PolicyEditor to finish parsing

[Jiri Vanek]

On 03/26/2014 05:04 PM, Andrew Azores wrote:
> On 03/26/2014 07:37 AM, Jiri Vanek wrote:
>> On 03/25/2014 09:36 PM, Andrew Azores wrote:
>>> On 03/24/2014 12:01 PM, Jiri Vanek wrote:
>>>> On 03/24/2014 04:46 PM, Andrew Azores wrote:
>>>>> On 03/24/2014 11:11 AM, Jiri Vanek wrote:
>>>>>> On 03/24/2014 03:44 PM, Andrew Azores wrote:
>>>>>>> On 03/24/2014 10:40 AM, Jiri Vanek wrote:
>>>>>>>> On 03/24/2014 03:01 PM, Andrew Azores wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> PolicyEditor loads and saves policy files async, obviously. However, this means that opening
>>>>>>>>> a new
>>>>>>>>> PolicyEditor instance on an existing policy file and programmatically adding a new codebase
>>>>>>>>> to the
>>>>>>>>> editor does not have a well-defined order. This can be problematic because PolicyEditor
>>>>>>>>> highlights/selects the last added codebase, so eg in the scenario where the editor is being
>>>>>>>>> launched
>>>>>>>>> from a security dialog, we want to be sure that the current applet's codebase is the one
>>>>>>>>> selected
>>>>>>>>> when the editor appears. This patch adds a method to PolicyEditor that indicates whether the
>>>>>>>>> editor
>>>>>>>>> is currently reading/writing a file, and adds a loop to the two security dialogs so that
>>>>>>>>> the new
>>>>>>>>> codebase is not added and the editor made visible until it completes its IO.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hm... I dont think this is correct approach. Why are you so strongly against modal dialogue?
>>>>>>>>
>>>>>>>>
>>>>>>>> J.
>>>>>>>
>>>>>>> I'm not... making PolicyEditor modal will come later. This patch is just about making sure that
>>>>>>> when
>>>>>>> you launch PolicyEditor from a security dialog, the current applet's codebase is guaranteed
>>>>>>> to be
>>>>>>> the selected one when the editor appears. This and modality are completely separate.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>> uff... wouldnt be much more easy to make the load/save in sync?  I do not see any reason why
>>>>>> it is
>>>>>> in new thread Thread(save/load).start() ....
>>>>>>
>>>>>>
>>>>>> J.
>>>>>
>>>>> I really don't want to be doing it sync on the UI thread. This makes the application less
>>>>> responsive, and is generally bad practice. Not worth the benefit of making it easier to guarantee
>>>>> the programmatically added new codebases come after. There are better ways to do it async than
>>>>> manually spawning a new Thread, though, yes. eg SwingWorker as you said on IRC, or Executors and
>>>>> Runnables as I suggested. Still, these potential enhancements to the multithreading model would
>>>>> still have the codebase ordering problem.
>>>>
>>>> I agree thath do it in gui thread is wrong. But I disagree with current aproach and new suggested
>>>> locking.
>>>>
>>>> Correct aporach is:
>>>> invoke thread (swingworker)
>>>>   disable gui or show dialog with progressbar or soem wait please
>>>>    (so gui will be "responsoble")
>>>> wait for thread.
>>>>
>>>>
>>>> Would be correct approach.
>>>
>>> Okay, this is an enhancement in that it uses the nice SwingWorker abstraction and provides a visual
>>> indicator of progress for the IO operation, but I don't see how it really solves the problem. If by
>>> "wait for thread" you mean Thread#join or similar, then we're again just blocking the EDT, so it
>>> won't actually be responsive until the IO is done. In that case we might as well just have the EDT
>>> do the IO directly. If we don't block the EDT and continue to let the IO complete async, albeit with
>>> visual progress indication and nicely abstracted, then there is still the problem of addNewCodebase
>>> being called externally and not having a well-defined order compared to the addNewCodebase calls
>>> made by the thread performing the IO.
>>
>>
>> Agree. Then please do this now:
>>
>> ** failed to import extension forest from /home/jvanek/hg-forest-ext/forest.py: No module named repo
>> diff -r 4c584d98c42d netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> --- a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java Wed Mar 26 12:21:48 2014
>> +0100
>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java Wed Mar 26 12:37:11 2014
>> +0100
>> @@ -1067,7 +1067,7 @@
>>                      OutputController.getLogger().log(e);
>>                  }
>>              }
>> -        }.start();
>> +        }.run();
>>      }
>>
>>      /**
>> @@ -1170,7 +1170,7 @@
>>                      showCouldNotSaveDialog();
>>                  }
>>              }
>> -        }.start();
>> +        }.run();
>>      }
>>
>>      /**
>>
>>
>>>
>>>>
>> because :
>>>>
>>>> However - the polici files *are* small. So the burden of *doing it IN*  AWT is imho acceptable.
>>>>
>>>>
>>
>
> Good to go?

Yes:)