-
Notifications
You must be signed in to change notification settings - Fork 387
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
Html export #104
Html export #104
Conversation
This is absolutely fantastic, thank you so much @wjs018. Everything seems to be up to the project's standards after a full review, I really appreciate your effort in that regard. This will make updating the documentation & licenses really easy for me. I left a few comments in the code review, but they are very minor so I will accept this as-is and work on those separately. Thank you again for your efforts with this, much appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follows project's standards quite well, clean implementation. Just a few minor comments, but I can address those pending your responses.
Thank you for both tackling this feature, and taking so much care with your implementation (I am very pleased with the quality of the code in this pull request!).
@@ -136,6 +140,96 @@ def write_scene_list(output_csv_file, scene_list, cut_list=None): | |||
'%d' % duration.get_frames(), duration.get_timecode(), '%.3f' % duration.get_seconds()]) | |||
|
|||
|
|||
def write_scene_list_html(output_html_filename, scene_list, cut_list=None, css=None, css_class='mytable', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to have this function in it's own .py file, just because it has nothing to do with the SceneManager class itself. That being said, this is something I can tackle myself - just a small comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I put it there because this is also where the write_scene_list
function is that writes the csv file. I basically tried to make these two functions the same. One minor thing is that the write_scene_list
function takes a file handle, but the html version just takes a filepath. This is because the simpletable
code does the opening of the file and I thought it easier to have this discrepancy than rewriting the simpletable
code at the time. If these two functions get consolidated, it might make sense to standardize the file handling as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points @wjs018, I'll make a note to see if I can modify write_scene_list to also just take the output file path as a string, instead of a file handle, to make it consistent. Also good point in that write_scene_list and write_scene_list_html belong together - perhaps I will move them into a new sub-module (e.g. export.py
or scene_export.py
- I'm not sure which is better, or if there's a better name - feel free to choose or suggest one!).
As discussed in #17 this allows for exporting the scene list and optionally include the images from the
save-images
command in the html output.