Fix GUI for new project reestructure #53
Conversation
|
@etiontdn Yes I was actually just on my way to say that a few PRs have gone through since the original GUI attempt, including two major project restructuring PRs. It would be good to try and sync you work back up with the master branch. Let me know if you have any questions or if you are having trouble with something. |
etiontdn
changed the title
|
Ok, fixed the errors and it looks like it works. I found a little bug i think, |
|
@etiontdn I am at work and can't review this today, but will as soon as possible. I will pay particular attention to comments and will leave a note on anything that may need to be changed. That was a bug I found recently too and opened issue #50 to discuss. I include a possible solution in that thread, take a look and let me know if you have any ideas or input. |
|
@etiontdn I started working on this, I made a few small changes directly to your branch. I am still going through it. I think the comments have improved overall and the GUI seems functional on my end. |
|
@etiontdn One thing I have noticed is that if you open the search files button and then back out of the popup, the instructions in the corresponding field disappear. Does this happen to you? |
|
@brettvanderwerff Tested it and happens maybe we should add a validate function that will validate if the entry is empty, path doesn't exist, etc. |
|
@brettvanderwerff
Any feedback or suggestion is welcome! |
|
@etiontdn I see you made some recent commits. This is a busy weekend for me but I'll take a look at the changes as soon as I can 👍 thanks for being so engaged in the project! |
|
@etiontdn Nice work here. Validation is super important in addition to being able to have a case to deal with any unintended input a user might make. Please see my comments and let me know if you have any questions. The GUI is getting better with every round of work! |
|
|
||
| class TestGUI(unittest.TestCase): | ||
|
|
||
| def setUp(self): |
| path = filedialog.askopenfilename(title= "Select the main html file normally index.html", | ||
| filetypes=(("Html Files", "*.html"), ("all files", "*.*"))) | ||
| self.html_location.set(path) | ||
| if self.validate_path(path): |
There was a problem hiding this comment.
Here it seems to take the path in any case where it is a path to a real file. You could use the endswith('.html') string method to validate that they actually select and html file or something like that. Anytime you have user input be it a gui or a webform the law is "never trust the user". Someone will switch the accept all files, pick a .jpg, and then wonder why it is not working and we need to have things laid out in a way that prevents headaches.
In my opinion it should be like this:
-
User opens dialog box and selects path to a valid .html file, the path gets presented in the box
-
User opens dialog box then backs out again right away without selecting anything "Select one HTML file from the template" gets recycled in the box, because nothing has changed really
-
User opens dialog box and picks anything other than and html file, some informative warning gets presented that they need to pick an html file
| and sets the html_location variable with the returned path value. | ||
| """ | ||
|
|
||
| path = filedialog.askopenfilename(title= "Select the main html file normally index.html", |
There was a problem hiding this comment.
Should the title be consistent with "Select one HTML file from the template" because we changed the prompt? I am actually not sure. Same for the others.
There was a problem hiding this comment.
Yes it is the title of the window i think it should be consistent.
It could even have more information since it has more space and etc.
| "JS Location"]) | ||
|
|
||
| def test_path(self): | ||
| """Tests the path_to_folder function from ChooseFilesGUI |
There was a problem hiding this comment.
This docstring is a bit difficult to read. I am not 100% sure what this test is doing, so I am not sure how to help you improve it. Could you try to reword it?
| return False | ||
|
|
||
| def validate_entries(self): | ||
| """Validates all entries and if |
There was a problem hiding this comment.
Maybe you can comment back and let me know, but isn't there validation of entries as the user is selecting the paths? I am trying to imagine a scenario of how the user can select invalid entries in the first place and have this function return false.
There was a problem hiding this comment.
I added it because manual input is an option as well since they are entries the user can type a path or copy & paste on it.
| if not js: | ||
| self.error_entries.append("'JavaScript File location' ") | ||
| if not html or not static or not js: | ||
| self.error_message = "There is errors in the entries: " |
There was a problem hiding this comment.
There are errors in the following entries:
|
|
||
| def get_values(self): | ||
| """Gets the html_location, static_location and js_location | ||
| values and sets them to new variables then uses them |
There was a problem hiding this comment.
Gets the html_location, static_location and js_location
values for paths to the HTML files, 'static' folder content, and Javascript files of the Bootstrap template. These paths are passed to the StructureDirectory class for flaskerization.
|
@etiontdn I went though it and made really small changes, mostly just last minute wording stuff. I want to give you a very special thank you because this was a big undertaking and you were extremely communicative and involved in the project over a long period of time. you did a really good job putting this together and I think it will help make the flaskerizer accessible to some people that would otherwise not use it without a gui. I am merging this now. Thanks again for all the time you spent on making this a great addition. |
|
@brettvanderwerff Thanks! The time i spent working on this was learning and contributing to the awesome world that is open source. |
Made the gui compatible with the new project reestructure.