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.

Sign up for GitHub

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
salzaverde wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
964b35b
Fix crash when filling list from database with filter not matching an…
salzaverde Oct 28, 2024
e45be31
Consistent behaviour for sendBankToSynth by avoiding direct updates f…
salzaverde Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions The-Orm/PatchView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ void PatchView::retrieveBankFromSynth(std::shared_ptr<midikraft::Synth> synth, M
}
}

void PatchView::sendBankToSynth(std::shared_ptr<midikraft::SynthBank> bankToSend, bool ignoreDirty, std::function<void()> finishedHandler)
void PatchView::sendBankToSynth(std::shared_ptr<midikraft::SynthBank> bankToSend, std::function<void()> finishedHandler)
{
if (!bankToSend) return;

Expand All @@ -399,7 +399,7 @@ void PatchView::sendBankToSynth(std::shared_ptr<midikraft::SynthBank> bankToSend
if (bankToSend->synth() /*&& device->wasDetected()*/) {
midikraft::MidiController::instance()->enableMidiInput(location->midiInput());
progressWindow->launchThread();
librarian_.sendBankToSynth(*bankToSend, ignoreDirty, progressWindow.get(), [bankToSend, finishedHandler, progressWindow](bool completed) {
librarian_.sendBankToSynth(*bankToSend, progressWindow.get(), [bankToSend, finishedHandler, progressWindow](bool completed) {
progressWindow->signalThreadShouldExit();
if (completed) {
bankToSend->clearDirty();
Expand Down Expand Up @@ -1096,6 +1096,12 @@ void PatchView::fillList(std::shared_ptr<midikraft::PatchList> list, CreateListD
return;
}
}

if(database_.getPatchesCount(currentFilter()) == 0) {
spdlog::error("The list can't be filled, there are no patches in the database matching the current filter.");
return;
}

if (fillParameters.fillMode == CreateListDialog::TListFillMode::Top) {
loadPage(0, (int) patchesDesired, filter, [list, finishedCallback, minimumPatches](std::vector<midikraft::PatchHolder> patches) {
// Check if we need to extend the patches list to make sure we have enough patches to make a full bank
Expand Down
2 changes: 1 addition & 1 deletion The-Orm/PatchView.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class PatchView : public Component,
std::shared_ptr<midikraft::PatchList> retrieveListFromDatabase(midikraft::ListInfo const& info);
void loadSynthBankFromDatabase(std::shared_ptr<midikraft::Synth> synth, MidiBankNumber bank, std::string const& bankId);
void retrieveBankFromSynth(std::shared_ptr<midikraft::Synth> synth, MidiBankNumber bank, std::function<void()> finishedHandler);
void sendBankToSynth(std::shared_ptr<midikraft::SynthBank> bankToSend, bool ignoreDirty, std::function<void()> finishedHandler);
void sendBankToSynth(std::shared_ptr<midikraft::SynthBank> bankToSend, std::function<void()> finishedHandler);
void selectPatch(midikraft::PatchHolder& patch, bool alsoSendToSynth);

// Macro controls triggered by the MidiKeyboard
Expand Down
36 changes: 21 additions & 15 deletions The-Orm/SynthBankPanel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,29 @@ SynthBankPanel::SynthBankPanel(midikraft::PatchDatabase& patchDatabase, PatchVie
sendButton_.setButtonText("Send to synth");
sendButton_.onClick = [this]() {
if (patchView_ && synthBank_) {
auto activeSynthBank = synthBank_;

if (isUserBank()) {
patchView_->sendBankToSynth(synthBank_, true, []() {
spdlog::info("Bank sent successfully!");
});
}
else
{
patchView_->sendBankToSynth(synthBank_, false, [this]() {
// Save it in the database now that we have successfully sent it to the synth
patchDatabase_.putPatchList(synthBank_);
// Mark the bank as not modified
synthBank_->clearDirty();
refresh();
});
// Retrieve the corresponding active synth bank
auto activeSynthBankId = midikraft::ActiveSynthBank::makeId(synthBank_->synth(), synthBank_->bankNumber());
midikraft::ListInfo info = {activeSynthBankId, ""};
auto activeSynthBank = std::dynamic_pointer_cast<midikraft::SynthBank>(patchView_->retrieveListFromDatabase(info));
if(!activeSynthBank)
return;

@salzaverde salzaverde Nov 2, 2024

Copy link
Copy Markdown
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?

@christofmuc christofmuc Nov 2, 2024

Copy link
Copy Markdown
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.

@salzaverde salzaverde Nov 3, 2024

Copy link
Copy Markdown
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.


// Update active synth bank from user bank
for(auto& patch : synthBank_->patches())
activeSynthBank->changePatchAtPosition (patch.patchNumber(), patch);
}
}

patchView_->sendBankToSynth(activeSynthBank, [this]() {
// Save it in the database now that we have successfully sent it to the synth
patchDatabase_.putPatchList(synthBank_);
// Mark the bank as not modified
synthBank_->clearDirty();
refresh();
});
}
};

exportButton_.setButtonText("Export bank");
Expand Down Expand Up @@ -288,4 +294,4 @@ void SynthBankPanel::showInfoIfRequired() {
saveButton_.setVisible(showBank && isUser && synthBank_->isDirty());
sendButton_.setVisible(showBank && synthBank_->isWritable());
exportButton_.setVisible(showBank );
}
}