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

[dmitry markov]

Friendly reminder

Thanks,
Dmitry
On 27/08/2014 14:44, 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.
>
> 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
>>>>
>>>
>>
>