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

Annotations refactor #1044

Merged
merged 3 commits into from
Feb 21, 2025
Merged

Annotations refactor #1044

merged 3 commits into from
Feb 21, 2025

Conversation

jakubfiala
Copy link
Collaborator

@jakubfiala jakubfiala commented Feb 21, 2025

Description of changes

Adapt the Annotations Viewer integration to the new API coming in Powerpack v1.0.0.

Technical details

  • load the two separate contexts (AnnotationsProvider and AnnotationsEditorProvider). AnnotationsProvider wraps the whole Viewer incl. the activity feed, while AnnotationsEditorProvider just wraps the image/video and the toolbar.
  • expose the useAnnotations hook which allows CommentInput to grab the annotations (via useAnnotationsSync)
  • remove useViewerAnnotations because its functionality is inside the addon now. The "unique annotated frames" logic has been moved to VideoPlayer because that's the only place it's used.
  • remove the annotations-related bits from the Store
  • pass the mediaType, atMediaTime and src props to the AnnotationsEditorProvider
  • in useAnnotationsUpload we now use the useAnnotations hook to export the full-size composite image only when the comment is submitted.

Additional context

@jakubfiala jakubfiala self-assigned this Feb 21, 2025
@jakubfiala jakubfiala added the type: enhancement Improvement of existing functionality or minor addition label Feb 21, 2025
@jakubfiala jakubfiala requested a review from Innders February 21, 2025 13:25
@jakubfiala jakubfiala marked this pull request as ready for review February 21, 2025 13:26
Copy link
Member

@Innders Innders left a comment

Choose a reason for hiding this comment

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

  • Image over the top of the buttons

image

  • Drawing and then going to another frame too quickly causes the drawing to be lost. Maybe we should decrease the debounce time a bit?
Screen.Recording.2025-02-21.at.14.11.33.mov

@jakubfiala
Copy link
Collaborator Author

@Innders sounds good, I've reduced the debounce delay (on latest powerpack branch) - I'll have a look at the border thing. It looks like an issue with the image aspect ratio

@jakubfiala
Copy link
Collaborator Author

@Innders added a "submitting" state to CommentInput, for now it's just non-interactive and greyed out. Lmk if that feels sufficient or you'd rather have a spinner over it, too

@Innders Innders self-requested a review February 21, 2025 14:54
@jakubfiala
Copy link
Collaborator Author

@Innders added a 250ms transition with a 250ms delay, seems like a happy medium. Lmk what you think. Also, if you pull latest powerpack, the annotated images now have unique filenames.

@jakubfiala
Copy link
Collaborator Author

that should also fix the weird prefix dot. It was being added by the code that's meant to split file name from the extension, but it can't handle names without extensions 🙃

@Innders Innders self-requested a review February 21, 2025 15:28
Copy link
Member

@Innders Innders left a comment

Choose a reason for hiding this comment

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

Great stuff, I'm excited to get this out for wider testing 👏

@jakubfiala jakubfiala merged commit bc86d86 into develop Feb 21, 2025
@jakubfiala jakubfiala deleted the jf-annotations-refactor branch February 21, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants