-
Notifications
You must be signed in to change notification settings - Fork 4
Feature/include file input #31
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
base: main
Are you sure you want to change the base?
Feature/include file input #31
Conversation
dragos-ana
left a 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.
Thanks for the nice addition!
I left some comments / questions.
| temp_dir=Path(self._server_vars["temp_dir_object"].name), | ||
| ) | ||
| self.state.vtu_path = fourc_geometry.vtu_file_path | ||
| if "render_window" not in self._actors: |
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.
| if "render_window" not in self._actors: | |
| if "render_window" not in self._server_vars: |
As in #30, the render window should be saved only once in the server vars.
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.
Yes of course. Overlooked it again
| self._server_vars["temp_dir_object"].name | ||
| + "\\" | ||
| + self._server_vars["fourc_yaml_name"] | ||
| ).exists(): |
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.
| self._server_vars["temp_dir_object"].name | |
| + "\\" | |
| + self._server_vars["fourc_yaml_name"] | |
| ).exists(): | |
| self._server_vars["temp_dir_object"].name | |
| / | |
| self._server_vars["fourc_yaml_name"] | |
| ).exists(): |
The current code is Windows-only! We don't need to specify delimiters for Path, but can use a normal forward-slash.
Please adapt this for each occurence, the webviewer does not start for other OS.
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.
Sorry, I didn't know about that. I will change it
| if not Path( | ||
| self._server_vars["temp_dir_object"].name | ||
| + "\\" | ||
| + Path( | ||
| self._server_vars["fourc_yaml_content"] | ||
| .sections.get("STRUCTURE GEOMETRY", {}) | ||
| .get("FILE") | ||
| ).name | ||
| ).exists(): |
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.
Please make this better readable by splitting the long chain into dedicated variables. You can already reuse geometry_file_name
| + "\\" | ||
| + self._server_vars["fourc_yaml_name"] | ||
| ) | ||
| geometry_file_name = ( |
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.
| geometry_file_name = ( | |
| # verify whether an eventual included geometry file exists within the temp dir | |
| geometry_file_name = ( |
| if "e" in all_contained_var_names: | ||
| all_contained_var_names.remove("e") | ||
| if "E" in all_contained_var_names: | ||
| all_contained_var_names.remove("E") |
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.
Maybe this check is better suited within get_variable_names_in_funct_expression directly
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.
Yes definitely. At the time it was just supposed to be a quick fix so i could open all the yaml files, but I will implement it properly now.
| with open(include_file_server, "rb") as fr: | ||
| with open(include_temp_path, "wb") as fw: | ||
| fw.write(fr.read()) |
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.
shutil.copyfile might be better for this. Also, what should this check provide? If a file with the same name is present in the initial file directory, then it should be copied to the temp dir? What about the case where you upload a completely new file, and within its INCLUDES section it references to a file which coincidentally has the same name as one of the files present in the initial file directory? Then it would basically get replaced by the old file, right? If so, this should be changed.
IMO, this function requires some comments to better understand the information flow.
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.
I commented the file so it can be better understood.
The reason why I added this copy functionality is, that if the user is working on a specific yaml file and uses the webviewer frequently, they should not have to upload the exodus file every time they open the yaml file. Now he can copy the .exo file into the server directory and it will be automatically loaded every time the file gets opened.
The important thing is that include_file_server does only refer to the initial directory if the file was uploaded from the server. Which would not be the case in most situations.
Of course I can remove this feature, but I believe that it could be very nice for quality of life.
I think it would be best if we had a small meeting about this and some other final things about the viewer.
| print( | ||
| "Warning: File does not have a .yml / .yaml / .dat / .DAT ending or is empty. Try opening another file." | ||
| ) | ||
| return |
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.
Why not raise an Exception?
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.
The viewer should not be closed if the user opens a wrong file. He can just open the right one without any problem. I can remove the Warning if you think its not adequate.
| self._server_vars["render_count"]["change_fourc_yaml_file"] += 1 | ||
| if self._server_vars["render_count"]["change_fourc_yaml_file"] > 1: | ||
| self.state.vtu_path = "" | ||
| if self._server_vars["fourc_yaml_read_in_status"]: |
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.
Shouldn't self.state.included_files be emptied at some point when changing files? We would not want residual files from the previous study within the current session...
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.
Yes, it gets emptied once you request the include files (request_included_files) which recalculates the needed files and prompts the user to upload them
|
|
||
| self._server_vars["fourc_yaml_name"] = temp_fourc_yaml_file.name | ||
|
|
||
| self.request_included_files() |
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.
Here you removed the render_count part, which emptied the vtu path if a new file was considered. This was only done for "non-initial" renderings, because this function is also called at startup. The vtu path was then created subsequent to conversion after pressing CONVERT. Does this work reliably after removing this aspect? Does changing files make the visualization disappear until you press CONVERT?
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.
This is also a topic I would like to discuss. I removed the convert functionality (although some artifacts of it remain currently) as I did not see the need to explicitly ask the user to confirm. The vtu path is now set in init_pyvista_render_objects, so exactly as the actors are set up. This works reliably and I have not encountered any problems until now.
| ), | ||
| blur=server.controller.on_leave_edit_field, | ||
| update_modelValue="flushState('general_sections')", # this is required in order to flush the state changes correctly to the server, as our passed on v-model is a nested variable | ||
| update_modelValue="flushState('add_value')", # this is required in order to flush the state changes correctly to the server, as our passed on v-model is a nested variable |
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.
I think this was more reliable for changes of general_sections rather than add_value. What is the motivation behind changing it? And why is the blur removed?
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.
This was a bug fix of the add value field at the bottom right of the edit mode. It should not flush general_sections because the variable it refences is add_value which is not part of general_sections.
No description provided.