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

Temporary workaround for app crashing #5

Merged
merged 5 commits into from Feb 25, 2022
Merged

Temporary workaround for app crashing #5

merged 5 commits into from Feb 25, 2022

Conversation

gdonisi
Copy link
Contributor

@gdonisi gdonisi commented Feb 22, 2022

The app is basically useless if it crashes before saving a note. This should fix #2 and gives you time to find a better solution if needed, because probably this isn't a perfect code style.
Tested with these languages as system default: English, Italian, French, German.

@gdonisi
Copy link
Contributor Author

gdonisi commented Feb 23, 2022

I've added a new commit. This fixes the app crash when a user clicks the Save button when he don't record the note. Also fixes a blank view if a user records a 0 seconds audio note:
0 second note

@gdonisi
Copy link
Contributor Author

gdonisi commented Feb 23, 2022

Third commit, I think using a timestamp instead of the note title as filename is better, I have two main reasons:

  • Title can be added later: if someone's in a hurry and just wants to record a quick note, he just starts recording and he can add the title before saving. Now the user can also modify the note title without consequences.
  • Saving two or more notes with the same title would overwrite the previous note. Having a timestamp prevents accidental loss.

Copy link
Owner

@certified84 certified84 left a comment

Choose a reason for hiding this comment

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

Hi there @gdonisi,

Well done so far. Saving a recording with the current time instead of the note title was always something I considered working on and I'm happy you already did. I'm also happy you found a better way of handling the roundOffDecimal() function.

Before merging this PR, I'd like to understand why you changed all the requireContext() to context.

@gdonisi
Copy link
Contributor Author

gdonisi commented Feb 25, 2022

Sure, it's just my style of coding: when I make a lot of calls to the same function like this, I prefer to have one variable to store the value (the context in this case) and use it instead of calling always the function.
Nothing changes to the behavior, you can use the requireContext again if you wish to 😁

@certified84
Copy link
Owner

Its fine since there are no changes to the behavior. Well done mate 🙌🙌
I'll be merging the PR now.

@certified84 certified84 merged commit 5f0e17d into certified84:v0.1.4 Feb 25, 2022
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.

None yet

2 participants