-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: add abstract app module and refactor config #206
feat: add abstract app module and refactor config #206
Conversation
Works on all three UIs offers a generic function to ask a question that platform independent. If the user fails to offer a response, the installer will terminate. In the GUI this still works, however it may not be desirable to prompt the user for each question. So long as we don't attempt to access the variable before the user has had a chance to put in their preferences it will not prompt them Changed the GUI to gray out the other widgets if the product is not selected. start_ensure_config is called AFTER product is set, if it's called before it attempts to figure out which platform it's on, prompting the user with an additional dialog (not ideal, but acceptable)
8042817
to
82d0c94
Compare
ou_dedetai/tui_app.py
Outdated
|
||
def get_version(self, dialog): | ||
self.product_e.wait() | ||
question = f"Which version of {config.FLPRODUCT} should the script install?" # noqa: E501 | ||
question = f"Which version of {self.conf.faithlife_product} should the script install?" # noqa: E501 |
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.
To my knowledge, I need to wait on self.product_e.wait() as if not, the TUI charges through the installer process; should this way now be handled by the TUI's implementation of app._hook_product_update()?
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.
Surprisingly enough it didn't charge through - it waited for the question to be answered before going to the next screen probably because we have two main threads in the TUI:
- The input processing thread (main, don't block me)
- When events are triggered they're spawned on a new thread (this is the installation thread). Since the ask call is blocking, this gets stopped while the user is inputting their value (which the processing is done on the first thread)
Then if you notice in TUI's ask implementation there is an event wait to communicate between the two threads
Comments on DemonstrationThanks for this! The framework you have here looks great. The TUI is a Frankenstein of my own thought, so anything that simplifies it and makes it less bloated is great—I do like seeing lines of code removed. As mentioned to Nate, I see tui_screen.py as a library that could feasibly be used outside of our project, so also the same with certain aspects of the display code in tui_app.py, say lines 1–350. LogosLinuxInstaller/ou_dedetai/tui_app.py Lines 1 to 350 in 0def2e5
(Given the hope of simplifying our queue/event code, many of these lines could be squashed/removed.) The task processor code in tui_app and in gui_app could also be brought into the abstract class. I would also hope that the choice_processor code in tui_app.py might find its way there. @n8marti has handled the GUI, so I will leave that to him. (Given your review, you might be the third person we've needed: someone who understands both the TUI and the GUI, haha.) I originally tried to code for the various UIs particularly in msg.py. This eventually got away from me and found its way back in msg.status(). There's likely a fair chunk of room for messaging to be brought into the abstract class given how much code is relatively unused in that module and how msg.status is accounting for each UI. The abstract class lets us do that without all the if/elif. General CommentsI had also tried this in installer, but the GUI needed enough odds and ends to be separated at the time. Now that we have our working base, I think it'd be great to try to reel in the various odd bits and make these more united, all in the spirit of #147. As mentioned elsewhere, this would also be helpful for #2 and #87, and for drastically improving code reusability/maintenance. Given your further comments about the suggested config changes, that would go well with #187. While I think all these issues are too much for one PR, I do think we could lump #147 and #187 into this PR's scope as a way of refactoring our code. Thinking Out LoudThis framework might enable me to further simplify the various calls within tui_app to tui_screen. tui_curses and tui_dialog are fairly static at this point. There are some parts of tui_screen that need to be abstracted, particularly in the console_screen. I've also considered changing the tui_screen class to utilize a method of the tui_app that flags the need for tui_app.refresh to be run again. |
Am I understanding correctly that in the GUI the user will see the "Choose which FaithLife product" window first, then they will see the GUI installer window, with all the options pre-populated with defaults? So then they can make adjustments, or just click "Install"? |
Not quite, I liked the current GUI flow so much I didn't want to modify it. Before and after this PR it behaves the same, however if someday in the future there was a code path that tried to retrieve the faithlife product before the prompt showed up, it would open a separate dialog asking that question. In the GUI's case we probably want to avoid this, however the code will handle that case and avoid an error if such a code path were to exist in the future. |
Okay, I'm happy with that. |
If you @ctrlaltf24 can take care of the potential merge conflicts, then I'll look it over again for approval. |
Expanding usage of this framework.... |
and migrate APPDIR_BINDIR and WINESERVER_EXE
Before the install button wouldn't enable when the network cache returned [], this new logic is more robust Tested: - editing network cache manually and setting the releases to an empty array - editing config to remove the release - editing both of the above to remove both System recovered in all cases
Fixed in 9bac0f1 |
what were the results of looking at the wine.log? |
fixed in 5e14603 |
Nothing in wine.log besides the subprocess command. But here's what I see in
Running directly from the terminal opens correctly and does not show any "Could not load" errors. However, in this same installation subsequent attempts to open with the
I have no idea what memory that would be referring to. There are no low-memory errors in |
This patch solves both the hung process issue and the issue of Logos not starting after a clean install. Take a look at it and see if you're happy with it: |
Also remove -p flag to check_wineserver, both found by Nate to be ineffective Co-Authored by: Nate Marti <[email protected]>
Co-Authored by: Nate Marti <[email protected]>
Co-Authored-By: T. H. Wright <[email protected]>
os.makedirs(os.path.dirname(app_log_path), exist_ok=True) | ||
|
||
# Ensure log file parent folders exist. | ||
log_parent = Path(config.LOGOS_LOG).parent | ||
if not log_parent.is_dir(): | ||
log_parent.mkdir(parents=True) | ||
log_parent = Path(app_log_path).parent | ||
log_parent.mkdir(parents=True, exist_ok=True) |
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.
Isn't running os.makedirs
redundant with log_parent.mkdir
?
I'm very happy with the simplifications and strong foundation that this refactor brings in. I see some typos and other minor fixes that should be done, as well as a full conversion to ruff linting, but those shouldn't hold up this merge. We can do a cleanup PR in the near future. |
was removed due to unused before, now it's used to filter out v39
observed when window was resized ``` Traceback (most recent call last): File "<frozen runpy>", line 198, in _run_module_as_main File "<frozen runpy>", line 88, in _run_code File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/main.py", line 447, in <module> main() File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/main.py", line 442, in main run(ephemeral_config, action) File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/main.py", line 380, in run action(ephemeral_config) # run control_panel right away ^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/main.py", line 339, in run_control_panel raise e File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/main.py", line 328, in run_control_panel curses.wrapper(tui_app.control_panel_app, ephemeral_config) File "/home/user/.local/lib/python3.12/curses/__init__.py", line 94, in wrapper return func(stdscr, *args, **kwds) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/tui_app.py", line 1235, in control_panel_app TUI(stdscr, ephemeral_config).run() File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/tui_app.py", line 463, in run self.display() File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/tui_app.py", line 422, in display self.active_screen.display() File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/tui_screen.py", line 244, in display time.sleep(0.1) File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/tui_app.py", line 364, in signal_resize self.resize_curses() File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/tui_app.py", line 360, in resize_curses logging.debug("Window resized.", self) File "/home/user/.local/lib/python3.12/logging/__init__.py", line 2226, in debug root.debug(msg, *args, **kwargs) File "/home/user/.local/lib/python3.12/logging/__init__.py", line 1527, in debug self._log(DEBUG, msg, args, **kwargs) File "/home/user/.local/lib/python3.12/logging/__init__.py", line 1684, in _log self.handle(record) File "/home/user/.local/lib/python3.12/logging/__init__.py", line 1700, in handle self.callHandlers(record) File "/home/user/.local/lib/python3.12/logging/__init__.py", line 1762, in callHandlers hdlr.handle(record) File "/home/user/.local/lib/python3.12/logging/__init__.py", line 1022, in handle rv = self.filter(record) ^^^^^^^^^^^^^^^^^^^ File "/home/user/.local/lib/python3.12/logging/__init__.py", line 858, in filter result = f.filter(record) ^^^^^^^^^^^^^^^^ File "/home/user/LogosLinuxInstaller-bravo/ou_dedetai/msg.py", line 42, in filter current_message = record.getMessage() ^^^^^^^^^^^^^^^^^^^ File "/home/user/.local/lib/python3.12/logging/__init__.py", line 392, in getMessage msg = msg % self.args ~~~~^~~~~~~~~~~ TypeError: not all arguments converted during string formatting ```
Setting switch_q to 1 when contents were changed removed the pending ask screen, causing install to stop halfway Also it's possible for the TUI to ask for the installed_faithlife_product_release before install_dir is set, causing two threads to both be asking, causing a invalid response. The application would probably recover, but it's not ideal.
Tested clean installs on TUI, GUI, and CLI one last time, all get to logos login page (needed a couple very simple bug fixes) PR ready to Squash and merge @thw26 (using this PR's description as the commit body). Don't want these individual commits on main, as the code isn't stable in between all the commits, no reason to have the history for what in essence is a scratch pad. Commit history will remain in this PR if we need it later. |
Improvements
Type Enforcement
Types are checked using a static analysis tool (mypy) to ensure nothing is None when it shouldn't be (or a Path when it should be a string)
Network Caching
All network requests are cached
Config Enforcement
Wrapper struct to ensure all config variables are set, if not the user is prompted (invalid answers are prompted again).
Additionally configuration hooks were added so all UIs can refresh themselves if the config changes underneath them. A reload function has also been added (TUI only as it's for power users and offers no benefit to the cli)
Config Format Change
In the interest of consistency of variable names, the config keys have changed. Legacy values will continue to be read (and written). New keys take precedence. This new config can be copied to older versions of the installer and it should (mostly) work (not aware of any deficiencies however downgrading is hard to support). Legacy paths are moved to the new location.
Graphical User Interface (tinker)
Ensured that the ask function was never called. We want to use the given UI, as it's prettier. However if the case should arise where we accidentally access a variable before we set it, the user will see a pop-up with the one question. Data flow has been changed to pull all values from the config, it no longer stores copies. It also now populates all drop-downs in real-time as other drop-downs are changed, as a result there was no longer any need for the "Get EXE" and "Get Release List" buttons, so they were removed. Progress bar now considers the entire installation rather than "completing" after each step.
Terminal User Interface (curses)
Appears the same as before from the user's standpoint, however now there is a generic ask page, greatly cutting down on the number of queues/Events. There may be some more unused Queues/Events lingering, didn't see value in cleaning them up.
Command Line Interface
Prompts appears the same as before, progress bar is now prepended to status lines
Misc other changes
Closing Notes
One goal of this refactor was to keep the code from being understood by someone who is already familiar with it. There is a couple new concepts, the abstract base class, reworked config, and network cache, but the core logic of how it works remains unchanged. If something isn't clear please ask.
Screenshots
GUI Sample
TUI Sample
CLI Sample
GUI behavior
Before Product is selected
After product is selected
Fixes: #147, #234, #35, #168, #155