relayout text for linked text frames unnecessary
Fahad-Alsaidi opened this issue · 11 comments
this follow up #136, now we need to relayout the frame only if it is required. Right now if you have 100 linked frame and you add a char in the 99th frame, scribus will relayout all 98 frame unnecessary.
Even in single frame, you should at worst relayout only from the current paragraph downwards (when HarfBuzz implements harfbuzz/harfbuzz#224, you will also be able to relayout just the affected parts of the paragraph).
@khaledhosny sure but this will required to refactor layout() which is thing I rather not doing it.
However, now by b383d7b, scribus will relayout only edited frames making it faster to edit long chained frames. This will hopefully fix 1036.
@aoloe & @MrB74 please test.
i've added a comment in the patch...
layoutAll() does not exist in trunk so 1036 is a bit separate from this. CTL is faster though but now the loop just does nothing.
Let's just be careful not to break existing and working layout code, including Andrea's layout caching code. We do want to merge CTL without more issues that need finding and fixing.
@MrB74 sure, we can always revert to last stable point prior to merging time.
fahad, as far as i can tell, layoutAll() makes sure that all affected frames are re-layouted.
as i wrote on irc, (and always AFAICT!) it seems to do the layout with a complexity of O(n!), while it should probably be O(n).
by completely removing layoutAll() you're indeed achieving a O(n) or O(1) complexity (which is a huge speedup against O(n!)!), but the code does not do have the same functionality anymore.
(on top of it, you should remove the while loop, since it now does nothing. i guess that you know got what i meant with my side note : - )
personally, i don't understand yet what the code exactly does, but i can for sure try to grok it... but not before friday (i guess it will take me a few hours... i have no idea of this part of the scribus code).
i'm sure that other people are more efficient at this task...
anyway, my first step would be to document the current layoutAll() and the beginning of layout()...
if layoutAll() is indeed not needed, this further simplification should be ok...
further comments:
This should be fixed by now in trunk.