Patch name updating issues

There are a number of issues which may be interrelated, so for now collecting them all here. These were all encountered when using a Prophet-10 desktop unit (in case any of these are adaptation specific).

Setup steps for issue

Select a patch in a bank on the synth (In Synth).
Switch to Current Patch tab.
Change the name and hit enter. The name is changed, and the log reports that the database is updated.

Resulting issues

[ ] If the patch (same data signature) is also visible in the left pane User Lists, the name in the user list is not updated until you collapse and reopen the list.
[ ] Sometimes after switching away from Current Patch (or selecting a different patch and back again), the name will revert back to the old name. Note: Closing and reopening the app "fixes" this behavior.
[ ] If the database pane is not showing the renamed patch, then switching to the "Synth Bank" tab will not reflect the new name. Getting the database pane to display the name will update the name in the bank. Note: Closing and reopening the app "fixes" this behavior.
[ ] Switching to another "In Synth" bank and back again will reset the name back to its original in both the bank view and in the center view (if it is on the page). It is still changed in the database, and selecting the tab it is on in the database view (or searching for it) will set the name back in the bank view and the database view. Note: Closing and reopening the app "fixes" this behavior.
[ ] There is no way to update the name change back on the synth. The patch in the synth bank is not highlighted in a red outline, and selecting Send to Synth does not update the synth. You also cannot drag the patch onto itself from the database view or bank view or user list (presumably because the data other than the name is the same), this has no effect.
[ ] Choosing "Import Again" on a bank that has patches of the same signature in the database will replace the names in the database, even if they were changed. This is especially problematic with the prior issue where changing a name is not sufficient to "dirty" a bank so it can be resent.

Good observations! I think these are two issues:

The non-refreshing UI issue
The name change doesn't cause dirty issue

The second should be rather easy to fix, we could even argue that if a patch is known to be "synth stored", i.e. there is a known position in any of the synth banks, we should either offer to send it to the synth right away or at least somehow inform the user. Given that not all synths have fast single program updates, and automatic write to synth seems dangerous/impossible. Marking the patch as dirty is a good solution.

Another aspect: When you reimport the bank, a changed name should not overwrite the one from the database, the database should always win IMHO. Of course, for that the fingerprint of the patch needs to be the same despite the rename, but that should be the case for the Prophet 10. The best solution I think would be if you reimport a bank but have changed the patch name in the database, the patch should be shown as dirty allowing to write it back to the synth.

Note there are synths which allow to extract the name, but not change it in the patch. You still can change the name in the software, but the patch data is not changed. For these of course, writing to the synth is not possible and the patch is probably not be flagged dirty.

Problem 1 is a bigger one that has different ways of solving... JUCE has multiple ways of Observers/Listeners, and while I had used hard coded update code in the beginning, with The Orm 2.0 I tried to switch to Values and ValueTrees and their listeners, but this is only half-baked and admittedly very complex, as copying Values not necessarily coped their ValueListeners, and suddenly not all Listeners get notified... I don't have a real recommendation, maybe the old way is the most robust - renaming can only be done on the CurrentPatch, and the UIModel singleton has a ChangeBroadcaster that any part of the software that needs to know of chanages to the CurrentPatch needs to listen to and react. The challenge is only to implement minimal UI updates on a generic "CurrentPatch has changed" notification call. Values bound to the UI would make that easier, but on the other hand are also complex to setup.

FWIW, I often set patch names on the actual synth, where that's possible, and it's usually a lot of work to do it so I'd want that change to be respected.

I often use synths without connecting them to a PC, edit patches on them and save them, or sometimes just change the name of a patch to something more descriptive.

I'm not sure why a patch which is the same other than the name can't be seen by the Orm as a different patch. That would seem to solve any renaming issues.

A way to deliberately search the database for patches like that would be useful, as a way of thinning out the database by identifying them (preferably not just deleting one automatically) but refusing to download one from the synth on those grounds wouldn't be what I'd want, personally.

I'm not sure why a patch which is the same other than the name can't be seen by the Orm as a different patch. That would seem to solve any renaming issues.

A way to deliberately search the database for patches like that would be useful, as a way of thinning out the database by identifying them (preferably not just deleting one automatically) but refusing to download one from the synth on those grounds wouldn't be what I'd want, personally.

