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

Canned message usability improvements #4437

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

robertfisk
Copy link
Contributor

This PR fixes some usability issues I've encountered with the canned message module and a standalone keyboard.

  1. The Screen timer jumps away from the CannedMessageModule frame while the user is typing a message. Kinda annoying.
    Solution: Return early from Screen::handleOnPress if cannedMessageModule->shouldDraw() returns True.

  2. The CannedMessageModule thread was not being scheduled to process the INACTIVATE_AFTER_MS timeout, because CannedMessageModule::runOnce() was running in the caller's context (e.g. the keyboard thread).
    Solution: Pass the return value of CannedMessageModule::runOnce() to setIntervalFromNow().

  3. Don't reset half-typed messages when timing out back to regular screens, so the user can resume typing later.

  4. Canned message list sometimes fails to timeout. Solution: Change timeout comparison operator from ">" to ">="

  5. Don't inconsistently clear freetext when scrolling canned message list.

  6. Restore old canned message state after displaying popup message (the user might be typing another msg). Also don't clear freetext/state after displaying popup message.

@CLAassistant
Copy link

CLAassistant commented Aug 11, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@todd-herbert todd-herbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with "scan and select", "up down select" and the RAK14014/T-Watch "free-text" system.
Everything seems to be working fine 👍 Just the one request to also apply your fix to the "scan and select" input (see below).


This PR does prevent the user from exiting the canned message selection screen via a user-button press. Just with a quick test, it feels like this won't be an issue, as the timeout for this selection screen is short (20 seconds).

I would like to know that @tropho23 is comfortable with the change, though. I believe some of his designs use rotary encoder inputs.

src/graphics/Screen.cpp Show resolved Hide resolved
todd-herbert
todd-herbert previously approved these changes Aug 14, 2024
Copy link
Contributor

@todd-herbert todd-herbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: user button will no longer dismiss the canned message frame.
Some input methods will need to wait 20 seconds for timeout.

Merge only if this is acceptable.

@HarukiToreda
Copy link
Contributor

Note: user button will no longer dismiss the canned message frame. Some input methods will need to wait 20 seconds for timeout.

Merge only if this is acceptable.

is there a way to allow the user button to still dismiss the canned message frame? it's a button and it can be easily pressed by mistake. Having to wait 20sec does stop you from looking at other things that may be needed urgently.

@HarukiToreda
Copy link
Contributor

This PR fixes some usability issues I've encountered with the canned message module and a standalone keyboard.

  1. The Screen timer jumps away from the CannedMessageModule frame while the user is typing a message. Kinda annoying.
    Solution: Return early from Screen::handleOnPress if cannedMessageModule->shouldDraw() returns True.
  2. The CannedMessageModule thread was not being scheduled to process the INACTIVATE_AFTER_MS timeout, because CannedMessageModule::runOnce() was running in the caller's context (e.g. the keyboard thread).
    Solution: Pass the return value of CannedMessageModule::runOnce() to setIntervalFromNow().
  3. Don't reset half-typed messages when timing out back to regular screens, so the user can resume typing later.
  4. Canned message list sometimes fails to timeout. Solution: Change timeout comparison operator from ">" to ">="
  5. Don't inconsistently clear freetext when scrolling canned message list.
  6. Restore old canned message state after displaying popup message (the user might be typing another msg). Also don't clear freetext/state after displaying popup message.

does this allow the escape button to still clear the message? because having to backspace a whole message may be a huge chore we didn't otherwise have to do before.

@todd-herbert
Copy link
Contributor

is there a way to allow the user button to still dismiss the canned message frame?

There'd definitely be a way to make that happen. It'd mean suppressing the carousel behavior in a slightly different spot; maybe somewhere like here?

@tropho23
Copy link
Contributor

This PR fixes some usability issues I've encountered with the canned message module and a standalone keyboard.

1. The Screen timer jumps away from the CannedMessageModule frame while the user is typing a message. Kinda annoying.
   **Solution:** Return early from Screen::handleOnPress if cannedMessageModule->shouldDraw() returns True.

