fix(color): Limit label lengths, improve lbl display, prevent inv. char (#2287)

* (cpn) Add bullets & length warning

* (fw) Show labels with bullet separators

* (fw) Check all running twice. Remove from here

* (fw) Correct buffer overflow when labels too long

* (fw) Rem bullets in model label string

* (fw, cpn) Labels length

cpn,fw Limit label length
cpn Check correct model on rename length
cpn Focus issues
cpn Add rename,  move up/down.. still has an issue with selection. TODO...
cpn Switch to utf8 encoding
cpn Rename button ;)

* (cpn) Move up down working

re-Fix Checkbox, partially isn't what I thought (not greyed)

* (fw) Add CSV string funcs

Limit length of model edit shorter in portrait mode

* (cpn) Correct Label option showing on B&W

* (fw) Radio filename set but no file exists don't create

* (cpn) Simplify wording

* (fw) Correct debug values

* (fw) Reduce truncation length

* (both) Don't allow special chars in label name

(cpn) Prevent move when at top and bottom
(fw) Remove multiple label length defines
(fw) Shorten truncate length even more

* (fw) Close model_select on new model

radio/src/dataconstants.h

@@ -38,7 +38,7 @@
 #endif
 
 #define LABELS_LENGTH 100 // Maximum length of the label string
-#define LABEL_LENGTH 20   // Maximum length of a single label
+#define LABEL_LENGTH 16
 
 #if defined(PCBHORUS) || defined(PCBNV14)
   #define MAX_MODELS                   60

radio/src/gui/colorlcd/model_select.cpp

@@ -383,7 +383,6 @@ void ModelsPageBody::selectModel(ModelCell *model)
   storageCheck(true);
 
   modelslist.setCurrentModel(model);
-  checkAll();
 
   // Exit to main view
   auto w = Layer::back();
@@ -496,8 +495,8 @@ class LabelDialog : public Dialog
       Dialog(parent, STR_ENTER_LABEL, rect_t{}),
       saveHandler(std::move(_saveHandler))
   {
-    strncpy(this->label, label, MAX_LABEL_SIZE);
-    this->label[MAX_LABEL_SIZE] = '\0';
+    strncpy(this->label, label, LABEL_LENGTH);
+    this->label[LABEL_LENGTH] = '\0';
 
     auto form = &content->form;
     form->padRow(lv_dpx(8));
@@ -505,7 +504,7 @@ class LabelDialog : public Dialog
     auto form_obj = form->getLvObj();
     lv_obj_set_style_flex_cross_place(form_obj, LV_FLEX_ALIGN_CENTER, 0);
 
-    new TextEdit(form, rect_t{}, label, MAX_LABEL_SIZE);
+    new TextEdit(form, rect_t{}, label, LABEL_LENGTH);
 
     auto box = new FormGroup(form, rect_t{});
     box->setFlexLayout(LV_FLEX_FLOW_ROW);
@@ -538,7 +537,7 @@ class LabelDialog : public Dialog
 
  protected:
   std::function<void(std::string label)> saveHandler;
-  char label[MAX_LABEL_SIZE + 1];
+  char label[LABEL_LENGTH + 1];
 };
 
 //-----------------------------------------------------------------------------
@@ -625,11 +624,9 @@ void ModelLabelsWindow::newModel()
     // On complete update the current cell's data
     modelslist.updateCurrentModelCell();
 
-    auto labels = getLabels();
-    lblselector->setNames(labels);
-    lblselector->setSelected(modelslabels.getLabels().size());
-    mdlselector->update(0);
-    setTitle();
+    // Close Window
+    auto w = Layer::back();
+    if (w) w->onCancel();
   });
 }
 
@@ -778,8 +775,8 @@ void ModelLabelsWindow::buildBody(FormWindow *window)
         menu->setTitle(selectedLabel);
         menu->addLine(STR_RENAME_LABEL, [=]() {
           auto oldLabel = labels[selected];
-          strncpy(tmpLabel, oldLabel.c_str(), MAX_LABEL_SIZE);
-          tmpLabel[MAX_LABEL_SIZE] = '\0';
+          strncpy(tmpLabel, oldLabel.c_str(), LABEL_LENGTH);
+          tmpLabel[LABEL_LENGTH] = '\0';
           new LabelDialog(this, tmpLabel, [=](std::string newLabel) {
             auto rndialog =
                 new ProgressDialog(this, STR_RENAME_LABEL, [=]() {});

radio/src/gui/colorlcd/model_select.h

@@ -30,8 +30,6 @@
 #include "storage/modelslist.h"
 #include "tabsgroup.h"
 
-constexpr int MAX_LABEL_SIZE = 20;
-
 class ModelButton;
 
 class ModelsPageBody : public FormWindow
@@ -78,7 +76,7 @@ class ModelLabelsWindow : public Page
 
  protected:
   ModelsSortBy sort = DEFAULT_MODEL_SORT;
-  char tmpLabel[MAX_LABEL_SIZE + 1] = "\0";
+  char tmpLabel[LABEL_LENGTH + 1] = "\0";
   ListBox *lblselector;
   ModelsPageBody *mdlselector;
   std::string currentLabel;

radio/src/gui/colorlcd/model_setup.cpp

@@ -127,7 +127,7 @@ void SubScreenButton::event_cb(lv_event_t* e)
   auto btn = (SubScreenButton*)lv_obj_get_user_data(obj);
   if (!btn) return;
 
-  lv_event_code_t code = lv_event_get_code(e);  
+  lv_event_code_t code = lv_event_get_code(e);
   if (code == LV_EVENT_DRAW_PART_BEGIN) {
 
     lv_obj_draw_part_dsc_t* dsc = lv_event_get_draw_part_dsc(e);
@@ -142,7 +142,7 @@ void SubScreenButton::event_cb(lv_event_t* e)
   } else if (code == LV_EVENT_DRAW_PART_END) {
 
     if (btn->text.empty()) return;
-    
+
     lv_obj_draw_part_dsc_t* dsc = lv_event_get_draw_part_dsc(e);
     if (dsc->part != LV_PART_MAIN) return;
 
@@ -243,7 +243,7 @@ void ModelSetupPage::build(FormWindow * window)
   new StaticText(line, rect_t{}, STR_LABELS, 0, COLOR_THEME_PRIMARY1);
   auto curmod = modelslist.getCurrentModel();
   labelTextButton =
-    new TextButton(line, rect_t{}, modelslabels.getLabelString(curmod ,STR_UNLABELEDMODEL), [=] () {
+    new TextButton(line, rect_t{}, modelslabels.getBulletLabelString(curmod ,STR_UNLABELEDMODEL), [=] () {
        Menu *menu = new Menu(window, true);
        menu->setTitle(STR_LABELS);
        for (auto &label: modelslabels.getLabels()) {
@@ -253,8 +253,9 @@ void ModelSetupPage::build(FormWindow * window)
                modelslabels.addLabelToModel(label, curmod);
              else
                modelslabels.removeLabelFromModel(label, curmod);
-             labelTextButton->setText(modelslabels.getLabelString(curmod,STR_UNLABELEDMODEL));
-             strcpy(g_model.header.labels, modelslabels.getLabelString(curmod,STR_UNLABELEDMODEL).c_str());
+             labelTextButton->setText(modelslabels.getBulletLabelString(curmod,STR_UNLABELEDMODEL));
+             strncpy(g_model.header.labels, ModelMap::toCSV(modelslabels.getLabelsByModel(curmod)).c_str(),sizeof(g_model.header.labels));
+             g_model.header.labels[sizeof(g_model.header.labels)-1] = '\0';
              SET_DIRTY();
            }, [=] () {
              return modelslabels.isLabelSelected(label, curmod);
@@ -285,7 +286,7 @@ void ModelSetupPage::build(FormWindow * window)
   form->setFlexLayout(LV_FLEX_FLOW_ROW_WRAP, lv_dpx(8));
   lv_obj_set_style_flex_main_place(form->getLvObj(), LV_FLEX_ALIGN_SPACE_EVENLY, 0);
   form->padAll(lv_dpx(8));
-  
+
   Window* btn = new IntmoduleButton(form);
   lv_obj_set_style_min_width(btn->getLvObj(), LV_DPI_DEF, 0);
 
@@ -342,11 +343,11 @@ TimerBtnMatrix::TimerBtnMatrix(Window* parent, const rect_t& r) :
   setText(1, TR_TIMER "2");
   setText(2, TR_TIMER "3");
   update();
-  
+
   lv_btnmatrix_set_btn_width(lvobj, 3, 2);
   lv_obj_set_width(lvobj, lv_pct(100));
   lv_obj_set_height(lvobj, LV_DPI_DEF / 2);
-  
+
   lv_obj_set_style_bg_opa(lvobj, LV_OPA_0, 0);
   lv_obj_set_style_pad_all(lvobj, lv_dpx(8), 0);
 

radio/src/storage/modelslist.cpp

@@ -330,10 +330,14 @@ LabelsVector ModelMap::getLabels()
 
 int ModelMap::addLabel(std::string lbl)
 {
-  if (lbl.size() == 0) return -1;
   if (lbl == STR_UNLABELEDMODEL) return -1;
 
-  // Add a new label if if doesn't already exist in the list
+  // Limit maximum label length, TODO... Truncate UTF8 Properly
+  lbl = lbl.substr(0, LABEL_LENGTH);
+  removeYAMLChars(lbl);
+  if (lbl.size() == 0) return -1;
+
+  // Add a new label if it doesn't already exist in the list
   // Returns the index to the label
   int ind = getIndexByLabel(lbl);
   if (ind < 0) {
@@ -419,8 +423,7 @@ std::string ModelMap::toCSV(const LabelsVector &labels)
       csv += ',';
     }
     // Escape out commas
-    replace_all(lbl, "/", "//");
-    replace_all(lbl, ",", "/c");
+    escapeCSV(lbl);
     csv += lbl;
     comma = true;
   }
@@ -440,14 +443,36 @@ LabelsVector ModelMap::fromCSV(const char *str)
   std::istringstream f(str);
   std::string lbl;
   while (std::getline(f, lbl, ',')) {
-    // Un-escape commas
-    replace_all(lbl, "/c", ",");
-    replace_all(lbl, "//", "/");
+    unEscapeCSV(lbl);
     lbls.push_back(lbl);
   }
   return lbls;
 }
 
+// TODO - Fix me, ideally there should be no limitations
+void ModelMap::escapeCSV(std::string &str)
+{
+  replace_all(str, "/", "//");
+  replace_all(str, ",", "/c");
+}
+
+// TODO - Fix me, ideally there should be no limitations
+void ModelMap::unEscapeCSV(std::string &str)
+{
+  replace_all(str, "//", "/");
+  replace_all(str, "/c", ",");
+}
+
+// TODO - Fix me, ideally there should be no limitations
+void ModelMap::removeYAMLChars(std::string &str)
+{
+  replace_all(str, "\\", "");
+  replace_all(str, "\"", "");
+  replace_all(str, ":", "");
+  replace_all(str, "\'", "");
+  replace_all(str, "-", "");
+}
+
 /**
  * @brief Replace all occurances of <from> to <to> in <str>
  *
@@ -585,13 +610,17 @@ bool ModelMap::moveLabelTo(unsigned curind, unsigned newind)
  * @return false success
  */
 
-bool ModelMap::renameLabel(
-    const std::string &from, const std::string &to,
+bool ModelMap::renameLabel(const std::string &from, std::string to,
     std::function<void(const char *file, int progress)> progress)
 {
+  if (from == "") return true;
   DEBUG_TIMER_START(debugTimerYamlScan);
 
-  if (from == "") return true;
+  // Limit max label name. TODO: Warn user they entered too long of a string
+  to = to.substr(0, LABEL_LENGTH);
+  removeYAMLChars(to);
+  if(to.size() == 0) return true;
+
 
   ModelData *modeldata = (ModelData *)malloc(sizeof(ModelData));
   if (!modeldata) {
@@ -604,6 +633,23 @@ bool ModelMap::renameLabel(
 
   bool fault = false;
   ModelsVector mods = getModelsByLabel(from);  // Find all models to be renamed
+
+  // Scan all these models first, recombine their labels to a csv,
+  // make sure re-size is going to fit before starting. Otherwise new partial
+  // labels would be created on next scan.
+  for(const auto &model: mods) {
+    int curlen = toCSV(getLabelsByModel(model)).size();
+    std::string csvto = to;
+    escapeCSV(csvto);
+    std::string csvfrom = from;
+    escapeCSV(csvfrom);
+    if(curlen + csvto.size() - csvfrom.size() > LABELS_LENGTH - 1) {
+      TRACE("Labels: Rename Error! Labels too long on %s", model->modelName);
+      if (progress != nullptr) progress("", 100); // Kill progress dialog
+      return true;
+    }
+  }
+
   int i = 0;
   for (const auto &modcell : mods) {
     if (progress != nullptr) {
@@ -613,25 +659,9 @@ bool ModelMap::renameLabel(
     readModelYaml(modcell->modelFilename, (uint8_t *)modeldata,
                   sizeof(ModelData));
 
-    // Make sure there is room to rename, use toCSV to capture escape chars
-    LabelsVector tmpvect;
-    tmpvect.push_back(to);
-    int newlen = ModelMap::toCSV(tmpvect).size();
-    tmpvect.clear();
-    tmpvect.push_back(from);
-    int oldlen = ModelMap::toCSV(tmpvect).size();
-
     // Separate Curent CSV
     LabelsVector lbls = ModelMap::fromCSV(modeldata->header.labels);
 
-    int nlen = lbls.size() + newlen - oldlen;
-    if (nlen > LABELS_LENGTH - 1) {
-      fault = true;
-      TRACE("Labels: Rename Error! Labels too long on %s - %s",
-            modeldata->header.name, modcell->modelFilename);
-      continue;
-    }
-
     // Replace from->to strings
     for (auto &lbl : lbls) {
       if (lbl == from) lbl = to;
@@ -691,19 +721,26 @@ bool ModelMap::renameLabel(
 }
 
 /**
- * @brief Returns a comma separated list of the labels, used in model_setup
+ * @brief Returns a bullet separated list of the labels, used in model_setup
  *
  * @param curmod Model Cell
  * @param noresults String to return when no labels found
  * @return std::string of all Labels, if no results return
  */
 
-std::string ModelMap::getLabelString(ModelCell *curmod, const char *noresults)
+std::string ModelMap::getBulletLabelString(ModelCell *curmod, const char *noresults)
 {
   std::string lbls = ModelMap::toCSV(getLabelsByModel(curmod));
+  replace_all(lbls, ",", "\u2022");
+  unEscapeCSV(lbls);
+
   if(lbls.size() == 0) {
     return noresults;
   }
+  if(lbls.size() > LABEL_TRUNCATE_LENGTH) {
+    lbls = lbls.substr(0, LABEL_TRUNCATE_LENGTH);
+    lbls += "...";
+  }
   return lbls;
 }
 
@@ -745,7 +782,7 @@ bool ModelMap::updateModelFile(ModelCell *cell)
 {
   // Update memory copy if on current model
   if (cell == modelslist.getCurrentModel()) {
-    strncpy(g_model.header.labels, getLabelString(cell).c_str(),
+    strncpy(g_model.header.labels, ModelMap::toCSV(getLabelsByModel(cell)).c_str(),
             LABELS_LENGTH - 1);
     g_model.header.labels[LABELS_LENGTH - 1] = '\0';
     storageDirty(EE_MODEL);
@@ -761,7 +798,7 @@ bool ModelMap::updateModelFile(ModelCell *cell)
   bool fault = false;
   readModelYaml(cell->modelFilename, (uint8_t *)modeldata, sizeof(ModelData));
 
-  strncpy(modeldata->header.labels, getLabelString(cell).c_str(),
+  strncpy(modeldata->header.labels, ModelMap::toCSV(getLabelsByModel(cell)).c_str(),
           LABELS_LENGTH - 1);
   modeldata->header.labels[LABELS_LENGTH - 1] = '\0';
 
@@ -1087,6 +1124,9 @@ bool ModelsList::load(Format fmt)
     TRACE("ERROR no Current Model Found");
     if (modelslist.size()) {
       modelslist.setCurrentModel(modelslist.at(0));
+      strncpy(g_eeGeneral.currModelFilename, modelslist.at(0)->modelFilename,
+              sizeof(g_eeGeneral.currModelFilename));
+      g_eeGeneral.currModelFilename[sizeof(g_eeGeneral.currModelFilename) - 1] = '\0';
       TRACE("  - Set current model to first available");
     } else {
       TRACE("  - No Models Found, making a new one");
@@ -1127,7 +1167,7 @@ const char *ModelsList::save(LabelsVector newOrder)
   if(newOrder.empty())
     newOrder = modelslabels.getLabels();
   for (auto &lbl : newOrder) {
-    f_printf(&file, "  %s:\r\n", lbl.c_str());
+    f_printf(&file, "  \"%s\":\r\n", lbl.c_str());
     if (modelslabels.isLabelFiltered(lbl))
       f_printf(&file, "    selected: true\r\n", lbl.c_str());
   }

radio/src/storage/modelslist.h

@@ -48,6 +48,12 @@
 
 #define DEFAULT_MODEL_SORT NAME_ASC
 
+#if LCD_W > LCD_H // Landscape
+#define LABEL_TRUNCATE_LENGTH 21
+#else
+#define LABEL_TRUNCATE_LENGTH 16
+#endif
+
 struct ModelData;
 struct ModuleData;
 
@@ -134,8 +140,7 @@ class ModelMap : protected std::multimap<uint16_t, ModelCell *>
       const std::string &,
       std::function<void(const char *file, int progress)> progress = nullptr);
   bool moveLabelTo(unsigned current, unsigned newind);
-  bool renameLabel(
-      const std::string &from, const std::string &to,
+  bool renameLabel(const std::string &from, std::string to,
       std::function<void(const char *file, int progress)> progress = nullptr);
   std::string getCurrentLabel() { return currentlabel; };
   void setCurrentLabel(const std::string &lbl)
@@ -143,7 +148,7 @@ class ModelMap : protected std::multimap<uint16_t, ModelCell *>
     currentlabel = lbl;
     setDirty();
   }
-  std::string getLabelString(ModelCell *, const char *noresults = "");
+  std::string getBulletLabelString(ModelCell *, const char *noresults = "");
   void setDirty(bool save = false);
   bool isDirty() { return _isDirty; }
 
@@ -161,6 +166,9 @@ class ModelMap : protected std::multimap<uint16_t, ModelCell *>
 
   static std::string toCSV(const LabelsVector &labels);
   static LabelsVector fromCSV(const char *str);
+  static void escapeCSV(std::string &str);
+  static void unEscapeCSV(std::string &str);
+  static void removeYAMLChars(std::string &str);
   static void replace_all(std::string &str,
                           const std::string &from,
                           const std::string &to);

radio/src/storage/yaml/yaml_labelslist.cpp

@@ -49,8 +49,8 @@ struct labelslist_iter
     bool        modeldatavalid; // Used to determine if reading yaml values is necessary
     uint8_t     level;
     uint8_t     section;
-    char        current_attr[LABEL_LENGTH+1]; // set after find_node()
-    char        current_label[LABEL_LENGTH+1]; // set after find_node()
+    char        current_attr[LABELS_LENGTH+1]; // set after find_node()
+    char        current_label[LABELS_LENGTH+1]; // set after find_node()
 };
 
 static labelslist_iter __labelslist_iter_inst;
@@ -130,9 +130,9 @@ static bool find_node(void* ctx, char* buf, uint8_t len)
       bool found=false;
       for(auto &filehash : modelslist.fileHashInfo) {
         if(filehash.name == mi->current_attr) {
-          TRACE_LABELS_YAML("  Model %s has a real file, creating a modelcell", filehash.name);
+          TRACE_LABELS_YAML("  Model %s has a real file, creating a modelcell", mi->current_attr);
           if(filehash.celladded) {
-            TRACE_LABELS_YAML("    Duplicate found labels.yml model cell %s already added", filehash.name);
+            TRACE_LABELS_YAML("    Duplicate found labels.yml model cell %s already added", mi->current_attr);
             break;
           }
           ModelCell *model = new ModelCell(mi->current_attr);
@@ -158,8 +158,8 @@ static bool find_node(void* ctx, char* buf, uint8_t len)
     if(mi->level == 1 && mi->section == labelslist_iter::SEC_Labels)  {
       TRACE_LABELS_YAML("Label Found -- %s", mi->current_attr);
       modelslabels.addLabel(mi->current_attr);
-      strncpy(mi->current_label,mi->current_attr, LABEL_LENGTH);
-      mi->current_label[LABEL_LENGTH] = '\0';
+      strncpy(mi->current_label,mi->current_attr, LABELS_LENGTH);
+      mi->current_label[LABELS_LENGTH] = '\0';
     }
 
     return true;
@@ -167,7 +167,11 @@ static bool find_node(void* ctx, char* buf, uint8_t len)
 
 static void set_attr(void* ctx, char* buf, uint8_t len)
 {
-  char value[40];
+  char value[LABELS_LENGTH + 1];
+  if(len > LABELS_LENGTH) {
+    TRACE("ERROR: YAML too long for buffer");
+    return;
+  }
   memcpy(value, buf, len);
   value[len] = '\0';
 
@@ -256,7 +260,7 @@ static void set_attr(void* ctx, char* buf, uint8_t len)
 
   // Sort Order
   } else if (mi->level == 0 && mi->section == labelslist_iter::SEC_Sort)  {
-    TRACE_LABELS_YAML(" Sort Order Found -- %s", mi->current_attr);
+    TRACE_LABELS_YAML(" Sort Order Found -- %s", value);
     modelslabels.setSortOrder((ModelsSortBy)strtol(value,NULL,10));
   }
 }

radio/src/storage/yaml/yaml_parser.h

@@ -24,7 +24,7 @@
 
 #include <stdint.h>
 
-#define MAX_STR 40
+#define MAX_STR 100
 #define MAX_DEPTH 16 // 12 real + 4 virtual
 
 struct YamlParserCalls
@@ -78,7 +78,7 @@ class YamlParser
     // tree iterator state
     const YamlParserCalls* calls;
     void*                  ctx;
-    
+
     // Reset parser state for next line
     void reset();
 
@@ -97,7 +97,7 @@ public:
     YamlParser();
 
     void init(const YamlParserCalls* parser_calls, void* parser_ctx);
-    
+
     YamlResult parse(const char* buffer, unsigned int size);
 
     void set_eof() { eof = true; }