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

BugFix for two clippings get removed instead of one when removing clip #27

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

kabbaage
Copy link
Contributor

Fix for #26
The issue was that the clips were using the timestamp as the unique identifier, particularly during deletion. However, the timestamp generated with toLocaleString is in format MM/DD/YYYY, HH:MM:SS AM/PM. This means that if two clips were made very quickly, within the same second, they would have the same timestamp. And therefore, removing one would remove the other
My suggested fix is to use the true UTC timestamp as the "creationDate" for clips and as the unique identifier. This is in milliseconds therefore two clips will never have the same creationDate. And only for displaying it do we go back to calling toLocaleString

@saifabusaleh
Copy link
Owner

saifabusaleh commented Oct 19, 2021

hi @kavisherlock ,

Thanks for the PR and well done for knowing the issue :)

Please note that there is another PR that adds the export option

With your solution, when exporting we will need also to convert each UTC timestamp again to toLocaleString

What you think? maybe saving both of them is useful?

@kabbaage
Copy link
Contributor Author

I personally don't think we need to save both. They both convey the same info and having both just leaves potential for someone to accidentally change one without changing the other
I think it'd be better to convert it to human-readable time right before the export
But up to you. If you want me to have both, I can do that and use the real timestamp for deletion, etc and the creationDate for display/export

@saifabusaleh
Copy link
Owner

@kavisherlock I agree, its better to have one source of data, if we kept two sources of data (timestamp and display time) it can be source of bugs later when something changes, then we will need to change in two places

the code looks good, but I will test it a little bit later and merge

@saifabusaleh
Copy link
Owner

Hi @kavisherlock

export to JSON PR just got merged

please rebase from master and your changes to work with the new export functionality

Thanks

@kabbaage
Copy link
Contributor Author

Done. I also slightly modified the JSON stringify so that the exported file is a bit more readable
image
instead of being one line

@kabbaage
Copy link
Contributor Author

hmm. it keeps telling me to also squash the export commits into mine. Can you not do a squash and merge?

@saifabusaleh
Copy link
Owner

saifabusaleh commented Oct 28, 2021

hmm. it keeps telling me to also squash the export commits into mine. Can you not do a squash and merge?

you can either do squash or rebase
you should do rebase

also, the export commit is already in master

@kabbaage
Copy link
Contributor Author

Okay. The rebase finally worked

@saifabusaleh saifabusaleh merged commit 3a31776 into saifabusaleh:master Oct 28, 2021
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.

[Bug] when removing saved clip - two clippings get removed instead of one
2 participants