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

- added possibility to write comment tags #87

Merged
merged 5 commits into from Apr 7, 2021

Conversation

brainbugfix
Copy link
Contributor

I added the possibility to add the comments to the image during encoding as they come from decoding or added manually

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks @brainbugfix! add a round-trip test for this?

types will also need to be updated

lib/encoder.js Outdated

function writeCOM(comments)
{
if (typeof comments === "undefined") return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (typeof comments === "undefined") return;
if (typeof comments !== 'string') return;

perhaps? but ya know, inside the loop with an array check here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will not work, I am afraid. The type is 'object' as it is an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, misunderstood your comment. I will change that tomorrow.

@patrickhulce
Copy link
Collaborator

thanks that change looks great, would you mind tackling...

add a round-trip test for this?
types will also need to be updated

:D

@brainbugfix
Copy link
Contributor Author

I am not sure if I understand what you want me to do. I can add a test, that should not be a big deal but what exactly do you mean with updating the types?

@patrickhulce
Copy link
Collaborator

Oh sorry, I mean update the typescript definitions in index.d.ts at

export declare function encode(imgData: RawImageData<BufferLike>, quality?: number): BufferRet;

It's already a bit out of date but just changing the first encode parameter to RawImageData<BufferLike> & {comments?: string[]} should work?

- added a test for writing the comments
@brainbugfix
Copy link
Contributor Author

Did what you asked me for. Can you please double-check the TypeScript because I've never used this before.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks @brainbugfix this is great!

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
test/index.js Show resolved Hide resolved
lib/encoder.js Outdated Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator

if all these suggestions are ok with you @brainbugfix I'm ready to merge! :)

test/index.js Outdated Show resolved Hide resolved
@patrickhulce patrickhulce merged commit 13e1ffa into jpeg-js:master Apr 7, 2021
@patrickhulce
Copy link
Collaborator

thanks again :)

zed-0xff pushed a commit to zed-0xff/jpeg-js that referenced this pull request Feb 24, 2023
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