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

Interactive/progressive checklist #1417

Closed
wants to merge 30 commits into from

Conversation

piotrva
Copy link
Contributor

@piotrva piotrva commented Jan 11, 2022

Summary of changes:
Adds an option to display check-boxes at the beginning of each checklist / model notes line.

  • Item to be currently processed has selected (active) check-box, confirmed item - selected checkbox, unconfirmed item - empty.
  • You can only advance by one in the checklist (by clicking ENTER - rotary encoder click).
  • You can normally scroll the checklist if it exceeds screen size and in addition it is automatically scrolled when currently processed item is at the bottom (or row above bottom) and there are more items available.
  • You can remove displaying and processing of some items by placing = sign as a first character in a line
  • You can mark only items, that are displayed on a screen (no off-screen item confirmation)
  • Interactive checklist loaded on model selection (and after reset flight) might be closed only when complete (additional ENTER click after last item completed), see Checkable preflight-checklist #133
  • Interactive checklist loaded from View Notes is closed by pressing EXIT/RTN, no check-boxes are displayed in the view.

image
image
image

Notes:

  • All changes were tested only on following hardware: Radiomaster TX12
  • Changes for colorlcd tested only in simulator
  • Tested that opening yaml file without positions for the settings is safe (defaults to this option disabled - backwards compatible)

@pfeerick
Copy link
Member

Nice... so this goes towards resolving #133 for B&W, and gives a model for how it could be done for colorlcd as well.

@piotrva
Copy link
Contributor Author

piotrva commented Jan 11, 2022

So colorlcd does not have such an option (I am getting mine in a few weeks - delivery from CN)?
Actually as of 8755465 the checklist can be closed by clicking EXIT/RTN button (at any state).
I was considering to make this not even seeing that issue.
Work in progress - should be one commit hopefully soon ;)

@pfeerick
Copy link
Member

No, colorlcd is basically the same as B&W in that respect - it can show the model notes file as pre-flight checklist, but there is no interactivity.

@piotrva
Copy link
Contributor Author

piotrva commented Jan 12, 2022

So for now it should resolve #133 for B&W radios - I will probably go with color version as I get my radio, cause otherwise I have no way to test it.
A thing to discuss is:

  • Interactive checklist loaded from View Notes menu might also be closed by pressing EXIT/RTN

It might be easily changed with one condition, and I think it is not necessary to have this option to enable/disable in the menu. If someone might comment if my approach on this is right or not - I would appreciate.

@pfeerick
Copy link
Member

Just to clarify - so when you have it set as interactive, it would be interactive on powerup/model switch, but when you view notes, not-interactive? If so, I think this is the correct behaviour - as you said you wanted to view the notes, not redo the checklist. You can reset the flight if you want to do that, as that will go through all the pre-flight checks again.

@piotrva
Copy link
Contributor Author

piotrva commented Jan 12, 2022

When you view notes - check-boxes are still displayed and you can still go-through them and complete the checklist but in addition you can close checklist by pressing RTN/EXIT (so in addition not instead).

@piotrva
Copy link
Contributor Author

piotrva commented Jan 12, 2022

Of course it might be changed - this is open for discussion ;)

@pfeerick
Copy link
Member

I'm not really fussed either way. I would have probably gone with not showing the checkboxes in the view notes dialog... but lets see if others have any thoughts on that.

@piotrva
Copy link
Contributor Author

piotrva commented Jan 12, 2022

That would be reasonable, as it would clearly indicate this this window is to be closed in the other way. Also a very simple change 😄
Of course if the way I manage this is OK.

@wimalopaan
Copy link
Contributor

Very cool! Thank you!

@wimalopaan
Copy link
Contributor

Maybe you should rename checklistInteractiveBW to checklistInteractive because it is also introduced for the colorLCDs

@piotrva
Copy link
Contributor Author

piotrva commented Jan 12, 2022

@wimalopaan I will rename this setting, as (as you can see beginning of the discussion) I have thought that colorlcd already have this feature hah 😄
And what about loading checklist as "View Notes" ?

@gagarinlg
Copy link
Member

I think you should try to do this on the color lcd radios also. This just UI stuff that can be perfectly developed with simu/simulator, probably even better than on the radio itself. Tests on the real hardware can be done by he people who have access to all the different kinds of radios. I made a feature only of B/W radios without ever having one in my hands.
Even when you get your TX16S there is also NV14 that has to be tested, which has a completely different screen layout and in the future PL18, which has the same display as the NV14, but in landscape mode.

@piotrva
Copy link
Contributor Author

piotrva commented Jan 12, 2022

@gagarinlg I'd rather leave it for a separate PR - the issue is not the layout itself, but the way radio handles some action (like menu enter/exit). This is only different between colorlcd and B&W.

@wimalopaan
Copy link
Contributor

And what about loading checklist as "View Notes" ?

In my personal view I would prefer to see the "View Notes" with the previosly left state, because this might be important. But I do not see a need to complete the checklist at this time.

@piotrva
Copy link
Contributor Author

