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

Model Labels Selector #1374

Merged
merged 15 commits into from
Sep 2, 2022
Merged

Model Labels Selector #1374

merged 15 commits into from
Sep 2, 2022

Conversation

dlktdr
Copy link
Collaborator

@dlktdr dlktdr commented Jan 7, 2022

This PR brought to you by @kevinkoenig, @raphaelcoeffic and @dlktdr.

Models now have a label associated with them instead of a Category. Each model can belong multiple labels.

Models are now stored in /MODELS/labels.yml

  • All models are scanned at startup. The YAML files are opened and the important information is read.
  • After the first scan a model won't be opened on boot to keep startup fast.
  • labels.yml keeps track of which files need to be re-scanned. Also keeps all labels added and their order. The file can't be hand-edited it's refreshed on every boot and when models or labels have changed - Edit: the only part of the file that can be edited is the Labels section, which keeps the order. Deleting the file should always result in a refresh is something goes wrong.
  • Keeps extra RF data of each model & checks if the receiver id is used. ELRS might have some issues with this warning. Need a way to distinguish ELRS vs CRSF. Only a warning. won't prevent any action.
  • Deleting a model moves the file into /models/deleted just in case of accidental delete. They cannot be left in /models as they would just be re-added on next boot

Potential issues I see,

  • Companion cannot write the file hash (The modified date & time and size) and properly fill labels.yml with all information, as soon as it's written to the SD the models will have a different modified time. This means that after a companion change the radio will re-scan every model and the first boot will be slow than normal.

GUI

  • Select labels on the left bar
  • Models on right
  • Sort order at bottom of the list
  • Long press on labels or models for extra options

To Do

  • Unplugging USB needs to cause a re-scan - I guess it actually was working!
  • Progress bar on rename a label as it can be slow if there are lots of labels
  • NV14 Portrait mode
  • Auto create a Favorites label if no labels found
  • Companion modifications
  • Fix Hardware keys in Listbox & Page buttons
  • Work PR#1467 into new selector
  • Refresh receiver ID on change in model_setup
  • Replace multiple labels selection with AND. Want e.g Planes AND Favorites to show not OR
  • Delete label causes a crash
  • Multiselect menu for label selection, used model_setup and model_select
  • Progress bar on rename/delete labels
  • Label name dialog buttons in wrong place
  • Re-work for modified labels.yml structure
  • Save Sort Order

Alright here we go all squashed, hopefully ready for some extra testing.

To do, if there is time before 2.8.. if not needs to be completed after

  • Flatten the Companion Tree Model to remove some more unused code.
  • Allow label re-order in Companion.

@elecpower
Copy link
Collaborator

@kevinkoenig and @dlktdr can you please add to your To Do list Companion changes

@pfeerick pfeerick added color Related generally to color LCD radios UX-UI Related to user experience (UX) or user interface (UI) behaviour labels Jan 9, 2022
@pfeerick pfeerick added this to the 2.7 milestone Jan 9, 2022
@pfeerick
Copy link
Member

I've not tried this PR yet, so my apologies if this has already been dealt with. Can you also consider how to make it so it is harder to accidentally switch models? i.e. the current model in 2.6 is tap model, menu shows, tap select model. #1467 is a really good step forward, but I think (and it was mentioned in another issue but I can't find it again now) it would be better to make it so double tap is the switch to model trigger. Thus also making the "press/tap and hold shows the menu" a consistent behavior also.

@dlktdr
Copy link
Collaborator Author

dlktdr commented Jan 24, 2022

I've not tried this PR yet, so my apologies if this has already been dealt with. Can you also consider how to make it so it is harder to accidentally switch models? i.e. the current model in 2.6 is tap model, menu shows, tap select model. #1467 is a really good step forward, but I think (and it was mentioned in another issue but I can't find it again now) it would be better to make it so double tap is the switch to model trigger. Thus also making the "press/tap and hold shows the menu" a consistent behavior also.

I have made some modifications to help with this. After some changes a short press on a model does nothing. A long press brings up the selection menu. It used to bring up the labels selector but is now in a sub menu. I feel like you really have to try now to change the model... although I still don't know why you would even try if your model is connected and flying :)

You can now also select multiple labels at a time. Still needs work on the hardware keys.

image

I will incorporate PR#1467 as I think this is a good idea too,

@pfeerick
Copy link
Member

Me also... but it happens, so I'm told.

That is perfect! :) Having the Create / New Model broken out of the menu into it's own button also alleviates one of the the other problems... the "accidentally created a new model" one - plus it really didn't make sense when you think of it - since creating a new model is not relevant in the context menu of another model. Looking forward to trying this out in a couple of days.

