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

Add attachment to note #1825

Closed
ehhc opened this issue Apr 17, 2018 · 4 comments
Closed

Add attachment to note #1825

ehhc opened this issue Apr 17, 2018 · 4 comments
Labels
discussion 💬 Issue concerns a discussion.

Comments

@ehhc
Copy link
Contributor

ehhc commented Apr 17, 2018

In #1723 i asked for the feature to include attachments in nodes. I've submitted a really ugly quick fix. Unfortunately there are still some points:

  1. Why are the dropped files named with Math.random().toString(36).slice(-16) (CopyImages.js) instead of something more resilient like Hash(Timestamp + filename) ? Math.random() might return the same result again. Especially when not properly initialized!
  2. Why are there (at various places) different definitions of markdown-templates to include images? I’ve found at least 3 without being able to figure out why they aren’t at one central place and in what cases they are used
  3. Why are there multiple definitions of the image folder path? e.g. moveNote.js:77, exportNote.js:9, copyImage.js:22 and none global definition? A global reliable definition of this constant variable is missing.
  4. No documentation (e.g. JavaScript Doc) is provided. You have to guess what a method does and why it is necessary
    My quick fix hasn’t fix these issues.
    I’m working on them at the moment and will submit a more suitable solution as soon as possible.
@Rokt33r
Copy link
Member

Rokt33r commented Apr 17, 2018

I think most of your concerns sound right.

Why are the dropped files named with Math.random().toString(36).slice(-16) (CopyImages.js) instead of something more resilient like Hash(Timestamp + filename) ? Math.random() might return the same result again. Especially when not properly initialized!

I agree with you. We should use crypto.randomBytes.

Why are there (at various places) different definitions of markdown-templates to include images? I’ve found at least 3 without being able to figure out why they aren’t at one central place and in what cases they are used

Could you describe more about this?

And, I'm really sorry that there are lots of crap in the code of Boostnote. The reasons I couldn't catch up the current code are:

  1. I've been absent for a year.
  2. As a company for profit we've been preparing other service. But, it doesn't mean we abandon this project. The other services help us to make Boostnote much sustainable if self. So, I think we could focus Boostnote about this summer, probably the first week of July or much earlier.

Why are there multiple definitions of the image folder path? e.g. moveNote.js:77, exportNote.js:9, copyImage.js:22 and none global definition? A global reliable definition of this constant variable is missing.

Yea, we should refactor them and make one reliable definition.

No documentation (e.g. JavaScript Doc) is provided. You have to guess what a method does and why it is necessary
My quick fix hasn’t fix these issues.

TBH, we've been preparing an app, which probably replace the current Boostnote app. So, lots of things will be written from the scratch and it will use Typescript.
And, as my experience of typescript, JSDoc is quite redundant like the comment. So, we probably won't use it.
But, having api document sounds reasonable.

@Rokt33r Rokt33r added the discussion 💬 Issue concerns a discussion. label Apr 17, 2018
@ehhc
Copy link
Contributor Author

ehhc commented Apr 18, 2018

@Rokt33r can you already tell more about this new app? will it be open source and free as well?

@ehhc
Copy link
Contributor Author

ehhc commented Apr 18, 2018

@Rokt33r btw: my issue wasn't meant as a accusation. I can imagine that it's hard to maintain such a big project. I hope to make the project a little better (as it's intended by the open source idea) with my constribution. I've only mentioned the issues above so that's possible to understand why I try to refactor a few things.

I hope to finish my fix during the comming weekend. I'm looking forward to hearing your thoughts about it.

@Rokt33r
Copy link
Member

Rokt33r commented Apr 21, 2018

@ehhc Yes, it will be opensource and free!

Rokt33r added a commit that referenced this issue Apr 26, 2018
Fixes #1825 Refactoring of the attachment/image management
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 Issue concerns a discussion.
Projects
None yet
Development

No branches or pull requests

2 participants