2. The CannedMessageModule thread was not being scheduled to process the INACTIVATE_AFTER_MS timeout, because CannedMessageModule::runOnce() was running in the caller's context (e.g. the keyboard thread).
   **Solution:** Pass the return value of CannedMessageModule::runOnce() to setIntervalFromNow().

3. Don't reset half-typed messages when timing out back to regular screens, so the user can resume typing later.

4. Canned message list sometimes fails to timeout. **Solution:** Change timeout comparison operator from ">" to ">="

5. Don't inconsistently clear freetext when scrolling canned message list.

6. Restore old canned message state after displaying popup message (the user might be typing another msg). Also don't clear freetext/state after displaying popup message.

I'm not seeing a 20 second timeout, or any timeout when I start typing and stop using CardKB. I'm running the 2.5.0 technical preview on a RAK board. Are you using some custom firmware build where this occurs?

@tropho23
Copy link
Contributor

Here's my point-by-point observation of your observations:

1. The Screen timer jumps away from the CannedMessageModule frame while the user is typing a message. Kinda annoying.

  • I don't experience this.

2. The CannedMessageModule thread was not being scheduled to process the INACTIVATE_AFTER_MS timeout, because CannedMessageModule::runOnce() was running in the caller's context (e.g. the keyboard thread).

  • No comment.

3. Don't reset half-typed messages when timing out back to regular screens, so the user can resume typing later.

  • I don't experience a timeout, and half-typed messages are not reset. Just keep typing, it switches back to the freetext entry screen and you can pick up where you left off.

4. Canned message list sometimes fails to timeout.

  • Again I never experience a timeout at all, so if there is supposed to be one then it is indeed failing.

5. Don't inconsistently clear freetext when scrolling canned message list.

  • I experience clearing of a typed, but not sent message if I start typing then press the up or down key to enter canned messages mode. If I resume typing the previously typed, unsent message has been cleared. This happens every time for me, not inconsistent.

6. Restore old canned message state after displaying popup message (the user might be typing another msg). Also don't clear freetext/state after displaying popup message.

  • This would be nice, I agree. Right now you can resume typing but it's not obvious. If the user presses the Enter key, the half-typed message is sent. If the user presses Esc in an attempt to clear the received message screen, the half-typed message is lost.

@todd-herbert todd-herbert dismissed their stale review August 28, 2024 15:54

New feedback from CardKB users

@robertfisk
Copy link
Contributor Author

robertfisk commented Aug 29, 2024

I'm not seeing a 20 second timeout, or any timeout when I start typing and stop using CardKB. I'm running the 2.5.0 technical preview on a RAK board. Are you using some custom firmware build where this occurs?

I saw the carousel timeout issue when testing with 2.4.2 and 2.4.3 on a RAK4631. Do you have "Auto screen carousel" set to a low number like 10 seconds? If so then I will re-check on 2.5.0

The 20 sec canned message timeout is currently nonfunctional due to a couple of bugs, fixed in this PR.

@robertfisk
Copy link
Contributor Author

does this allow the escape button to still clear the message? because having to backspace a whole message may be a huge chore we didn't otherwise have to do before.

Yes, canned message will still close on reception of a meshtastic_ModuleConfig_CannedMessageConfig_InputEventChar_CANCEL message, which is generated by the ESC key

- Don't let Screen timer jump away from CannedMessageModule frame
- Correctly timeout CannedMessageModule with INACTIVATE_AFTER_MS
- Don't reset half-typed messages on timeout
- Reliably timout when showing canned message list
- Don't inconsistently clear freetext when scrolling canned message list
- Restore old canned message state after displaying message
- Don't clear freetext/state after displaying message
@fifieldt
Copy link
Contributor

@tropho23 - does this patch break anything for you?

@caveman99 caveman99 added the pinned Exclude from stale processing label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Exclude from stale processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants