refactor(color): classes used to manage models and labels, improve duplication UX

[philmoz]

Refactor:

• Rework the classes used to manage models and labels to simplify the interface. The interface is reduced from 3 classes (ModelCell, ModelMap and ModelsList) to 2 by removing the ModelMap class and moving the functionality to the other two. The primary interface object has been renamed modelCellManager to make to code more readable.
• Use storageDirty to mark the labels file for update instead of saving immediately.
• Remove the delete / rebuild of the model management class objects when labels are renamed or deleted. Keeps the ModelCell pointers valid so the manage models page does not need to be rebuilt.

Enhancements:

• prevent invalid characters from being entered with the keyboard when creating labels (instead of deleting after entering the label).
• when a model is duplicated a unique name is set on the new model based on the current name and focus is set to the new model button in the manage models UI.

[pfeerick]

• when a model is duplicated revert the name of the new model to the default name (MODELxx) and set focus to the new model button in the manage models UI.

This is because it would be harder to figure out what numerical number to add to the end to differentiate? As I don't see having to re-enter "Skyhunter" a second time if I duplicate the model as an enhancement (let alone anything more complex naming). If so, I get it, but it certainly isn't an enhancement.

[philmoz]

• when a model is duplicated revert the name of the new model to the default name (MODELxx) and set focus to the new model button in the manage models UI.

This is because it would be harder to figure out what numerical number to add to the end to differentiate? As I don't see having to re-enter "Skyhunter" a second time if I duplicate the model as an enhancement (let alone anything more complex naming). If so, I get it, but it certainly isn't an enhancement.

Well I thought it would be better than having multiple models all with the same name; but yes it would be better to try and append a unique number to the current name.

If there is no space for a unique number would it be better to fall back to the default name or truncate the current name to make room?

[pfeerick]

Well I thought it would be better than having multiple models all with the same name;

I am in full agreement on that!

but yes it would be better to try and append a unique number to the current name.

If there is no space for a unique number would it be better to fall back to the default name or truncate the current name to make room?

The easy option is to fall back, but truncation preserves as much of the original name as possible. It may even be worth considering splitting that behaviour out of this PR, as ideally you want bw and colorlcd to behave the same way, so may require changes there to keep from introducing diverging behaviours (in a PR ostensibly focused on colorlcd refactor)?

[philmoz]

The easy option is to fall back, but truncation preserves as much of the original name as possible. It may even be worth considering splitting that behaviour out of this PR, as ideally you want bw and colorlcd to behave the same way, so may require changes there to keep from introducing diverging behaviours (in a PR ostensibly focused on colorlcd refactor)?

The issue with B&W is the model duplication is done by just physically copying the model yaml file.
The rename is possible now with color radios because of the ability to rewrite the model file without mangling the active model data. This isn't an option for B&W at the moment.

[philmoz]

Updated to try and generate a unique name from the current name by appending a number as "(x)" to the current name.
If the current name is too long it is truncated and if a unique name can't be created it falls back to the default.

[pfeerick]

Couple of things:

• renameLabel() doesn't have any input validation any more... so among other things you can rename a label using prohibited characters, such as "... might want to review what other checksfrom ModelsList::renameLabel that are still relevant here
• shouldn't textedit.cpp:79 be using <= ?? - it is dropping ~ even though though it it's not in the excluded character list per labelExcludedChars
• modelslist.cpp#L1213 doesn't seem to be used for anything (also on next line)?
• modelslist.cpp#L333 return value from is being swallowed - don't you want to show an error or something if that fails?

Claude is moaning about a off by one error, which will occur if someone is daft enough to duplicate a model with 12 character name more than 9 times... but it is still a off by one error regardless 🤷

modelslist.cpp:273.
LEN_MODEL_NAME is 15 on colorlcd, and the buffer s is 16 bytes. For a name of exactly 12 characters, truncate is false. From the 10th duplicate on (i >= 10), "(10)" is written at s+12, which puts its NUL at index 16 (one past the buffer). The following strAppend(modelName, s) has no length bound, so it overflows modelName[16] as well.

Fix: decide truncation per iteration, using LEN_MODEL_NAME - (i >= 10 ? 4 : 3). Also pass LEN_MODEL_NAME to strAppend.
Names of 13–15 characters lose 3 characters even when 1 would do. That is cosmetic.
The setDefaultName() fallback isn't checked for uniqueness. The name check is also case-sensitive, while sorting is case-insensitive.

Added some more unit tests... they should fail until some of those regressions / edge cases are fixed. Let me know if any were over-eager or wrong. Only two are testing against new behaviours - four of the failures would have passed on the prior API - RejectedLabelDoesNotMakeModelLabelled and UniqueNameTwelveCharNameTenthDuplicate are the new edge case ones.

[philmoz]

@pfeerick Are the 'RenameLabelRejectsYamlUnsafeCharacters' and 'RenameLabelTruncatesToLabelLength' tests really needed?
Both of these cases are handled in the UI by the keyboard entry for the label name (length limit and invalid character rejection).

[philmoz]

@pfeerick The issues should be fixed. I've also fixed a number of issues with the maximum length of labels for a model in both companion and firmware.