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

[Jiri Vanek]

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.
>>
>>