Skip to content

Consistent behaviour for sendBankToSynth by avoiding direct updates from user banks #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

salzaverde
Copy link
Contributor

@salzaverde salzaverde commented Oct 28, 2024

No description provided.

@salzaverde salzaverde changed the title Fix crash when filling list from database with filter not matching an… #353 Consistent behaviour for sendBankToSynth by avoiding direct updates from user banks Oct 31, 2024
@salzaverde salzaverde changed the title #353 Consistent behaviour for sendBankToSynth by avoiding direct updates from user banks Consistent behaviour for sendBankToSynth by avoiding direct updates from user banks Oct 31, 2024
midikraft::ListInfo info = {activeSynthBankId, ""};
auto activeSynthBank = std::dynamic_pointer_cast<midikraft::SynthBank>(patchView_->retrieveListFromDatabase(info));
if(!activeSynthBank)
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently relies on the user having loaded the corresponding synth bank first.

Instead of aborting with an error, we could call "Librarian::startDownloadingAllPatches" for that bank and perform the update in the finished callback. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I understand correctly you force a download of the bank first in case it hasn't been downloaded? That would work, my main concern was that you could accidentally overwrite patches in the synth that have never been downloaded, so you'd have no backup of them.

As some synths are really slow to download, the user could be surprised in that a lengthy download operation is started, but I think as this is abortable that's probably ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's a problem. On a higher level the idea was that this could work similar to Git, where there is a local state which can be modified and then pushed to the synth:

  • The synth banks in the database represent the local synth state
  • Before sending anything to the synth, the local state needs to be up to date with the remote state - if it's not, you need to import the corresponding bank from the synth first

Since this would be a rather fundamental part of the design, we should probably implement it in a more general place, though.

Then, how we solve the bigger problem of managing deduplication will have an impact, too (didn't find the time to think about it yet but there are some ideas). We could let this simmer for a bit until we have a clearer plan including the bigger picture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants