ClementGre / PDF4Teachers

PDF editing software in large quantities. Designed for teachers, this app keeps recorded previous annotations, and offers features like marking scale, PDF conversion, vectorial drawing...
https://pdf4teachers.org/
Apache License 2.0
144 stars 18 forks source link

refactor(TextWrapper): simplify and optimize text wrapping logic #164

Closed arturhg closed 3 months ago

arturhg commented 1 year ago
Streamline the text wrapping logic in the TextWrapper class by:
    - Combining fillLineWithWord and fillLineWithChar methods into a single fillLine method with a 'splitByWords' parameter, removing duplicate code for handling word and character wrapping
    - Replacing the usage of ScratchText with Text from JavaFX to measure the text width
    - Using StringBuilder for better performance when building wrapped lines and appending lines, instead of concatenating strings
    - Replacing the custom test method with the more descriptive exceedsMaxWidth method for determining if a line's width exceeds the maximum allowed
Update the TextElement class to use the new hasWrapped() method in TextWrapper:
    - Replace the previous call to doHasWrapped() with hasWrapped() to reflect the updated method name
Remove unused imports from TextWrapper:
    - Remove the import of ScratchText, as it is no longer used in the class
    - Remove the import of Pattern, as the regular expression functionality is no longer required in the updated implementation
Update the wrapFirstLine and wrap methods in TextWrapper to use the new wrapTextLineByWordsOrChars method, which consolidates the logic for wrapping text by words or characters based on whether the first word exceeds the maximum width
arturhg commented 1 year ago

Hi, @ClementGre. Can this PR be merged?

ClementGre commented 1 year ago

I'm just unsure whether switching from ScratchText to Text for checking text width would cause issues. As I said, the Text class has styling attributes that prevent setting a custom font to it. Have you noticed any difference in behaviour?

ClementGre commented 6 months ago

Can you change back Text class to ScratchText? Then this PR will be ready to be merged.

arturhg commented 3 months ago

Hi @ClementGre.

I changed back Text class to ScratchText.

Thank you!

arturhg commented 3 months ago

@ClementGre

I resolved the merge conflict.

ClementGre commented 3 months ago

@arturhg thank you, your code works well!