Skip to content
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

Export and replay generated wav #402

Merged
12 commits merged into from
Jul 9, 2020

Conversation

matheusfillipe
Copy link
Contributor

So this is my attempt of adding replay and export functions. I first thought on only keeping the last played wav, but then I thought would be nice to let the user choose among the last generated ones for like, comparing and choosing the best.

I added a new row on the right side for the 3 new widgets. Turned out to be the best placement and I hope this is fine until the interface cleanup. The interface looks like this now:

test

@ghost
Copy link

ghost commented Jul 7, 2020

@matheusfillipe I tested the update and it works very nicely. The replay and history features are a very useful addition!

toolbox/ui.py Show resolved Hide resolved
@matheusfillipe
Copy link
Contributor Author

I think the extension issue is fixed now, on my system the default is wav for some reason :P

@matheusfillipe matheusfillipe requested a review from a user July 7, 2020 00:49
toolbox/__init__.py Outdated Show resolved Hide resolved
toolbox/__init__.py Outdated Show resolved Hide resolved
toolbox/__init__.py Outdated Show resolved Hide resolved
@matheusfillipe matheusfillipe requested a review from a user July 7, 2020 02:39
toolbox/__init__.py Outdated Show resolved Hide resolved
#TODO better naming for the combobox items?
self.ui.waves_list_model.setStringList(["%d" % (i+1) for i in range(n_waves)])
self.ui.waves_cb.setCurrentIndex(n_waves-1)
cb_items = ["%d" % (self.waves_count - i) for i in range(0, min(self.waves_count, MAX_WAVES))]
Copy link

Choose a reason for hiding this comment

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

What I intended to suggest is to maintain a list called self.waves_namelist and append the count to the list (as a string) every time a wave is made.

This will make it easier to implement the TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am already doing on line 250:

self.waves_namelist = ["%d" % (self.waves_count - i) for i in range(0, min(self.waves_count, MAX_WAVES))]

I am recreating the list every time, because this is exactly what goes on the combobox, and I wouldn't be able to just keep appending to it unless if poping 0 like in line 247 with self.waves_list and then reverting this one but I don't actually see how is that going to help on the todo. Is that what you mean anyway?

What I meant by that TODO is that maybe there is a better way to name those or let the user name them.

@matheusfillipe matheusfillipe requested a review from a user July 7, 2020 03:06
toolbox/__init__.py Outdated Show resolved Hide resolved
@matheusfillipe matheusfillipe requested a review from a user July 7, 2020 03:26
@ghost
Copy link

ghost commented Jul 7, 2020

Thanks for addressing my feedback @matheusfillipe ! There is one more here: #402 (comment)

@ghost
Copy link

ghost commented Jul 7, 2020

I have pushed some suggested changes to the source branch (matheusfillipe:save_and_replay_wav).

@ghost ghost requested a review from CorentinJ July 7, 2020 18:30
@ghost
Copy link

ghost commented Jul 7, 2020

@CorentinJ Requesting your feedback on these changes.

@ghost
Copy link

ghost commented Jul 7, 2020

Clear to perform squash and merge @matheusfillipe . Thank you for this very nice toolbox enhancement.

@ghost
Copy link

ghost commented Jul 9, 2020

Hi @matheusfillipe , please go ahead and perform the squash and merge.

@matheusfillipe
Copy link
Contributor Author

Hi @matheusfillipe , please go ahead and perform the squash and merge.

I don't have write access @blue-fish .

@ghost
Copy link

ghost commented Jul 9, 2020

My apologies @matheusfillipe , I got "collaborator" and "contributor" mixed up and thought that you would have write access as a contributor. I'll merge it in now.

@ghost ghost merged commit 6944770 into CorentinJ:master Jul 9, 2020
@ghost ghost mentioned this pull request Sep 10, 2020
This pull request was closed.
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