piotrva commented Jan 12, 2022

Updated as we discussed:

  • config value name changed
  • added padding field
  • when loaded as View Notes no check-boxes and closing using EXIT/RTN button
  • included screen in the commit comment

@piotrva piotrva mentioned this pull request Jan 12, 2022
@raphaelcoeffic raphaelcoeffic added this to the 2.7 milestone Jan 16, 2022
This is also crucial when it comes to analyse non-checkable items on a checklist, as in this way there is no need to load non-visible items from SD-card
…essive-checklist

# Conflicts:
#	companion/src/modeledit/setup.ui
#	radio/src/datastructs.h
#	radio/src/datastructs_private.h
#	radio/src/storage/yaml/yaml_datastructs_nv14.cpp
#	radio/src/storage/yaml/yaml_datastructs_t12.cpp
#	radio/src/storage/yaml/yaml_datastructs_t8.cpp
#	radio/src/storage/yaml/yaml_datastructs_tlite.cpp
#	radio/src/storage/yaml/yaml_datastructs_tpro.cpp
#	radio/src/storage/yaml/yaml_datastructs_tx12.cpp
#	radio/src/storage/yaml/yaml_datastructs_x10.cpp
#	radio/src/storage/yaml/yaml_datastructs_x12s.cpp
#	radio/src/storage/yaml/yaml_datastructs_x7.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9d.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9e.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9lite.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9lites.cpp
#	radio/src/storage/yaml/yaml_datastructs_xlite.cpp
#	radio/src/storage/yaml/yaml_datastructs_xlites.cpp
#	radio/src/storage/yaml/yaml_datastructs_zorro.cpp
@piotrva
Copy link
Contributor Author

piotrva commented Jan 31, 2022

@wimalopaan Would be grateful if you give your opinion about the feature now.
Finally I decided to use a single-character (as it greatly simplifies the code responsible for omitting non-checkable items).
So any line beginning with = character would be non-checkable (and would be skipped), while any other line would be checkable. The character indicating non-checkable lines might be easily changed in the source code if needed. I don't see reason to define this character in configuration of the radio.

It takes into consideration also the fact, that user might want to enable interactive checklist for previously prepared .txt file, so I decided to have checkable items by default.

@piotrva
Copy link
Contributor Author

piotrva commented Jan 31, 2022

@pfeerick thanks for merging #1395, so we are left with this one and #1425

@pfeerick
Copy link
Member

pfeerick commented Feb 5, 2022

Sorry for the delay, didn't see the notification for this. I'll trade you review of this for testing of #1548 since I know you have a TX12 also. 😁

@piotrva
Copy link
Contributor Author

piotrva commented Feb 6, 2022

Sorry for the delay, didn't see the notification for this. I'll trade you review of this for testing of #1548 since I know you have a TX12 also. 😁

@pfeerick Testing done, If you had something to test in future - feel free to tag me - not always having time to go through issues/PRs. And also starting a day before yesterday - I am an owner of Eachine TX16s too ;)

@piotrva
Copy link
Contributor Author

piotrva commented Feb 12, 2022

@wimalopaan - any thoughts on the status of this feature now?

@pfeerick
Copy link
Member

btw, I was only holding reviewing this PR as #1481 needs to go in for 2.6.1 and whichever PR goes in first will need the other rebased... but I think it needs more work for companion anyway so this can go in first. So I will review this one today, and possibly your other one as well and then can do a single update to the other PR.

@pfeerick
Copy link
Member

pfeerick commented Feb 16, 2022

Right, so finally playing with this on TX16S. Companion side of things seems fine. I've noticed a few... quirks... in behaviour ... probably because I'm using this the wrong way. ;)

I know you have it so that adding = as the first character inhibits the checkbox... but to my way of thinking it would be better to have it requiring a = as the first character. What was the benefit of doing it this way?

Blank links also need some catering for also, for colorlcd at least, as they seem to behave as if there is a checkbox still (consuming a ENTER) - it made me think at first that scrolling when checking boxes seems would break, but that was because blank lines would give no idea as to where the next 'check' was.

There is no way to get out of the checklist? This was a showstopper in conjunction with the prior - as it was nearly impossible to get out of the checklist. RTN/Long RTN to exit? I can use touch exit, but that won't non-touch radios.

Scrollbar in the model notes view is broken - although admittedly I think it acts a bit odd with 2.6.

i.e. this is the dummy checklist I'm using atm... basically a mock model notes that has had the checklist added... as you can see it would be much... neater... with use = for the four check items. That now makes me wonder as to whether a separate file would be better for checklist... something to think about in the future?

=FPV 250
=3S/4S Battery

Check Props
Check Battery
Check GPS
Check Failsafe

=Switches:
=SF\up  = Arm

=SD\up  = Self-Level
=SD-    = Horizon
=SD\dn  = Acro

=SC\up  = Level
=SC-    = Horizon
=SC\dn  = Acro

=SB\up  = None
=SB-    = Buzzer
=SB\dn  = GPS Rescue

@piotrva
Copy link
Contributor Author

