rupa / epub

python/curses cli epub reader
380 stars 63 forks source link

Long book title lines wrapping to next line in TOC page #21

Closed Boruch-Baum closed 7 years ago

Boruch-Baum commented 7 years ago

When a book's title length exceeds the window width it wraps to the first line of the chapter listings. This is probably the case also for chapter listings themselves. I noticed this in a case where the first chapter "title" was blank, so there was nothing to over-write the word-wrap of the prior line's title. What then happens is that the next line will partially over-write the wrapped line, so the result will be the correct current line followed by the remainder of the previous line's wrap.

If you want, I can attach the offending epub example file.

Boruch-Baum commented 7 years ago

My current solution is to replace current line 173: screen.addstr(i, 0, '{0:-5} {1}'.format(start, title)) With: screen.clrtoeol() screen.addstr(i, 0, '{0:-5} {1}'.format(start, title.strip()))

Note: This also incorporates my fix from issue #20.

keithstellyes commented 7 years ago

Unfortunately the original author seems to be MIA, given a lack of response to my pull request and the issue I mentioned: #19. I may try e-mailing them/hunting down their social media.

It's a shame he never declared a license, otherwise I would be incorporating these fixes people have...

For many students in the USA, this is finals week, so maybe he's one of the ones affected?

keithstellyes commented 7 years ago

Also, do you mind if I incorporate your fix into my development branch of my fork?

https://github.com/keithstellyes/epub/tree/develop

The master is just the branch pending merge with his branch.

I will of course credit you.

Boruch-Baum commented 7 years ago

I'm not ready to give up on him/her quite so quickly, but sure, you can incorporate the patches. I'm now playing with getting images working, but no guarantee I'll persevere till the job gets done.

As for the licensing issue. My understanding is that no license = public domain, but in github there may be a site-wide default and a user-wide default (in user preferences somewhere?)

BTW, What are the details on your own patch?

rupa commented 7 years ago

don't mean to be MIA but this hasn't been a priority project for a while. Happy to see forks and improvements, and fine with whatever, but I don't have a timetable - I keep wanting to rewrite the core to be less crappo. WTFPL

keithstellyes commented 7 years ago

I'm sure the author will come back soon, I didn't mean to imply I wanted to give up on him that quickly, but I see a lot of folks wanting to contribute, and he's just not responding.

My patch is pretty simple, I just cleaned up controls handling by making controls not hard-coded but instead constants at the top, and I improved his, not-very-good-way of handling the key event, 'Q', where he did:

try:
    if chr(ch) == 'q':
        return
except:
    pass

The segment caught my attention originally because catch-all except blocks are rarely good. That idiom is a common way of "if error, ignore", but I've had to debug problems where that was hiding other problems I needed to fix.

He was doing an except because sometimes that would catch key events with values greater than 255, which when passed to chr(), throws an error. So, he did an except block to not crash it. I opted instead to write it as if ch == ord('c') basically. I didn't exactly since it's not hardcoded, though. That way simplifies the code and gets rid of one more way bugs can hide.

keithstellyes commented 7 years ago

Oh wow, literally as I'm writing that the author returns :))

keithstellyes commented 7 years ago

@rupa Do you mind merging my PR, then doing his patch?