Yes, this was my thinking as well. In my case (the Prophet-10 rev4), there literally is no way to change (or even see) the patch names on the instrument itself. However, I still want a "correct" view of what is in the synth bank. Any sort of automatic consolidation, or blurring between what is in a synth bank and what is in the database is likely to have these weird edge cases and misbehave. I don't want importing a synth bank to ever change anything other than Orm's view of the synth bank (although as Christof notes, this is a bug), and I don't want the Orm to misrepresent what is actually on the synth (after all, for synths that show patches, it would be weird to see the mismatch when patch names cannot be sent back).

My proposal would be (and apologies if some of these are already true or intended to be true, this is a statement of the proposed end result, and I want to be somewhat comprehensive):

Patch names are considered part of the patch (not just the underlying data). In practice means two "identical" patches can exist in the database and in a bank with different names.
Patches in a bank are all considered unique from each other and other banks. It is quite common to have many duplicate patches in a bank, and they should all be considered separate (data, name, and tags). This mirrors the behavior if these were modified on the synth, and makes it easier to manage names on a per-slot basis.
Synth banks import always overwrites its own patches (changing name and data). If these patches were also in a user list, those are updated.
The database view has an additional filters allow filtering duplicates (same name and data, but different bank).
The "Current Patch" tab should show where the patch being edited is: synth or user, bank name (not just a number) and slot.
Modified patches in synth banks (via drag and drop or name change) should be visually indicated in all views (red outline in center view and bank view, and "In synth" text for the modified bank should be red). This "dirty" patch is stored separately (essentially synth banks have another "hidden" bank containing just modifications). This could be saved in the database, so it is still there if you close and reopen Orm. There are three ways to resolve this "dirty" state: Re-importing the bank, sending the bank (or patch, if supported) to the synth, or an "Undo changes" button on both the Synth Bank and Current Patch tabs. (The undo is possible as it is stored as a separate patch)

Bonus features... These are not really related or required for consistent behavior but may become more important given the above approach.

Patches can be outside of a bank as well (available via the database view and user lists). Patches outside of a bank do not have a bank/slot number. Bank-less patches can be made via a "Copy" or "Archive" button on the Synth Bank and Current Patch tabs. These patches are never overwritten by importing synth banks, and not deleted by deleting user banks.
More sorting options for the center view. This will also affect the patch shown when duplicates are filtered (the first is shown). Sort by (patch name -> bank -> slot) or just by (bank -> slot). For bank it is always sorted as shown under "All patches": The "In synth" banks are first, then all user banks in User Banks in the order they appear there. Note: currently it seems to sort first by "sourceID", but that isn't resulting in anything intuitive.
Explicit center view filters for including/excluding banks. There is some weird filtering happening when I click on various banks on the left pane, but regardless if that was a filter it would still be one bank at a time unless multi-select is supported.

I need to think about this a bit more, but quick comment: One of the main features of The Orm before bank management was the possibility to finally clean up and deduplicate large patch collections, so the notion of "this is the same patch but only the name or location is different" was central.

With bank management, this now becomes more complex, as you clearly want to be able to rename patches on the computer and send the new names back into the synth, (rename # 1), but also rename them on the synth and get an updated bank view (rename # 2). Additionally, patches can be renamed by importing the patch from a file with a "better name" (rename # 3). This is important for synths that have stupid default names (P01-01) but better names exist when people have made the effort to enter a name. To make things more complex, there is at least one synth (the Matix1000) that has space for a patch name in the data (because it is compatible with the Matrix 6, which had a display), but will overwrite the patch name with the default name when loading into the synth and back (because it did not waste precious RAM on the name it could not display). This is meant to be solved by the "default name" mechanism, which always treates default names inferior to given names.

Add to that there are many old synths that have no patch name stroage, and the renamed name is only in the database anyway, or incomplete adaptations that have not yet implemented renamePatch().

I always wanted to add the possibility to record all imports for a patch, not only the first one, so you could trace back from which import a certain patch appeared, maybe under a different name and/or program position. Currently, it will just be skipped as "already exists". Maybe in that way the Synth Bank import is just like an additional import, and it can give a new name to an existing patch, and records the name of the patch within the bank position, not within the patch itself.

Note that we also need to deal with the merge conflict on bank import, as you might have renamed a patch on the synth and on the computer, so neither rename 1 nor rename 2 is applicable...

Additionally, it is up to the adaptation author to implement fingerprinting with or without name blank out. It is recommended, but not necessary.

Complex!

I need to think about this a bit more, but quick comment: One of the main features of The Orm before bank management was the possibility to finally clean up and deduplicate large patch collections, so the notion of "this is the same patch but only the name or location is different" was central.

Yes, this is really helpful! I guess I fall more on the side of having tools to actively clean things up rather than something that auto-magically does it for me -- because then I (as the user) are still in control. Quick tools to find duplicates and merge, rename, etc. are easier to get correct.

That said we are really only dealing with names, tags, and the raw patch data. The raw patch data is uneditable (and so "sharing" by its signature is clearly always fine). Tags are in the database only (so there is no synchronization conflict). Names are what are left and have all the complexities we've been talking about. So perhaps there is a name-specific solution to this. My only vague concern is there will be new things we would want down the line that may complicate a name-specific solution:

Tags also are a concept in many modern synthesizers -- we may want to leverage (and edit) those in the future. Then they become like names in complexity (actually, kinda worse).
Editor / Librarians are a thing... I can totally see wanting to evolve (for synths that support it) into a more full featured editor. I can totally see doing that myself, for fun, for the Prophet 10 anyway. This complicates any under-the-hood sharing as you don't want every edit to create a new copy, but you don't want to change other patches not being edited (but otherwise identical)

This is basically the thinking that lead me to patches in banks being fully unique from the get-go. It is more future proof for the above.

Additionally, patches can be renamed by importing the patch from a file with a "better name" (rename # 3).

Yeah, I didn't consider the import/export to files. I'm not sure how this works in Orm, but I could see that this likely pokes a big hole in my proposal. At least without the inclusion of supporting bank-less patches. How do the various supported file formats interact with Orm banks?

This is important for synths that have stupid default names (P01-01) but better names exist when people have made the effort to enter a name.

clipped <<<< Add to that there are many old synths that have no patch name stroage, and the renamed name is only in the database anyway, or incomplete adaptations that have not yet implemented renamePatch().

Yeah... I still want to see the synth view of the data though in the bank. It could be something each adaptation has to report (whether names are writable) If they are not, then the synth-specific name is stored (if it has names at all), and the UI allows toggling which names you see in a bank (the synth one or the Orm one ).

Note that we also need to deal with the merge conflict on bank import, as you might have renamed a patch on the synth and on the computer, so neither rename 1 nor rename 2 is applicable...

This was one the points of separate the dirty patch (instead of it just being a tracked in-memory state for a patch). It gives some flexibility... My proposal was to not deal with merge conflicts but simply overwrite as a starting point. Import clears any edits in the bank, and "send to synth" overwrites what is in the synth. However, if they are maintained as separate patches, then there are UI options to allow the user to decide what to do if we want the flexibility. For instance a new "Sync synth bank" option could do the following:

Import bank from the synth into temporary storage (not the database).
Check which synth bank patches changed from what was known in Orm (the non-edit patches). This checks both fingerprint and name (if names are writable, see previous comment).
If any conflicts occurred (there is an edit patch and a different synth patch), bring up a UI. If there is only a change on one side then that becomes the new version (skip the next bullet), if there are no changes then nothing to do clearly (skip the next bullet).
The conflict UI would list each conflicting patch indicating what the name differences are and whether the synth data changed. The user could then select whether they wanted the synth version or the Orm edit version.
Send the new bank to the synth if any Orm edits are still wanted.
If there is no error, update the synth bank patches and clear all edit patches. If there is an error, report it and abort. Everything remains the same.

Maybe in that way the Synth Bank import is just like an additional import, and it can give a new name to an existing patch, and records the name of the patch within the bank position, not within the patch itself.

Very interesting! This seems like a very small change and pretty much achieves about 90% of what I was wanting from a behavior perspective... I'm not seeing really any holes in this approach. Because the only thing that has possible conflict is the name -- it makes the synth bank patches effectively unique from the user's perspective. I still think there are perhaps some larger design questions regarding future-proofing to new Orm capabilities, but perhaps this could be a general path for this as well. I'm interested to understand how you would see this affecting the names displayed throughout the UI... I presume since it is in the bank only, it would not affect the visible patch name in any other pane (except perhaps "Current patch")? That could be confusing... But IMO, still better than what it is now.

Additionally, it is up to the adaptation author to implement fingerprinting with or without name blank out. It is recommended, but not necessary.

So.... I could make a Prophet-10 adaptation myself that includes the name in the fingerprint and I sort of get close to what I am suggesting? I guess I would call that a "bug" in an adaptation for the most useful version of Orm as it stands now. Certainly it will work, and with my earlier proposal it sort of works implicitly anyway, as the fingerprint is really just used as a tool to find duplicates -- not fundamental to how patches are stored (and shared) between banks.

Complex!

Yes, I may put together a shared Google Doc which is a bit better for describing flows and proposals... I'll cover the quick version (storing just the patch name in a bank) as well as my perhaps overly complex version. Also, if you want me to back off I can (I won't be offended) I realize I am perhaps overstepping here, as we are starting to tread dangerously on some fundamental design choices and this is all subjective really.

So.... I could make a Prophet-10 adaptation myself that includes the name in the fingerprint and I sort of get close to what I am suggesting? I guess I would call that a "bug" in an adaptation for the most useful version of Orm as it stands now.

Absolutly, the duplicate detection solely relies on the fingerprint calculated, so if you make a copy of the Prophet 5 file, and just don't specify the name position in the patch data, it will keep the name as a relevant part of the patch data and no deduplication will happen.

What we'd need to add then is to add a new deduplication query showing patches with multiple names, and you could decide which ones to delete. The UX of this will suffer as we still have no proper multi select, but it would be very similar to the current find duplicate names search (which finds patches which have different fingerprints but same name, so the exact opposite search).

To implement this, we'd still need the current fingerprint, so maybe we need to think about the calculateFingerprint() method first... maybe add a parameter "include name" which is default False, so current implementations are compatible, and once you implement it correctly the software will set True, which will make two patches differ if the name differs - then only the duplicate search will calculate the fingerprint excluding name to find patches which differ only in name.

Google Docs is a good idea, this is more than a single issue now!

What we'd need to add then is to add a new deduplication query showing patches with multiple names, and you could decide which ones to delete. The UX of this will suffer as we still have no proper multi select, but it would be very similar to the current find duplicate names search (which finds patches which have different fingerprints but same name, so the exact opposite search).

Would it be possible to have instant single delete, without confirmation but with an undo stack? Deleting multiple items is mostly awkward because of being asked to confirm each one but if there is an undo stack in the database (mark as deleted and add to a LIFO list), there's no real need for confirmation.

Google Docs is a good idea, this is more than a single issue now!

Yeah, will do. Currently I'm spending some quality time in the code base trying to learn how things work (and are intended to work) first. I likely will go dark for a while and/or will try and find some low hanging bug fixes that doesn't make such dramatic changes to learn more.

So, I made an attempt at solving the non-refreshing UI issue, as that's a problem no matter what the larger issues are. However, as predicted it gets problematic. Tracing down the reasons for each failure seems to come down to the same thing each time. Basically, it looks like it is coming from stale midikraft::PatchList/SynthBank or midikraft::PatchHolder instances being fed into UI classes to refresh them.

I attempted to fix some some places where UI classes are directly caching midikraft::PatchHolder (PatchButtonPanel and PatchButtonRow), but it was insufficient as PatchListModel gets a stale midikraft::SynthBank, which is pushes stale values in again. I'll keep whacking at this -- but I expect really there should be no caching of the midikraft-librarian classes anywhere (as member variables), and they need to always get rebuilt from the database whenever they are needed. This seems like it could be... slow (at least it feels slow). Another option could be creating an auto-updating wrapper for each of these that classes could hold onto instead of the librarian class directly.

Tested this again with the 2.0.8, which had a bit of cleanup in the Patchholder code, and can confirm the problem still persits. I think these are multiple bugs, I will spin off multiple issues from this ticket.

One more thing on the deduplication and name complexity: on #71 we already had the idea to record the original name as additional metadata, and have that searchable. The import name should probably kept forever, to allow tracing patches back.

christofmuc / KnobKraft-orm

Patch name updating issues #251

Setup steps for issue

Resulting issues