piotrva commented Feb 18, 2022

I know you have it so that adding = as the first character inhibits the checkbox... but to my way of thinking it would be better to have it requiring a = as the first character. What was the benefit of doing it this way?

Ok, that is the point, my idea was (as I explained) that doing this in the way I presented allows users to use their "old" checklists with every item checkable without need to alter the text file.

Blank links also need some catering for also, for colorlcd at least, as they seem to behave as if there is a checkbox still (consuming a ENTER) - it made me think at first that scrolling when checking boxes seems would break, but that was because blank lines would give no idea as to where the next 'check' was.

Ok, Important thing to note and quite easy to detect - CR or LF ad a first char should always render non-checkable line.

There is no way to get out of the checklist? This was a showstopper in conjunction with the prior - as it was nearly impossible to get out of the checklist. RTN/Long RTN to exit? I can use touch exit, but that won't non-touch radios.

This was also discussed, that checklist should only be closed if completed. But also this is a point that there should be to cancel it when executed by mistake or in similar situations. I will think about it - maybe a confirmation window after hitting/holding [RTN] button? I haven't know about exit option from touch screen on colorlcd - will have to check it though.

Scrollbar in the model notes view is broken - although admittedly I think it acts a bit odd with 2.6.

Will have a look at it.

Probably in the beginning of the next week I will go through all the remarks. And for now I leave a little bit of time to discuss things mentioned earlier, like if lines should have checkbox or not by default.

@pfeerick
Copy link
Member

pfeerick commented Feb 22, 2022

Right, I'm slowly getting back up to speed again.

My own personal preference would be explicit checklist character, but I can also understand the other point of view, so I say go with it as it is unless there is further commentary on it. Or if you want I can throw it up on Discord for a quick poll.

I think being able to abort the checklist is a basic UX requirement (whether whether with [preferable] or without confirmation) - i.e. if I go into the model to view or change a setting without the craft powered, why do I need to do the checklist? And it gets tedious if I'm looking at a couple of models.

@pfeerick pfeerick removed their assignment Mar 18, 2022
@pfeerick pfeerick modified the milestones: 2.7, 2.8 Mar 28, 2022
# Conflicts:
#	radio/src/storage/yaml/yaml_datastructs_nv14.cpp
#	radio/src/storage/yaml/yaml_datastructs_t12.cpp
#	radio/src/storage/yaml/yaml_datastructs_t8.cpp
#	radio/src/storage/yaml/yaml_datastructs_tlite.cpp
#	radio/src/storage/yaml/yaml_datastructs_tpro.cpp
#	radio/src/storage/yaml/yaml_datastructs_tx12.cpp
#	radio/src/storage/yaml/yaml_datastructs_x10.cpp
#	radio/src/storage/yaml/yaml_datastructs_x12s.cpp
#	radio/src/storage/yaml/yaml_datastructs_x7.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9d.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9e.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9lite.cpp
#	radio/src/storage/yaml/yaml_datastructs_x9lites.cpp
#	radio/src/storage/yaml/yaml_datastructs_xlite.cpp
#	radio/src/storage/yaml/yaml_datastructs_xlites.cpp
#	radio/src/storage/yaml/yaml_datastructs_zorro.cpp
@piotrva
Copy link
Contributor Author

piotrva commented Jun 4, 2022

Right, I'm slowly getting back up to speed again.

I also had a small interrupt with the topic ;)

My own personal preference would be explicit checklist character, but I can also understand the other point of view, so I say go with it as it is unless there is further commentary on it. Or if you want I can throw it up on Discord for a quick poll.

If you could - please go with the poll on Discord - I will implement what will suite users better.

I think being able to abort the checklist is a basic UX requirement (whether whether with [preferable] or without confirmation) - i.e. if I go into the model to view or change a setting without the craft powered, why do I need to do the checklist? And it gets tedious if I'm looking at a couple of models.

This is the point I will work on now.

So, tu sum up, for nowe we have:

  • Discussion wherever checkable or non-checkable items should be marked with a special character at the beginning of the line
  • Implementing exiting the checklist, preferably with a confirmation window
  • Check the scrollbar

@gagarinlg
Copy link
Member

It would probably make sense when you rebase on the lvgl branch, as it will soon be merged into main.

@Eldenroot
Copy link
Contributor

Could you add some screenshots how it looks like right now?

@pfeerick
Copy link
Member

Not just at the moment, as this needs some work done to it before it will work (hence the mention of rebase earlier). However, I don't think even then it will look significantly different just at the moment.

@pfeerick pfeerick modified the milestones: 2.8, 2.9 Sep 9, 2022
@Eldenroot
Copy link
Contributor

@piotrva is there any way how to make some points in the list "bold" - you know, the very important things to check.

@pfeerick pfeerick modified the milestones: 2.9, 2.10 Apr 10, 2023
@gagarinlg gagarinlg self-assigned this May 5, 2023
@gagarinlg
Copy link
Member

superseeded by #3564

@gagarinlg gagarinlg closed this May 6, 2023
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.

6 participants