@dlktdr
Copy link
Collaborator Author

dlktdr commented Feb 10, 2022

So here is what I currently have for the Labels in Companion.
Been quite a few code changes and some learning so have not added to the pull yet.

image

I Still need to incorporate writing the labels.yml file, move, add, delete, rename. but this is the basic plan..

Feedback before I get too carried away?

@pfeerick
Copy link
Member

As far as I'm concerned, that looks perfect - simple and effective. Do you have it smart enough (yet?) to show when labels are relevant (i.e. colorlcd), but not when it's not (i.e. B&W)?

@elecpower
Copy link
Collaborator

Companion looks good to me.
Remember to add the handling of the labels.yml file in the functions Read and Write Models Settings; reading and writing etx file format; and board conversions.

@dlktdr
Copy link
Collaborator Author

dlktdr commented Feb 10, 2022

As far as I'm concerned, that looks perfect - simple and effective. Do you have it smart enough (yet?) to show when labels are relevant (i.e. colorlcd), but not when it's not (i.e. B&W)?

Yes that part does work.
image

@elecpower - Can read labels.yml. writing not done and conversions need to be verified. I'll post it soon and let you look at the code for some suggestions.. after I fix up a glaring botch. ;)

For conversion testing. Does anyone have a few backups .otx and .etx from various versions and radios I can test with? I'm limited on my backups I have from my 2019x9d+ and tx16s in OTX days. Would be helpful to verify for radio side too.

@pfeerick
Copy link
Member

Hey @dlktdr @kevinkoenig ... health and wellbeing check :) How are things progressing with this PR? I was hoping this would make 2.7 but it's looking like it might miss that, and be early 2.8?

@dlktdr
Copy link
Collaborator Author

dlktdr commented Mar 16, 2022

Hey @dlktdr @kevinkoenig ... health and wellbeing check :) How are things progressing with this PR? I was hoping this would make 2.7 but it's looking like it might miss that, and be early 2.8?

Health good. Free time... none. Been on a huge project for work since mid Dec, I'm so over it and would much rather work on this. One week left Mar 22, and I'm out if it's done or not :) then life goes back to normal (whatever that is). But yeah I think it's going to have to be pushed to 2.8.

I didn't make to much progress on companion above where I was before. Got stuck trying to decide how to modify the model/view framework to allow edit the labels. Proxy Model / custom view or what I leaned towards was a custom labels model that connects the main. Made some progress on this one but don't know if I love it.

I think maybe I'll go back to the radio and polish that off first as there wasn't too much else I needed here. At least it can be tested by users for some feedback. Maybe I'll make a separate PR for the companion changes. What do you think?

@pfeerick
Copy link
Member

Ah, that elusive creature, free time. No worries, 2.8 it will be... not like I can crack a whip and make you finish it tomorrow 😁

Yeah, splitting it into two parts will be fine... then the nightly testers will be able to poke and prod the radio side while the companion side is brought up to speed.

@pfeerick pfeerick modified the milestones: 2.7, 2.8 Mar 16, 2022
@jmxp69
Copy link

jmxp69 commented Mar 23, 2022

Do you still need sample .otx files to test conversions? I've got tons of them.

@dlktdr
Copy link
Collaborator Author

dlktdr commented Mar 27, 2022

Do you still need sample .otx files to test conversions? I've got tons of them.

Yeah that would be great. Are you on discord can PM them there?

Also looks like I set this to the side for too long.. Have to see how to incorporate templates now ;)

@pfeerick
Copy link
Member

pfeerick commented Mar 27, 2022

Also looks like I set this to the side for too long.. Have to see how to incorporate templates now ;)

Yup, that's the danger of stepping away here... resolving and managing the PR conflicts can be fun at times 😁

I don't think it will disturb the labels UI though, since it hooked into the "new model" path, as well as adding a context menu option to save as a template...

@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 2, 2022

I'm running the latest build on my tester TX16 (which doesn't have any model images on it so I'll have to load the profile from my daily driver on it and see how it performs. Without any model images, this is fast, and I haven't noticed any slowdown in speed.

That's good. Give it a try deleting the labels.yml and booting your radio too. This is where you might notice that slow down the first time (I only really noticed when I was lots of models (50+).

Bitmaps also only load 6 at a time, so still quick opening, but then slowed down the scrolling as new ones are read. Maybe let me know if this was a good idea.

My wishlist at the moment would be (if not practical/not sure how to implement at present, that is totally fine!) consists soley of "it would be nice to use the PAGE hard keys to change between label groups". Simple, right? 😆

Will have to think about how to implement this with multi-selectable items. Open to thoughts here too.

The other one 'issue' is possibly a misunderstanding of how things are intended to work, or just uncomfortable with change in workflow, or a bug.... I'll leave it up to you to decide 😁 So, I have Favourites (automatically created), 'test', 'abc' (creative, right?! Picasso has nothing on me!) and 'Unlabeled'. So I press on 'test' to view what's tagged with that label (ok, lets call it 'one' for simplcity). hm... ok, that has a tick next to it now. I want to see what's in 'abc' (rechristened as two)... now that has a tick next to it, and I don't see anything on the right at all now... eek "where did my models go!". Hm, ok, I need to press on 'test'/'one' before I can see what's in 'abc'/'two'. If the multi-ticks let me see multiple labels at once that would sort of be ok, but having to go back and untick to view the one I want to look at next just doesn't feel right. Plus I would have thought you had to press and hold or something to multi-select.

I can understand why this might be confusing yes. It is like @gagarinlg said more of a filter. So You can pick your Favorite & Drones. Mini & Planes. Broken & Planes ;) for instance @JimB40 brought this one up, maybe he can chime in on thoughts too. I had it before more like you were thinking, but then it's was no different than a category.

Maybe a message if there is nothing showing, or a title "Filter" on the checklist. I'm open to ideas here to as it might bring up some questions. Button on the blank section when there are no models (Clear check filter?). Or a checkbutton under the labels to deselect all?

And if you didn't at least get a little chuckle out of that rambling story, well fine, I'll be more serious next time :-P

edit: oh - do we consider the "new label" dialog pre-populating with the last label created a feature or a bug? I'm cool with either ;)

That would be a bug.. :) Duplicate was supposed to have all the previous labels. New should have just showed up under only under Unlabeled.

later edit: Companion build finally finished so tried that. Simple sync worked to and from radio was fine. Did some more labelling, seems ok. It would be nicer if add label prompted for the label name - maybe straight-away switch to rename mode if possible? Ok, that gripe could bet aimed generally at Companion, so ignore that one.

Perhaps a button for renaming labels also? I know you can 'just' two single click or F2 for rename, but some of us aren't that computer savvy 😇

Had that in there. I'll add it back. Thought this sees kind or redundant but your right, for the non-savvy 👍

@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 2, 2022

Hmm thinking about this some more, might be a bit more work but how about if you tap on the label text it checks only that label, clearing all the others.

The check box to have a square border added around it, like a classic GUI. If I tap in that box it will select multiple.

Edit: Hmm but then what to do about the keys...

@gagarinlg
Copy link
Member

As this is targeted for 2.8, how about getting it merged and let more people check it out and then decide how to continue.
I personally have no problem with this being in 2.8 like it is, minus the bugs, and add UI improvements in 2.9 or a 2.8.1 or even 2.8, if there is enough time left.

@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 2, 2022

Sounds good. I'll check out that new label bug, and can add other changes on top in new PR's

@pfeerick - I can't seem to get it to add a new model to anything other than un-labeled. Or also shows up when no labels are selected e.g. all models

Can you let me know the sequence you used. Could you open that new model under model_settings and see what shows for the labels there?

@Eldenroot
Copy link
Contributor

Good idea, would be nice to create some TODO list :)

@pfeerick
Copy link
Member

pfeerick commented Sep 2, 2022

Hm... Haven't tried with a new model yet, only with already esisting models. Will try that next. And yes, let's do any followup in fututure PRs so this doesn't get any conflicts. It's usable enough now without any major issues so far, and has time to improve before release :)

@pfeerick
Copy link
Member

pfeerick commented Sep 2, 2022

Oh, wait, maybe I didnt explain that clearly enough... The new label dialog - when you use it a multiple times it pre-fills with last created label name (as opposed to being blank). I was just checking if deliberate or a bug.

@pfeerick
Copy link
Member

pfeerick commented Sep 2, 2022

Hmm thinking about this some more, might be a bit more work but how about if you tap on the label text it checks only that label, clearing all the others.

The check box to have a square border added around it, like a classic GUI. If I tap in that box it will select multiple.

If that were possible, that would be perfect... As I see that as allowing for you to clearly indicate the intention to have more than one ticked vs just changing views. I did end up adding a few images, and it still loads way faster than the original implementation. And loading images "on demand" sounds perfect :)

@pfeerick pfeerick merged commit c869356 into EdgeTX:main Sep 2, 2022
@HThuren
Copy link
Contributor

