Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various file and exception handling changes #34

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

chpoit
Copy link
Contributor

@chpoit chpoit commented Dec 3, 2018

This PR contains the following:

  • Added a checkbox to allow for the reuse of the chapter files after an epub is generated.
  • Moved the chapters to be in a subfolder for the novel (example: Overgeared/og-chapter-1.xhtml)
  • Added a big try/finally around the button_press code to allow the user to regenerate the epub if an unhandled error occurs
  • Moved the file_path generation to a small util function

Pretty cool stuff you made, hope you like my improvements

@@ -117,9 +131,16 @@ def button_press():
ending_chapter_chosen = ttk.Entry(app, width = 5, textvariable = ending_chapter)
ending_chapter_chosen.grid(column = 1, row = 3, sticky = "W")

#Code for delete chapters
label5 = ttk.Label(app, text = "Cleanup when done: ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly confusing for the user (Cleanup what?).

I'd recommend to change the wording to "Delete temporary chapter files after downlaod"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree, the message made sense to me while writing, but wont make sense to the users.
I'm a pleb with TkInter, so I have no idea how to make the text wrap as to not make too large of a window.

Here is the current result:
image

#Code for delete chapters
label5 = ttk.Label(app, text = "Cleanup when done: ")
label5.grid(column = 0, row = 4, pady = 10, sticky = "W")
delete_chapters = tk.BooleanVar()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the default value of delete_chapters to True, so the default behaviour still deletes the temporary files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of having all the chapters deleted by default, but it is indeed a breaking change, so I added a config.json file with a boolean that is read when teh program launches so the user can configure it at will.

Main reason I'm not a fan of the deletion by default is because 1, it stresses wuxia servers if the user created a book from chapter 10 to 879 has has to restart to have 1-879, and 2, because it can take a while to get all the chapters. I think it took 20 minutes for me to get all the currently released Overgeared chapters.

This ties into the fact that I think it could be cool to have the option to split by "volume" , which could let the user say to split into parts of 300 chapters each. Allowing them to keep the temp chapters would let them "complete" a volume once more chapters come out.

Of course, by keeping a cache of chapter you lose the chance of getting a "revised" chapter with a typo correction and whatnot, but such revisions are probably so infrequent people wont care.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think I never downloaded a chapter twice except for testing. I just assumed that this would be only practical for a few people and clutter up the harddrive for the rest.

But the config file should be the most optimal solution.

@MichaelGuardian
Copy link
Contributor

Thanks for the contribution!
Really like the improvements and looking forward to merge them. I've added two comments for suggested improvements on some small things.

If you disagree with one of the suggestions feel free to comment as there are based on personal preference.

After these two comments are sorted out I'll merge the changes :)

@MichaelGuardian MichaelGuardian merged commit 8c589fc into MakeYourLifeEasier:master Dec 3, 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