Skip to content

Add configuration file, multiple views, keybindings, and scale modes#21

Closed
Dudemanguy wants to merge 29 commits into
klaxa:masterfrom
Dudemanguy:master
Closed

Add configuration file, multiple views, keybindings, and scale modes#21
Dudemanguy wants to merge 29 commits into
klaxa:masterfrom
Dudemanguy:master

Conversation

@Dudemanguy
Copy link
Copy Markdown
Contributor

This essentially takes care of #13 and #14. ConfigObj is now a new dependency for reading configuration files and setting default settings/keybindings. Double page mode and manga mode have also been implemented as well as best fit, height fit, and width fit. The options page notes everything down. Just a note, the implementation of double page mode and the fitting modes isn't totally perfect. However, the only real "issue" is that rotating the image to the 90 degree or 270 degree (i.e. sideways) in double page mode will only display the first page due to the code that checks the width of images to make sure two can fit on the screen. Perhaps I'll attempt to take care of that edge case later.

@klaxa
Copy link
Copy Markdown
Owner

klaxa commented Apr 1, 2018

Hey, wow nice work! I'm currently busy with other things, but when I find time I will see to review and merge your pull request; hopefully in the next couple of days. Thank you!

@Dudemanguy
Copy link
Copy Markdown
Contributor Author

Sure no problem. I just got bored for a few days.

@Dudemanguy
Copy link
Copy Markdown
Contributor Author

Okay, I just added a few more commits and got height, width, and best fits to work properly with double page mode and rotation. I believe all possible cases should be covered now.

@klaxa
Copy link
Copy Markdown
Owner

klaxa commented Apr 20, 2018

Ok, I looked at the code, changes seem fine. In some places you do:

if condition:
    do_stuff()
elif not condition:
    do_other_stuff()

or

if condition:
    do_stuff()
if not condition:
    do_other_stuff()

where

if condition:
    do_stuff()
else:
    do_other_stuff()

is completely equivalent.

Other than that before merging I'd like to test it. I seem to have missed where the coordinates for cutting out the image for OCR is taking into account double pages and rotations. Maybe that's not even needed and it just works? I might have a chance later tonight (I need my other computer for this.)

@Dudemanguy
Copy link
Copy Markdown
Contributor Author

All of the elif I used were accompanied by an else later down the chain (which perhaps could be rewritten anyway). There's definitely some sloppy if's in there though like you said. OCR works fine on the double page. The code is down there at the bottom. It won't work with the rotation though. The OCR image will just fetch rotated characters which obviously won't fetch anything useful. It wouldn't be hard to add it in though.

@Dudemanguy
Copy link
Copy Markdown
Contributor Author

Sorry for constantly adding more commits to this. I replaced some of the ifs with elses to hopefully make it nicer. I also realized I had a pretty big flaw on the width checking (when pages were bigger than the screen), so I moved that out to a separate function and made it handle those use cases. One thing I noticed is that going back pages in double page mode isn't totally perfect. It's possible for it go back only one page instead of two pages if the current display is only on one. I haven't thought of a way to nicely handle that without adding a ton of boilerplate that I don't think is really worth it.

@Dudemanguy
Copy link
Copy Markdown
Contributor Author

Closed in favor of #22

@Dudemanguy Dudemanguy closed this May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants