Skip to content

Batch delete all “unused” kit rows (the ones with no notes)#234

Merged
jamiefaye merged 11 commits into
SynthstromAudible:communityfrom
soymonitus:monitus/delete_unused_rows
Jul 26, 2023
Merged

Batch delete all “unused” kit rows (the ones with no notes)#234
jamiefaye merged 11 commits into
SynthstromAudible:communityfrom
soymonitus:monitus/delete_unused_rows

Conversation

@soymonitus
Copy link
Copy Markdown
Collaborator

@soymonitus soymonitus commented Jul 25, 2023

This is a big deal for me when I load an entire folder of percussion samples, play around with them till I find maybe a few that I like and create a sequence. Then later I want to quickly delete all the other rows that I didn’t use.
https://www.youtube.com/watch?v=R7WwzryMg4c
UPDATE:
Changed the key combo to: Kit + Shift + Save/Delete

This is a big deal for me when I load an entire folder of percussion samples, play around with them till I find maybe a few that I like and create a sequence. Then later I want to quickly delete all the other rows that I didn’t use. I am using the Shift + Delete shorcut which was unused now, while in Kit view
@weavermedia
Copy link
Copy Markdown
Contributor

Love this feature! 👏

@m-m-adams
Copy link
Copy Markdown
Collaborator

What does this shift+delete do in other contexts?

@soymonitus
Copy link
Copy Markdown
Collaborator Author

What does this shift+delete do in other contexts?

It does nothing. It only does something while in Kit view, but not in Synth, Midi or CV view

@weavermedia
Copy link
Copy Markdown
Contributor

weavermedia commented Jul 25, 2023

@m-m-adams @soymonitus According to the manual:

  • when browsing samples it deleted the sample from the SD card (I did not know that!)
  • when loading a song it deletes the focussed song
  • in multi-sample synths it deletes a selected note range

There may be others not mentioned in the manual.

@soymonitus
Copy link
Copy Markdown
Collaborator Author

soymonitus commented Jul 25, 2023

@m-m-adams @soymonitus According to the manual:

  • when browsing samples it deleted the sample from the SD card (I did not know that!)
  • when loading a song it deletes the focussed song
  • in multi-sample synths it deletes a selected note range

There may be others not mentioned in the manual.

I didn't know that. What I meant was that in Kit view it appeared to do nothing so I used that shortcut which is similar to holding a note from a row and pressing Delete, but without pressing any note, to indicate that you don't want a specific row to be deleted but all rows that you don't use, there should be another button combination to press, because just pressing Save/Delete saves the song. I am fine with any other button combination that is suggested, what is important to me is being able to clean a kit so fast and therefore save some CPU and RAM for my songs

@weavermedia
Copy link
Copy Markdown
Contributor

weavermedia commented Jul 25, 2023

This probably needs a community features setting since it introduces new behavior. Then the user will have to opt in to this behavior and then, hopefully, know what to expect.

@weavermedia
Copy link
Copy Markdown
Contributor

Should there also be a confirmation display like when deleting songs or samples. Press SHIFT+DELETE to start the action, then display shows DELEte, then press again to confirm the action. That's the standard behavior.

@soymonitus
Copy link
Copy Markdown
Collaborator Author

soymonitus commented Jul 25, 2023

I wanted to implement it that way, with community feature toggle and confirmation popup. But didn't know how..(it's my first feature). Any simple feature that has this that i can look into its PR to learn how to do it myself? And does the feature switch have to be On by default(as i found other features to be activated by default)?

@weavermedia
Copy link
Copy Markdown
Contributor

I wanted to implement it that way, with community feature toggle and confirmation popup. But didn't know how..(it's my first feature). Any simple feature that has this that i can look into its PR to learn how to do it myself?

My Catch Notes setting is one of the simplest features so far. It has a community setting: #221

@soymonitus
Copy link
Copy Markdown
Collaborator Author

What would be a good 4 letters word to popup for confirmation on 7seg displays? And feature name for the menu?

@m-m-adams
Copy link
Copy Markdown
Collaborator

Maybe delete+kit to clear the unused pads? That's a nice mirror of shift+kit to create a kit from a folder of samples and I think makes the action more explicit than just pressing delete while in kit view

@weavermedia
Copy link
Copy Markdown
Contributor

weavermedia commented Jul 25, 2023

You'll want a message for OLED and 7-seg, like "Delete unused pads?" and "DELE".

Might also be nice to implement a CANT popup too, something like:
numericDriver.displayPopup(HAVE_OLED ? "No unused pads to delete" : "CANT");

@weavermedia
Copy link
Copy Markdown
Contributor

weavermedia commented Jul 25, 2023

Maybe delete+kit to clear the unused pads? That's a nice mirror of shift+kit to create a kit from a folder of samples and I think makes the action more explicit than just pressing delete while in kit view

So that would be SHIFT+DELETE+KIT? Because just SHIFT+KIT saves the kit.

@m-m-adams
Copy link
Copy Markdown
Collaborator

Yeah shift+save+kit, delete is printed as shift+save on the front panel

@soymonitus
Copy link
Copy Markdown
Collaborator Author

I think the Shift + Delete is a good combination. Delete + Kit is confusing because if you press it in the other order, that is, Kit + Delete, it prompts you to Save the kit! Following the same press order as for deleting a single row, which is first pressing a row and then Delete, it is more logical to instead of pressing a row, pressing something like Shift and then Delete. It is a Delete action inside the current UI, which is the kit itself. This same combo works on other views so it is then more logical to implement it here in this view where it has no functionality yet.

@m-m-adams
Copy link
Copy Markdown
Collaborator

To be clear by delete+kit I mean the three button combo shift+save+kit. Shift+save is printed on the deluge as Delete

The issue with just using delete and no modifiers is it doesn't specify what you're deleting. Just pressing the save or load button without a modifier is a song level action, so I think delete (shift+save) would also be a song level action. Delete+kit makes it clear that the delete is targeted at the kit clip. I also think it's much harder to do a three button combo accidentally

@weavermedia
Copy link
Copy Markdown
Contributor

weavermedia commented Jul 25, 2023

I agree that DELETE+KIT (meaning SHIFT+SAVE+KIT) would be more suitable than simply SHIFT+DELETE for both reasons: [1] it's more explicit that your acting on the kit rather than at song level and [2] it's less likely to be pressed by mistake.

The community features page will explain how to the use the feature and it will also be a community setting so anyone using it will presumably be well informed.

@m-m-adams What about a confirm message? Seems like operations that delete from the SD card have this but operations that delete things in the current song don't. Going by that logic there wouldn't be a confirm step.

Regardless of the confirm step I still think it should show a CANT message in that case that no pads can be deleted.

@jamiefaye
Copy link
Copy Markdown
Collaborator

Perhaps we should have a place in the Community menu system where we would put functions like this, as opposed to having a key command sequence that I am guaranteed to forget or otherwise blunder. Perhaps a category called "batch actions", where this, auto-sampling, etc. can go?

@soymonitus
Copy link
Copy Markdown
Collaborator Author

Changed the key combo to Kit + Shift + Delete according to feedback from Discord conversartions and this PR feedback also

@m-m-adams
Copy link
Copy Markdown
Collaborator

Perhaps we should have a place in the Community menu system where we would put functions like this, as opposed to having a key command sequence that I am guaranteed to forget or otherwise blunder. Perhaps a category called "batch actions", where this, auto-sampling, etc. can go?

Yeah that's probably a good idea - maybe a top level 'actions' item added to the main settings menu

@soymonitus
Copy link
Copy Markdown
Collaborator Author

Perhaps we should have a place in the Community menu system where we would put functions like this, as opposed to having a key command sequence that I am guaranteed to forget or otherwise blunder. Perhaps a category called "batch actions", where this, auto-sampling, etc. can go?

I pictured this feature as something I would do quite often on my workflow while creating kit clips, to keep my songs CPU/RAM performant and clean, so I would prefer to have a shortcut combination

@m-m-adams
Copy link
Copy Markdown
Collaborator

Perhaps we should have a place in the Community menu system where we would put functions like this, as opposed to having a key command sequence that I am guaranteed to forget or otherwise blunder. Perhaps a category called "batch actions", where this, auto-sampling, etc. can go?

I pictured this feature as something I would do quite often on my workflow while creating kit clips, to keep my songs CPU/RAM performant and clean, so I would prefer to have a shortcut combination

Having a menu doesn't mean there can't be a shortcut, and it helps with discover ability and accessibility for new users who haven't memorized everything on the deluge. I do think it can be a separate PR though

… + Delete”

- Added also a  “Deleted unused rows” popup.
- Removed the now unneeded  “At least one row needs to have notes” popup because that condition can’t happen (if you hold a note in an empty row you will unavoidably add a new note, making that row not empty, so it is impossible that all rows are empty when you initiate this command
@soymonitus
Copy link
Copy Markdown
Collaborator Author

Added also a "Deleted unused rows" popup to inform the user of what the command did

@jamiefaye
Copy link
Copy Markdown
Collaborator

This is shaping-up quite well. I will merge 'er in tomorrow when I get in.

Copy link
Copy Markdown
Collaborator

@m-m-adams m-m-adams left a comment

Choose a reason for hiding this comment

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

Code looks good, only thing missing is an entry in the community features document explaining it!


clip->yScroll = 0; // Reset yScroll

actionLogger.deleteAllLogs(); // Can't undo past this
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this too hard to change? I feel like this is an action where an undo might be really handy

Copy link
Copy Markdown
Collaborator Author

@soymonitus soymonitus Jul 26, 2023

Choose a reason for hiding this comment

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

Seems difficult. I am keeping feature parity with the existing Note + Delete command (a single row delete), which also can't be undone.

@PaulFreund
Copy link
Copy Markdown
Collaborator

Just a quick question without looking into it, do you make sure there is always one row left? Deleting the last row most likely breaks a lot of other functions that rely on it since the UI doesn't allow to delete it so far

@soymonitus
Copy link
Copy Markdown
Collaborator Author

Just a quick question without looking into it, do you make sure there is always one row left? Deleting the last row most likely breaks a lot of other functions that rely on it since the UI doesn't allow to delete it so far

Yes, it always leaves at least one row.

@sichtbeton
Copy link
Copy Markdown
Contributor

This is great!

@jamiefaye jamiefaye added this pull request to the merge queue Jul 26, 2023
Merged via the queue into SynthstromAudible:community with commit 841df52 Jul 26, 2023
@soymonitus soymonitus deleted the monitus/delete_unused_rows branch July 27, 2023 23:44
@ok-reza
Copy link
Copy Markdown
Collaborator

ok-reza commented Aug 9, 2023

great work.

just a small note that maybe when trying to delete without any notes in any rows that the OLED message should say "1 ROW NEEDS NOTES TO DELETE UNUSED ROWS" instead of "AT LEAST ONE ROW NEEDS TO HAVE NOTES" so if a user stumbles on it by accident they have more context.

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.

7 participants