wustho / epy

CLI Ebook (epub2, epub3, fb2, mobi) Reader
GNU General Public License v3.0
981 stars 53 forks source link

Add user setting for indenting paragraphs #63

Closed 3N4N closed 2 years ago

3N4N commented 2 years ago

I sent a patch of this commit as an email. @wustho didn't respond. So I decided to fork and open a PR.

There is probably a good reason it hasn't been incorporated yet, but as a novel reader I am fond of the text-indentation at the start of paragraphs. This patch adds support for a user setting, ParaIndent (you may wanna rename it so it's worthy of sharing codespace with your other well-named variables), which is an int representing how many characters a paragraph should indent. With this option set to zero, which is the default value, epy should behave as before (i.e., as if this PR hasn't been applied).

wustho commented 2 years ago

Hey there @3N4N , thanks for this. Yes I was wondering where your patch is, I thought you posted in issue or PRs but didn't find it until you mention you only sent it via email. I will check this first.

3N4N commented 2 years ago

I will check this first.

I think there has been a misunderstanding. The patch I sent through email is superseded by this PR. They contain the same commit. So you don't have to worry about the email patch. I mentioned it just for disclosure and reference.

P.S. I have another, similar patch. I'll be waiting for your review on this PR so I can understand how you want your code to be designed (where to define parameters and where not to, etc.) before opening a new PR.

wustho commented 2 years ago

Woww just, checked, it's amazing man, thanks @3N4N merging this...

I remembered I really want this, and I thought it was also cooler if we can get justified text, but getting left-right justified is impossible with current framework. I'm currently looking at https://github.com/Textualize/textual and see if it can modernize this epy.

Thanks again

3N4N commented 2 years ago

Thanks for the merge. And I look forward to a more modernized interface. Although . . . I can't see how it can do justified alignment. The reason epy's current framework can't do it is because of the monospace fonts, right? We need proportional fonts. And terminals don't support proportional fonts . . . But I have yet to see Textualize/textual; looks cool.

wustho commented 2 years ago

Hey, there @3N4N , there is an issue, tho, with the styling. I just reverted it.

So the issue is, seems like it move the styling (italic, bold) by paraindent variable, it was so subtle (especially italic). eg. It is supposed to be "Who are you?" but if I set the paraindent=4 it become "Who are you?".

Sorry for this, this epy built on top of primitive ncurses with no external TUI framework, so there is a lot of hardcode done.

wustho commented 2 years ago

I'll be happy to look into it again, if you're able to make some patch btw.

3N4N commented 2 years ago

@wustho I mean, a patch for this exact problem is simple.

diff --git a/epy.py b/epy.py
index ee7362e33bc4..98407a941610 100755
--- a/epy.py
+++ b/epy.py
@@ -1143,11 +1143,11 @@ class HTMLtoLines(HTMLParser):
         # formatting
         elif tag in self.ital:
             if len(self.italic_marks) == 0 or self.italic_marks[-1].is_valid():
-                char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1]))
+                char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1])+self.paraindent)
                 self.italic_marks.append(TextMark(start=char_pos))
         elif tag in self.bold:
             if len(self.bold_marks) == 0 or self.bold_marks[-1].is_valid():
-                char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1]))
+                char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1])+self.paraindent)
                 self.bold_marks.append(TextMark(start=char_pos))
         if self.sects != {""}:
             for i in attrs:
@@ -1204,11 +1204,11 @@ class HTMLtoLines(HTMLParser):
             self.text.append("")
         # formatting
         elif tag in self.ital:
-            char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1]))
+            char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1])+self.paraindent)
             last_mark = self.italic_marks[-1]
             self.italic_marks[-1] = dataclasses.replace(last_mark, end=char_pos)
         elif tag in self.bold:
-            char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1]))
+            char_pos = CharPos(row=len(self.text) - 1, col=len(self.text[-1])+self.paraindent)
             last_mark = self.bold_marks[-1]
             self.bold_marks[-1] = dataclasses.replace(last_mark, end=char_pos)

But there may/must be other hidden bugs. A few days of testing would be great. But I don't really know enough about ebooks to know what to look for. I didn't even notice the italic/bold issue. What should I do?

wustho commented 2 years ago

@3N4N that should work, let me check it. Altho, there still might be issue when searching text tho.

3N4N commented 2 years ago

@wustho I don't have issues searching texts. Would you share exact reproducible steps, like you did with the issue with italic?

And I force pushed the patch above to feat/paraindent.

wustho commented 2 years ago

Ah thanks @3N4N I'll test it first more thoroughly this time...