-
Notifications
You must be signed in to change notification settings - Fork 827
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
Screen set frames update #4043
Screen set frames update #4043
Conversation
|
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.
Just something else to consider. We have pop-up screens especially in the Audio and Canned Messages module (PTT; Message Status, Delivery report and so on...), but also for functionality that is enabled with keypresses. These screens still need to take fcocus, but of course should return to the last screen shown and not the first one
src/graphics/Screen.cpp
Outdated
|
||
if (currentFrameNum > numframes - 1) { // If we were on a frame that no longer exists | ||
ui->switchToFrame(numframes - 2); // Attempt to return to last frame | ||
} |
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.
What if that one doesn't exist as well? i'd rather return to the first frame, there's always one frame.
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.
Ah I think I might have caused some of this confusion. I made a (probably unclear) suggestion that it would be good to store numFrames
at the end of the method, so that next time setFrames()
is called, we can detect whether we were previously on the log buffer / settings frames, and then move to either (numFrames - 1) or (numFrames - 2), which would handle changes to numFrames.
I'm not familiar enough with the modules to know exactly what needs to be accommodated, but I can certainly imagine the sort of attention to detail it'd take to catch all those little cases. Now that you mention this, I wonder if rather than trying to catch every case, it might be better to just target one: If we changed the signature of |
Added code to attempt to revert back to the same frame that user was on prior to setFrame reload.
Make screen to revert to Frame 0 if the originally displayed frame is no longer there.
67f3fcf
to
cbd49ed
Compare
Inserted boolean holdPosition into setFrames to indicate the requirement to stay on the same frame ( if =true) or else it will switch to new frame . Only Screen::handleStatusUpdate calls with setFrame(true). ( For Node Updates) All other types of updates call as before setFrame(), so it will change focus as needed.
Hi guys, sorry attention drifted. I got your points and thanks @caveman99 and @todd-herbert for attention to detail. |
Hey no problem, really excited to see some action back on this one! |
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.
I'll send through a PR to slash-bit/meshtastic-echo-echo
with some fixes I had handy for this. It also catches one extra case, where the frame changes when user sends a text message.
I don't think these extra changes should disrupt any other modules, but if you were going to do some testing there, that'd be really helpful too.
There's one more spot I'd like to tackle tomorrow, where Screen::setScreensaverFrames()
tries to do the same job of returning to the same frame (not as nicely as the code we've been working on here though!). It can totally just use setFrames(true)
now instead; I'll test that out on my E-Ink stuff here.
src/graphics/Screen.cpp
Outdated
if (holdPosition) { | ||
ui->switchToFrame(currentFrameNum); // Attempt to return to same frame after rebuilding the frames, | ||
// if holdPosition is true (currently only Screen::handleStatusUpdate calls this | ||
} else { | ||
continue; // We leave the displayed frame as it is or chnage focuse to new frame |
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.
I did have a bit of a play with this a few days ago, and I think there might be a sneaky issue here: if you're trying to stay on the "log buffer" frame, and the number of frames increases, switchToFrame(currentFrameNum)
won't put you back in the same place.
src/graphics/Screen.cpp
Outdated
@@ -2054,9 +2054,11 @@ void Screen::setScreensaverFrames(FrameCallback einkScreensaver) | |||
#endif | |||
|
|||
// restore our regular frame list | |||
void Screen::setFrames() | |||
void Screen::setFrames(bool holdPosition = false) |
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.
Don't forget the one declaration in Screen.h either!
(because of the order of Screen.cpp, I also think you'll want to set that =false
default argument in Screen.h instead)
Great stuff, I think we are getting there! |
Collaboration on meshtastic#4043
Hey all, I have done some tests on the latest branch.
|
Just wanted to send a quick reply so I didn't leave you hanging here. I see you've got a fix out for those two. I haven't double checked, but I'm pretty sure I looked into it today too, and apparently manually removing a node isnt being done by |
Yeah, thanks. I suspected something wasn't right with devicestate.has_rx_text_message during limited testing that I have done . Will revert. Then other possible solution maybe to use handleTextMessage as a starting point to trigger switch to a message frame. firmware/src/graphics/Screen.cpp Line 2658 in 39c9f92
I am also not a user of the Waypoint or Telemetry in fact, but just reading commented lines in the code, I understand that's the intention to switch to those frames. Yes, it ll be nice to clarify about canned message , so we may get that sorted as well. I crack on with looking for a solution, and better stay away from the heated debate in the discord :-) |
I think if we can mark when a new waypoint arrives (and when a node is being removed manually from the db), we can probably move to the right frames at that point, without having to worry about what other modules might be doing. I've just been working on some code to pass this info through to Edit: The way I was handling it was over-thinking the whole thing. I don't have this code done tonight sorry, but if you want to have a go at doing the first part too, I was thinking maybe with two new Observables, then passing a "TargetFrame" enum to setFrames, instead of the bool. |
984f070
to
1243987
Compare
(Whoops, accidentally pushed here by mistake, when I meant to send it as a PR) |
One more thing I spotted: I don't think the firmware code handles "deleting a waypoint" yet, at least not on the device. |
:-) Aha , I was thinking the same , pass enum instead of boolean. But then I got distracted by .has_rx_text_message. |
This can be interesting one to find you. |
Actually, maybe it'd even work to record the value of numframes just before drawTextMessageFrame etc are called? |
Thank you for sending in a pull request, here's some tips to get started!
This PR fixes #4038
every time setFrame run the page reverts back to Message or Module frame , but not the same frame that was viewed by user before, which could be annoying having to manually switch back to say Status frame.
So I have added lines where it would keep the same Frame number that user was one prior to reloading the frames. If that frame still available.
There may be some more enhancements needed in the future to accommodate when total number of frames changes ( like enabling/disabling modules or resetting Node DB.
For now, testing on T-Echo seems reasonably stable and stays on the same frame while device runs.