Conversation
…odule that can be reused in different contexts
…ls.py module to handle most movie creation logic
fyffep
left a comment
fyffep
left a comment
There was a problem hiding this comment.
I appreciate the refactor. It makes it a much cleaner UX to have the movie generation function in its own window. That said, it is redundant of the old code in the setup/config dialogue. Is the plan to remove that? If we do, it should be a separate PR.
cc3d/player5/Plugins/ViewManagerPlugins/MovieGeneratorDialog.py
Outdated
Show resolved
Hide resolved
| return wrapper | ||
|
|
||
|
|
||
| def open_folder_in_file_browser(path, parent=None) -> bool: |
There was a problem hiding this comment.
Nice! But I still have a nitpick. 🙂 This could be placed in a file called Utilities/components.py to avoid bloating this file.
There was a problem hiding this comment.
This file is not bloated at all -200 lines of code - and because it lists reusable functions, I think this is a good place - for now. I would argue that components.py might actually be somewhat misleading because the content of this file does not really contain "components" in the way most people understandt he workd "component". We can always import a new PR to work on code reorganization, but let's do it in a separate PR, otherwise this PR will never end :)
There was a problem hiding this comment.
Don't worry about it. It has no effect on the functionality.
| def display_movie_result(self, movie_count): | ||
| display_movie_creation_result(movie_count=movie_count, q_label_obj=self.status_LB) | ||
| if movie_count > 0: | ||
| self.show_movies_folder_PB.setEnabled(True) |
There was a problem hiding this comment.
We should still allow this button to be clicked even if no movies have been generated recently. What if you want to check on the movies that you made yesterday?
There was a problem hiding this comment.
The show movies folder button is always on, and now it uses Configuration.getSetting("RecentMoviePath) to open the most recent movie location
|
@fyffep let me know if there is anything else that's needed, or if there is something I overlooked, or if you have new comments. In the case this code passes your acceptance threshold, let's approve it and work on the new features you listed in the new PRs |
…ings to open a most recent movies folder
| folder_with_movies = Path("/").resolve().anchor | ||
| if Path(self.simulation_path).exists() and Path(self.simulation_path).is_dir(): | ||
| folder_with_movies = self.simulation_path |
There was a problem hiding this comment.
I know I commented about this line before -- sorry for the back-and-forth. Do you think there's value in falling back to the simulation output directory like in choose_movie_directory?
| return wrapper | ||
|
|
||
|
|
||
| def open_folder_in_file_browser(path, parent=None) -> bool: |
There was a problem hiding this comment.
Don't worry about it. It has no effect on the functionality.
| def display_movie_result(self, movie_count): | ||
| display_movie_creation_result(movie_count=movie_count, q_label_obj=self.status_LB) | ||
| if movie_count > 0: | ||
| self.show_movies_folder_PB.setEnabled(True) |
2 participants
This is a slight refactor of the movie creation: