Skip to content
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: Defer reboot when changing settings #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spohtl
Copy link

@spohtl spohtl commented Dec 28, 2024

When changing any settings that involve sending a message to AdminModule, the device immediately reboots, which is very annoying, and a frustrating user experience. This change modifies the behavior to only reboot either when the "Apply & Reboot" button is pressed, or when the user exits the settings menu.

User flows

Before

  1. Open the Settings tab -> Tap on Region -> Change to US -> Click OK -> Device reboots

After

  1. Open the Settings tab -> Tap on Region -> Change to US -> Click OK -> Tap on Device Role -> Change to Client -> Click OK -> Click "Apply & Reboot" -> Device reboots
  2. Open the Settings tab -> Tap on Region -> Change to US -> Click OK -> Tap on Device Role -> Change to Client -> Click OK -> Open the Home tab -> Device reboots
  3. Open the Settings tab -> Tap on Region -> Change to US -> Click OK -> Tap on Language -> Change to German -> Click OK -> Device reboots

Screenshots:

Screenshot_20241228_142602
The "Apply & Reboot" button if no changes are made (disabled). I decided to put the button on top of the menu so that the user immediately notices it, and is therefore aware that changes are not automatically applied.

Screenshot_20241228_142644
The "Apply & Reboot" button if a change is made which would require a reboot (user name, region, role, modem, ...). The banner on top changes text to "Unsaved changes ..." to let the user know that changes will be lost if power is turned off.

Screenshot_20241228_142927
A message pops up and the device reboots when the "Apply & Reboot" button is pressed.

Screenshot_20241228_143003
The same thing also happens if the user leaves the Settings page. A more user-friendly approach would be to pop up a dialog asking to apply&reboot, or discard changes. (TODO?)

Technical details

The AdminModule API supports opening a "config transaction", during which multiple config changes can be made without rebooting the device, and the changes are only applied once the transaction is closed. This is done with meshtastic_AdminMessage_begin_edit_settings_tag and meshtastic_AdminMessage_commit_edit_settings_tag.
I added two wrapper functions in ViewController: openConfigTransaction(uint32_t nodeId) and closeConfigTransaction(uint32_t nodeId).

A config transaction is opened when a config parameter is changed. This is done by calling TFTView_320x240::configTransactionOpen() before calling sendConfig, which also takes care of enabling the "Apply" button. Currently, I only implemented this for the following config items: WLAN, User name, Device role, Region, Modem preset, Alert buzzer, Language. TFTView_320x240::configTransactionOpen() should be added to any future config items tat get implemented. Local changes that don't involve AdminModule don't need to open a config transaction.

A config transaction can be closed in one of two (well, three) ways: by clicking on the "Apply & Reset" button, or by clicking on one of the buttons for selecting a different home screen tab (Home, Maps, Messages, ...). This is to prevent users from trying to use the device while the settings are not applied. The transaction is also immediately closed when the language is changed, since we want the change to be effective immediately (and a user might not understand what to do next until the language is changed). Either way, this is done by calling TFTView_320x240::checkForConfigTransactionClose(), which checks if any changes were done which require a reboot. If such changes were made, it pops up a warning that the device is about to reboot, and calls ViewController::closeConfigTransaction().

Notably, a config transaction cannot be cancelled once it's opened (or at least I couldn't find a way to do so), which complicates a potential implementation of a "Do you want to save or discard changes?" dialog. Any changes made would need to be buffered in the GUI, and only once the user hits "Apply" would the GUI open a config transaction, issue multiple sendConfig calls, and close the config transaction. This is how most config dialogs work, and would be the most user-friendly way to do things, so I think it should be considered a future TODO.

I did my best to google-translate the three newly added messages to the other implemented languages, but I have no way of verifying how good the translations are.

Testing

I tested this using the native-tft platform, as I'm still waiting for my T-Deck to arrive. I went through all the basic settings in all the languages. Some languages have text that is too long for the pop up message at the default resolution, but I don't know enough about languages like Finnish to know how to properly shorten words.

@spohtl
Copy link
Author

spohtl commented Dec 28, 2024

Also, it would be easier to add GUI features if the EEZ Studio project files were committed to the repo instead of just the generated files. I submitted an issue on Github asking for the project files, but @rcarteraz closed it, and seemingly disabled issues for this repo..?

@rcarteraz
Copy link
Member

Also, it would be easier to add GUI features if the EEZ Studio project files were committed to the repo instead of just the generated files. I submitted an issue on Github asking for the project files, but @rcarteraz closed it, and seemingly disabled issues for this repo..?

Yes, and I did comment as to why it was closed which is the same that will provided here. While we appreciate the contribution, we're not currently at a place where we're accepting contributions (outside of minor things like translations) at this time. We're waiting until after we've released our first preview which will hopefully be soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants