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

[dmitry markov]

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.

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