HThuren commented Sep 3, 2022

@dlktdr and @pfeerick, we have an issue with
#define TR_NEW, #define TR_NEW_LABEL, #define TR_RENAME_LABEL, #define TR_DELETE_LABEL
in untranslated.h, since also in the specific language files, fx. da.h. Give error when translate and the #define differ.
Must be removed from untranslated.h

@HThuren
Copy link
Contributor

HThuren commented Sep 3, 2022

Same with
#define TR_ENTER_LABEL, #define TR_LABEL, #define TR_NONE, #define TR_CURRENT_MODEL

@HThuren
Copy link
Contributor

HThuren commented Sep 3, 2022

Now removed at @dlktdr model_select_2 branch, @dlktdr need to make a PR

@pfeerick
Copy link
Member

pfeerick commented Sep 3, 2022

That's not quite how it works - that branch has already been merged. I'll cherry-pick the commit into main. Thanks for doing that, was going to chase it up tomorrow.

@pfeerick
Copy link
Member

pfeerick commented Sep 3, 2022

btw, Cliff, John from rcmodelreviews had a request that there is a 'new label' button up the top next the new model if it fits/is possible. He's going do a video on this on this new model selector, is really excited about it :)

@HThuren
Copy link
Contributor

HThuren commented Sep 3, 2022

Don't we need a backward seup, ie read the old definitions and transfer to the new layout. None of mine models exits with this (good) improvement.

And there are a bug, when create new label, and then delete it again, Companion then crash.

@pfeerick
Copy link
Member

pfeerick commented Sep 3, 2022

Not sure what you mean by backwards setup... all your existing models should be automatically pulled in, and are in 'Unlabeled' until you assign them to labels. Or do you mean that old categories should be imported as labels?

@HThuren
Copy link
Contributor

HThuren commented Sep 3, 2022

None of my existing models are pulled in, not from radio (X10s express) nor stored settings file.
Just the models are fine, no need for the old categories.

@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 3, 2022

Same with #define TR_ENTER_LABEL, #define TR_LABEL, #define TR_NONE, #define TR_CURRENT_MODEL

Thanks, looks like they got added back in there during the rebase. 👍

And there are a bug, when create new label, and then delete it again, Companion then crash.

I can repeat that one. On it. 😆

btw, Cliff, John from rcmodelreviews had a request that there is a 'new label' button up the top

Yeah we can add a new label button that's easy 👍

None of my existing models are pulled in, not from radio (X10s express) nor stored settings file. Just the models are fine, no need for the old categories.

@HThuren In companion & radio? Can you post a zip of your RADIO and MODELS folder and upload here to test

@HThuren
Copy link
Contributor

HThuren commented Sep 3, 2022

In Companion, post zip and settings file
companion-windows-2.8.0.zip
Horus X10S-Express 18.zip

Hope that are what you need

@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 3, 2022

In Companion, post zip and settings file companion-windows-2.8.0.zip Horus X10S-Express 18.zip

Hope that are what you need

Yep that works & I see what you mean, will figure that out.

@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 3, 2022

Found those bugs. dlktdr@648176d

@pfeerick Can you review this change and cherry pick it? I haven't ever tried pushing to main & will let you look first

@elecpower
Copy link
Collaborator

Just started testing and so far:

  • opening an older version etx does not load any models (haven't tried read models and settings from radio on older version)
  • you can create duplicate labels and both will be ticked when used
  • copy and paste a model with a label that exists in the source file to the destination file where it does not does not cause the label list to be updated with the new label (if you save and reopen the etx file the list is correct)

@HThuren
Copy link
Contributor

HThuren commented Sep 3, 2022

I have tested reading from X10s express with older version, and noting appear in campanion.

rotorman pushed a commit that referenced this pull request Sep 3, 2022
@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 3, 2022

Just started testing and so far:

  • opening an older version etx does not load any models (haven't tried read models and settings from radio on older version)

I have fixed this and another issue here. Will get them added soon
dlktdr@648176d

  • you can create duplicate labels and both will be ticked when used

👍 Will fix

  • copy and paste a model with a label that exists in the source file to the destination file where it does not does not cause the label list to be updated with the new label (if you save and reopen the etx file the list is correct)

👍 Will fix

@dlktdr dlktdr mentioned this pull request Sep 3, 2022
@dlktdr
Copy link
Collaborator Author

dlktdr commented Sep 3, 2022

Ok found all those ones. Started a new PR #2266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color Related generally to color LCD radios UX-UI Related to user experience (UX) or user interface (UI) behaviour
Projects
None yet
8 participants