Suggestion: fix sorting of patches in submenus to be A->Z?

[esaruoho]

the patch loading submenus are not A-to-Z, instead whatever goes wherever. Surely something like that could be fixed, or what do you think @inigokennedy ?

[baconpaul]

They are just in whatever order readdir returns. A sort is a good idea and easy enough.

[baconpaul]

@jsakkine you were looking for a way to get into the ui. This one is pretty easy. There’s a vector ctge assembled in CPatchBrowser.cpp in the onmousedown. Apply the right flavor of std::sort to it before the subgroup iteration and this should be fixed.

[baconpaul]

Or I can do it in the new year. But just thought this may be an easier approach to Mac dev and test than the other harder one with parameter invalidation

[baconpaul]

Also looking at that screenshot we may want to adjust the split to submenu threshold

[baconpaul]

Oh and don’t forget to remap indices. Alternately you could sort the entire structure when it is loaded in surgestorage. That way you can sort categories too. But then you need to adjust when you save one as a user. Not sure which I would do - would want to stare at code a minute more

[jarkkojs]

Working on this now. Yeah, sorry had a busy weekend. Promised a bit too much when it comes to time. Nothing particularly hard in parameter validation. It is just that for yet unknown reason I cannot access any of the Surge's parameters from Ableton.

[jarkkojs]

Ugh, did not understand how deep in the code category array indexes are used as global identifiers, so yeah, doing something simpler while still trying to also clean up thinks just a bit.

[jarkkojs]

PR done. Next time I will do something to this will be after NYE.

[baconpaul]

https://github.com/kurasu/surge/pull/187#issuecomment-451442633 let me just link that here since it contains a design idea we should probably have over here on the issue rather than the PR.

[jarkkojs]

Thanks for linking that! Just starting work on this.

[esaruoho]

fingers crossed!

[jarkkojs]

@baconpaul, just detailing a bit #183 to check that I've not missed something.

To the class SurgeStorage we add:

std::vector<int> categoryOrdering;
std::vector<int> patchOrdering;

I think these are more descriptive names than patch_traversal_map.

These must be generated in the tail of SurgeStorage::refresh_patchlist() . Is it too intrusive to rename this function as refreshPatchList()?

For categories the correct index is mapped here in CPatchBrowser::onMouseDown():

   for (int c = 0; c < storage->patch_category.size(); c++)
   {
      int c2 = storage->categoryOrdering[c];

For patches the index is mapped in this sub-loop of the former:

         for (int p = 0; p < storage->patch_list.size(); p++)
         {
            int p2 = storage->patchOrdering[p];
            if (storage->patch_list[p2].category == c)
            {
               ctge.push_back(p2);
            }
         }

I think these changes should guarantee that:

1. Orderings are re-generated whatever situation patches are updated.
2. Orderings are accounted whenever the menu is built.

I though to check the design before sending PRs.

[baconpaul]

Your name is better than patch_traversal_map tho

I would not rename refresh_patchlist at this point, since it will make the textual diff bigger than the semantic diff

I would probably not use p and p2; rather I’d use names like “patchIndex” and “patchSortedIndex”

Also it’s weird to iterate over patch list size and then get from patch ordering. Why wouldn’’t

for (int p = 0; p < storage->patch_list.size(); p++)
         {
            int p2 = storage->patchOrdering[p];

Just be

  For ( auto p : storage->patchOrdering )

Instead?

But looks like the right path!

[jarkkojs]

OK, cool.

Then what about splitting a category into multiple submenus? I remember that from earlier review. That'd probably be anyway a commit of its own so I'll go ahead and write the first patch.

[baconpaul]

There’s code there that does that already. I just think the constant is set wrong. But yeah two steps is fine.

[jarkkojs]

Is this how it should look like (sort-patch-list branch in my GIT):

[jarkkojs]

I've chosen to prefix everything with std:: because generally it is considered an anti-pattern to import a whole namespace in a header file (e.g. globals.h). And using it actually makes the code more readable than not using it (short prefix and functions like std::iota() would look cryptic without that prefix).

[baconpaul]

That looks great!

Wonder if you should apply the same to wavetables while you are in there?

And yeah I use std:: everywhere and also looked sideways a little at the use namespace std in globals.h. Fine with me.

[baconpaul]

Oh and you are case insensitive sorting right? Polysynth has all sorts of case conventions on the patches.

[jarkkojs]

I can show my lambda for patches:

   auto patchCompare =
      [this](const int &i1, const int &i2) -> bool
      {
         return _stricmp(patch_list[i1].name.c_str(),
                         patch_list[i2].name.c_str()) < 0;
      };

similar for categories.

[jarkkojs]

And yes, I can do the same for WT's.

[baconpaul]

Oh and in your tests, perhaps save a patch and see if it pops up in the right place. And also remember to save a patch with a factory category name (which now works; and should give you that category both in the first section and third section).

Thanks for doing this!

[jarkkojs]

Tried that now without my commit applied (i.e. master branch) with the latest changes from mainline. I saved a patch named as Jarkko's Test with the category Pad.

The result is that I can see the patch in a Pad category in user patches group but I cannot see it in factory patches group. Are you sure that this should work?

[baconpaul]

I just tried with my master and yes I see Pad in both places.

Xcode cleans "poorly". Generally when I switch branches I do a ./build-osx.sh --clean-all. Maybe you had a remnant of your branch in your build assets? But it is definitely the case that user patch categories matching factory patch categories show up twice.

[baconpaul]

Without looking at your code, maybe what's happening is since you sort on name, two categories with the same name are ambiguous. Perhaps patch_category needs an id which gets incremented when created and you check if that's distinct.

And your patch sort lambda - does that need to compare categories before names? Or do you apply that sort on a per-category basis?

Again just thinking out loud.

[jarkkojs]

OK, I might gotten this wrong what you said earlier. I see the Pad category twice exactly as in your screenshot with and without my commit. But I cannot find Jarkko's Test in both
Pad-menus. Nothing to do with my commit per se.

[baconpaul]

Oh that’s right. It will only appear in user
If you take a factory preset and store it without changing name same category same name should appear twice

If you rename it the new one only appears once

[jarkkojs]

OK, then it works as expected and my commit does not break anything :) I'll do the WT-part and send a PR.

[jarkkojs]

Proof:

[jarkkojs]

And this:

I'll prepare a PR now.

[baconpaul]

Yay great! I’m out and about today but I will look at the pr in detail either tonight nyc time or tomorrow morning. Thank you

[jarkkojs]

@esaruoho, do you have capacity to test the PR at some point?

[jarkkojs]

@baconpaul, sure no worries!

[esaruoho]

@jsakkine yeah just hook me up with how i get the code from your repo.

[jarkkojs]

@esaruoho, I guess you can ack this now?

[esaruoho]

I tried it and on the surface everything looks fine. I have not saved a patch tho cos feel like that is a way huger thing to bugshoot

[baconpaul]

Thanks @jsakkine - all great stuff! I just merged it. Closing this issue.