bandantonio / obsidian-apple-books-highlights-plugin

Import highlights and notes from your Apple Books to Obsidian
https://obsidian.md/plugins?id=apple-books-import-highlights
MIT License
53 stars 1 forks source link

fix: remove new lines from the end of contextual text blocks #28

Closed absorpheus closed 4 months ago

absorpheus commented 4 months ago

Summary

Fixes #30

This removes newline characters which may exist at the end of contextualText text blocks which are creating unnecessary new lines in Obsidian's editing mode.

This screenshot illustrates the bug in contextualText text blocks using the default template:

contextualText-newline-characters

This is after the fix is applied:

contextualText-after-fix
bandantonio commented 4 months ago

Thank you. I was keeping this issue in mind for a while, and your PR is a good starting point. In my highlights, I also found the following cases (depends on a book):

So I thought about either making this helper more universal, or merging it (if I/we could find an elegant way) with the existing one that works with indentation: src/methods/aggregateDetails.ts#L51. That's why the issue is still here 😄

absorpheus commented 4 months ago

Thank you for sharing, I wasn't aware that other cases existed! I'm thinking it might be better to keep these helper functions modular so we can easily test them, however it may become cumbersome to call multiple functions nested within each other.

We could have a 'parent' helper function that takes in an array of helper functions and the text as parameters and loops through the helper functions array, invoking the helper function with the outputted result from previous returned text each time.

I'll have a think about it. It would be helpful if we could write some tests so we can better understand the edge cases we are attempting to solve.

absorpheus commented 4 months ago
  • Trailing spaces - along the way.
  • Newlines and tabs

    • reboot?\n\n
    • project.\n\n\n
    • simple answers.\n\t\t
    • success.\n\t\t\t
    • instead.\n\t\t\t\t\n\t\t\t\t\n\t\t\t\t\t
    • a book\n\t\t\t\t\t\t\n\t\t\t\t\t\n\t\t\t\t\t\n\t\t\t\t\t\t\n\t\t\t\t\t\t\t

Are these trailing spaces, newlines and tabs appearing only at the end of the highlighted text block (contextualText) or do they appear in other places too? e.g. in between highlighted paragraphs

bandantonio commented 4 months ago

Thank you for sharing, I wasn't aware that other cases existed!

No blame at all! I should have created an issue for that.

UPD: Created #30 Let's continue this discussion there and place this PR on hold for some time.

I'm thinking it might be better to keep these helper functions modular so we can easily test them, however it may become cumbersome to call multiple functions nested within each other.

Yes, that's how I thought initially. Modularity is a good approach both from coding and testing perspective, but eventually makes the code base a bit noisy since you end up having several helper functions doing relatively similar actions. Plus, wrapping text fragments into more than two helpers may worsen code readability.

We could have a 'parent' helper function that takes in an array of helper functions and the text as parameters and loops through the helper functions array, invoking the helper function with the outputted result from previous returned text each time.

Yeah, that's one of the possible ways.

I'll have a think about it. It would be helpful if we could write some tests so we can better understand the edge cases we are attempting to solve.

Yes, TDD definitely makes sense.

absorpheus commented 4 months ago

@bandantonio I've rebased this branch with master and squashed the commits down to one commit.

I force-pushed the branch so the branch's commit history on GitHub is consistent with my local branch's commit history. I think you'll need to fetch this branch again as it's history has been re-written and will be different to your local branch.

Apologies for the inconvenience.

Please let me know if these changes resolve all the suggestions you made.

Thanks