-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix: TUI color scheme cycle hang #302
base: main
Are you sure you want to change the base?
fix: TUI color scheme cycle hang #302
Conversation
Fix: FaithLife-Community#278 remove unneeded TUI queues also removed a clear/refresh in update_main_window_contents that may been the race condition we were looking for. This is done in draw. It renders correctly
it hangs the TUI too. Switched to a logging message for CLI users
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.
One of the changes has made the TUI color scheme selection work only once and then fail to work again. This kind of issue usually occurs because of the way choices are handled in relation to screens.
Once a choice is selected for a screen, that choice is "locked" to that instance of a screen in the current code.
Most likely what is happening is that we are interacting with an old instance of the main menu rather than the new instance, likely due to removing the switch_q.
The current method for drawing is to display the nth screen, then pop it off and return to the previous screen. Most (all?) of the time this is only two screens: main menu and some other menu. We currently associate screen with choice in order to handle processing: how do we know which option was selected without associating an option to a menu? We would need to refactor this so that choices are associated with their screen but handled independently of it. See tui_screen.py for how this is currently handled. The screen inserts its choice to the choice_q but that choice is not cleared on the screen. We could, for instance, submit both the screen ID and the choice so that we couple them together, and then clear the choice so that the menu is once again freed from being locked to its choice. This is also related to the fact that the menu's state is handled, whether it is first run or already run. Perhaps we could modify the screens to make the menus reusable by removing this. IIRC, these states were used to handle speeding up installs by not asking a user for the same choices over and over again, a way of handling restoring an install, if you will. With our other refactors, this may not longer be necessary. As a possible issue, if we remove this functionality, it may break the sequential choosing found in the installer. So we would need to test the install process, to see if it keeps working normally, and also if we return to the main menu during an install and resume the install, is the install procedure handled properly? |
I just tested this again and I cannot replicate the error I just posted about. I'm unsure why it happened then but not now. |
Current replication of problem:
|
This matches up with my description of the problem. So long as we keep hitting change color scheme, the choice associated with the menu remains the same. On attempting to change the choice associated with the menu, it fails, and subsequent choices then fail. |
Fix: #278
remove unneeded TUI queues
also removed a clear/refresh in update_main_window_contents that may been the race condition we were looking for. This is done in draw. It renders correctly
Tested: