Dont allow blank patch names or categories

[baconpaul]

Prompt a user error in this case rather than making blank named files
Fixes the error reported in #282 by not allowing the user to enter that
code path.

Eyeball or test appreciated. Thanks!

[jarkkojs]

What about a tab character or something even weirder (from the ASCII range)? I think you should validate that for a patch

• The only allowed non-displayable character is a space bar.
• Must have at least one displayable character.
• Is at least one character length.

I think these are fair assumptions for a patch name.

[jarkkojs]

e.g.

namespace Surge
{

namespace Utility
{

bool validatePatchName(const std::string& patchName)
{
   bool visible = false;

   // No need to validate size separately as an empty string won't have visible characters.
   for (const char &c : patchName)
      if (std::isalnum(c) || std::ispunct(c))
         visible = true;
      else if (c != ' ')
         return false;

   return visible;
}

}

}

[jarkkojs]

I think doing this C-style instead of string class magic functions documents actually best how a patch name is validated and probably performs best too.

[baconpaul]

Yeah that’s a better idea. I will adjust thusly

[jarkkojs]

Personal preference is std::string() + but not big issue for me.

[baconpaul]

Agree Never use it in a loop. For a one off concat it you don’t get the n^2 problem which ostringstream obviates so I think it is fine here.

[jarkkojs]

validatePatchName() could be called in both of these call sites.

I would also use it when we scanning all the patches in SurgeStorage, but that is up to you. I would just skip the patches that have invalid filenames, possibly giving one promptError() after the scan that reports the number of patches that were skipped because of this reason. Allowing to load such patches brings it own set of corner cases to the plugins run-time state, as you can probably understand.

[baconpaul]

Right let me add the function here and call it so we can reuse it but not reuse it yet. This is really a ui thing for not saving bad ones

On and the reason I only checked space is the ui doesn’t let you make a tab. But the is alpha church is way better. Thanks as always!

[baconpaul]

Thanks as always @jsakkine - good comments. I’ll fix em and merge over the weekend

[jarkkojs]

@baconpaul, glad to help :)

[baconpaul]

So @jsakkine I really I want to make a function with name Surge::Storage::isValidPatchOrCategoryName since SurgeStorage is the representation of a patch used in all parts of the codebase.

So do I

1. Put this namespace at the bottom of SurgeStorage.h
   • pros - it is code all in the same place
   • cons - it is just a namespace at the end
2. Put this namespace in a new header
   • pros - namespace stuff is distinct
   • cons - functions on storage are spread all around
3. Don't namespace this and make it a member of SurgeStorage
   • pros - the most like current code
   • cons - that's probably not true in the future and the future starts today

Your opinion welcome. I'm going to do 1 now but not force it until I hear from you

[jarkkojs]

I would name the function as isValidEntityName() and put it to Surge::Storage namespace. I would also move the class to that namespace and rename it as Storage. Since the function is in Storage namespace, it is obvious what is meant by entity.

The naming that you propose would ignore wavetables. It should be isValidPatchOrPatchCategoryOrWavetableOrWavetableCategoryName() to be exact :)

Becomes a largeish change, since also Patch and PatchCategory move to that name space, but I think it is good idea do things right at once.

[baconpaul]

Ha ha! IsValidThingWhichWeCanUseAsAStringForManyPurposesInSurge

I will do IsValidStorageName

I won't move the SurgeStorage class in this PR, but agree we should do it shortly.

[jarkkojs]

Why not just isValidName()? It is anyway in Storage namespace.

[baconpaul]

Yeah OK. It feels weird but not enough that I can push back so I'll pick that!

[baconpaul]

Thank you automated builds! Checking windows now.