<Swing Dev> [9] Review request for 8048110: Using tables in JTextPane leads to infinite loop in FlowLayout.layoutRow

[dmitry markov]

Hi Alexandr,

I have added updateAfterChange() method as you suggested. New version is 
located at http://cr.openjdk.java.net/~dmarkov/8048110/jdk9/webrev.03/

Thanks,
Dmitry
On 08/09/2014 14:44, Alexander Scherbatiy wrote:
> On 8/27/2014 2:44 PM, dmitry markov wrote:
>> Hello Alexandr,
>>
>> Thank you for the review. I have updated the fix according to your 
>> comments. Please find new version here - 
>> http://cr.openjdk.java.net/~dmarkov/8048110/jdk9/webrev.02/
>> Now FlowView.forwardUpdate() invokes super.forwardUpdate() to update 
>> the view responsible for the changed element and then sends update 
>> event to the GlyphViews followed by the changed place. It updates the 
>> GlyphViews regardless the type of the first view. Also new views are 
>> not updated twice anymore.
>
>    The FlowView checks directly the type of a view in the 
> forwardUpdate() method. May be it is better to add method 
> updateAfterChange() with package access to View which does nothing by 
> default and update elements for  GlyphView?
>
>   Thanks,
>   Alexandr.
>
>>
>> Thanks,
>> Dmitry
>> On 26/08/2014 14:02, Alexander Scherbatiy wrote:
>>>
>>>   - The fix sends an update event to all views followed by the 
>>> changed place only if the first updated view is GlyphView.
>>>     Should the GlyphViews be updated if the first view is not the 
>>> GlyphView?
>>>
>>>   - The javadoc for forwardUpdate() methods says: "If there were 
>>> changes to the element this view is responsible for, that should be 
>>> considered when forwarding (i.e. new child views should not get 
>>> notified)."
>>>     Does it mean that new views will be updated twice first in the 
>>> FlowView.forwardUpdate()  method and second during initialization?
>>>
>>>   Thanks,
>>>   Alexandr.
>>>
>>> On 8/21/2014 1:03 PM, dmitry markov wrote:
>>>> Hello,
>>>>
>>>> Reminder. Could you review this, please?
>>>>
>>>> Thanks,
>>>> Dmitry
>>>> On 18/08/2014 12:10, dmitry markov wrote:
>>>>> Hello Sergey,
>>>>>
>>>>> LogicalView.forwardUpdate() sends update event to all views 
>>>>> followed by the changed place. This event will cause view to drop 
>>>>> the cache and re-calculate its break points. This method was added 
>>>>> under JDK-8024395 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8024395> and intended 
>>>>> for the documents contained only text elements. The current 
>>>>> implementation does not expect that non-text element, (e.g. a 
>>>>> table) can be added to the document.
>>>>> If the table is added to the document with flow layout, the 
>>>>> invocation of LogicalView.forwardUpdate() will produce some 
>>>>> negative visual effects (table will be displayed several times). 
>>>>> In case of replacing the text by the table the situation much 
>>>>> worse, since the FlowView will try to re-layout the removed text 
>>>>> and hangs.
>>>>>
>>>>> We have to send the update events to all instances of GlyphView 
>>>>> followed by changed place for proper text line breaks calculation, 
>>>>> see JDK-8024395 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8024395>, JDK-8014863 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8014863>. That's why I 
>>>>> added checks for GlyphView instance. I understand this might be 
>>>>> not a good place, but only here we have information about all 
>>>>> views in the document and can forward update to them, if any.
>>>>> Please note another approach to solve the problems with line 
>>>>> breaks calculation - re-calculate break spots any time when a view 
>>>>> is laid out. As far as I know it is used in jdk6, but this 
>>>>> solution is not suitable, since it causes serious performance 
>>>>> problems in case of large documents.
>>>>>
>>>>> Also I have updated the fix. New version - 
>>>>> http://cr.openjdk.java.net/~dmarkov/8048110/jdk9/webrev.01/
>>>>> Now LogicalView.forwardUpdate() sends update only to GlyphView 
>>>>> followed by the changed place. This resolves the problem when the 
>>>>> table is located in the middle of the text line.
>>>>>
>>>>> Thanks,
>>>>> Dmitry
>>>>>
>>>>> On 15/08/2014 12:56, Sergey Bylokhov wrote:
>>>>>> Hello, Dmitry.
>>>>>>>> The LogicalView.forwardUpdate() should send an update event to 
>>>>>>>> the views
>>>>>>>> followed by the changed place, only when an instance of 
>>>>>>>> GlyphView has
>>>>>>>> changed; otherwise it should invoke View.forwardUpdate() to 
>>>>>>>> handle the
>>>>>>>> update properly.
>>>>>> Can you share additional information about the fix. Why 
>>>>>> LogicalView should send an update if GlyphView was changed only. 
>>>>>> Because before the fix LocalView knows nothing about GlyphView.
>>>>>>
>>>>>> Thanks,
>>>>>> Dmitry
>>>>>
>>>>
>>>
>>
>