format v5, add page numbers to annotations

[ddvk]

No description provided.

[lobre]

Hello,

Thanks for the PR!

Supporting v5 was wanted with issue #67. I hope I had time to do it some time ago but I did not. Nice that you are proposing these changes.

As mentioned in the issue, @peerdavid states that there are changes in the IDs of BrushTypes with v5. (see #67 (comment)).

Before merging this PR, I think it would be nice to have these changes implemented.

And maybe someone from the community finally found out what this new float32 added property was about? Might be interesting to dig other projects to check this out.

And if we don't find this info, we can leave this to "Unknown" but I would prefer having it typed as "float32" instead of "BrushSize" (as we don't know if it is brush size related).

Thanks again,
Loric

[ddvk]

that was just a quick fix (i would like to keep the v3 support + make further format version changes easier)

As mentioned in the issue, @peerdavid states that there are changes in the IDs of BrushTypes with v5. (see #67 (comment)).
Before merging this PR, I think it would be nice to have these changes implemented.

it makes sense, i'll port @peerdavid 's python script changes, if someone else doesn't beat me to it

And if we don't find this info, we can leave this to "Unknown" but I would prefer having it typed as "float32" instead of "BrushSize" (as we don't know if it is brush size related).

my bad, I forgot to rename it

[lobre]

Thanks!

For the support of multiple versions, I was wondering if it was worth it. (as explained here #67 (comment)).

As it is just a transition (notes will eventually all become v5), I don't know if it makes sense to "clutter" the code to maintain at the same time v3 and v5.

Contributions are quite rare and keeping the code as clean as possible seems to be a better choice in this trade-off.

As said in my comment, maybe a tool / package to transform a v3 note into a v5 note would be more appropriate and would keep the main code easy and clean with only one version...

I'll try to see if I can understand this"Unknown" field as well.

Loric

[ddvk]

Finally had some time to work on this.

After considering it a bit, I think that it should be ok to support multiple format versions (some users may never update their old notes and manually converting the files is cumbersome)

I tried to abstract the version handling, as you can see in the latest commit (still new to go and don't know if that is the idiomatic way, probably should have made new packages) so that the code is not cluttered with version specific logic

About the brush type codes, I'm not sure what the general idea is i.e. have a base set and map new versions to it (which is basically what I did) or handle it in a different way (depends on how to draw logic is implemented)

[sirupsen]

Great work @ddvk, thanks for this! I tested this branch, and it works well for the notes I tried. 👌

Something I'd love to see would be the ability to exclude page-numbers, possibly with a flag.

[lobre]

Hey, sorry for the delay in the response, I'll have time rather soon to review / comment!

[sirupsen]

Works wonderfully for me.

This comment may be more in general directed towards exporting annotations, but the output is visually quite different from exporting to PDF with Remarkable itself (which is close to how it appears on the device) and with rmapi. I doubt that's endemic to this branch, and shouldn't block.

Rmapi:

Remarkable:

[juruen]

This comment may be more in general directed towards exporting annotations, but the output is visually quite different from exporting to PDF with Remarkable itself (which is close to how it appears on the device) and with rmapi. I doubt that's endemic to this branch, and shouldn't block.

Yeah, the annotations need a bit more of love to make them look more close to what the official app renders.

I hope to have some time this week to give it another go and also look into overlaying annotations on top of the actual PDF based on @rorycl 's work.

[juruen]

👋 all,

I'm going to get this merged into master as soon as possible.

I'd still like to make some changes to the PR, but I think it'll be more beneficial for users to have something that works for v5 in master and take it from there.

Thank you all for your help!

[lobre]

Oh, I started a review yesterday on this PR that I wanted to submit today. I think this is of good grounds but there are a few things that can be improved to keep the code clear.

Was not expecting a merge just a the same time ^^. I'll still submit my review tonight and maybe we can refactor in another PR.

[juruen]

Oh, I started a review yesterday on this PR that I wanted to submit today. I think this is of good grounds but there are a few things that can be improved to keep the code clear.

Was not expecting a merge just a the same time ^^. I'll still submit my review tonight and maybe we can refactor in another PR.

I'm really sorry I didn't wait for your review. Please, submit it and I'll try to take care of the changes.

Thanks a lot and sorry again for the quick merge.

[lobre]

No worries, it is been ages that I wanted to review this and I finally found an hour yesterday!
It seems we have acted at the same time.
Maybe instead of reviewing, I'll try to make the corrections myself and propose a PR. It will be easier ;)

[juruen]

Maybe instead of reviewing, I'll try to make the corrections myself and propose a PR. It will be easier ;)

Thank you ❤️

[lobre]

Hello,

As not having been fast enough to review, I have "implemented" my comments/ideas with #84.

Feel free to comment!
Loric