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

Grid view and multiple context images supported #5542

Merged
merged 57 commits into from
Jan 16, 2023

Conversation

bsekachev
Copy link
Member

@bsekachev bsekachev commented Jan 1, 2023

Motivation and context

image

image

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@bsekachev
Copy link
Member Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

❌ Some checks failed
📄 See logs here

@nmanovic nmanovic changed the title [WIP] Bs/multiple context images [WIP] multiple context images Jan 2, 2023
@bsekachev
Copy link
Member Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

❌ Some checks failed
📄 See logs here

@bsekachev bsekachev changed the title [WIP] multiple context images [WIP] Grid view and multiple context images supported Jan 3, 2023
@bsekachev
Copy link
Member Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

❌ Some checks failed
📄 See logs here

@bsekachev
Copy link
Member Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

✔️ All checks completed successfully
📄 See logs here

Copy link
Contributor

@klakhov klakhov left a comment

Choose a reason for hiding this comment

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

I think for long image names text should not be covered with icons, maybe we can move it to the right and clip it with ellipsis. Maybe adding a tooltip in that case.
image

I think we should switch places of remove button and move button, having remove button at the first place can lead to accidental image removal
image

cvat/apps/engine/views.py Outdated Show resolved Hide resolved
cvat/apps/engine/views.py Outdated Show resolved Hide resolved
zip_file.writestr(f'{name}.jpg', result.tobytes())
# response = HttpResponse(wrapper, content_type='application/zip')
# response['Content-Disposition'] = 'attachment; filename=your_zipfile.zip'
return HttpResponse(io.BytesIO(zip_buffer.getvalue()), content_type='application/zip')
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably HttpStreamingResponse is better. Let's have HttpResponse here because in other places we also use it. Another idea is to use FileStream (I believe it is possible to use io.BytesIO together with the response type).

@@ -924,7 +924,7 @@ class FrameMetaSerializer(serializers.Serializer):
width = serializers.IntegerField()
height = serializers.IntegerField()
name = serializers.CharField(max_length=1024)
has_related_context = serializers.BooleanField()
related_files = serializers.IntegerField()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep here the list of names. But let's keep it as is for now. Need to think twice.

@nmanovic
Copy link
Contributor

Screen Recording 2023-01-13 at 22 28 14

@nmanovic nmanovic merged commit fb0b867 into develop Jan 16, 2023
@nmanovic nmanovic deleted the bs/multiple_context_images branch January 16, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants