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

Decouple Media Core #46

Merged
merged 72 commits into from May 13, 2021
Merged

Decouple Media Core #46

merged 72 commits into from May 13, 2021

Conversation

PJEstrada
Copy link
Contributor

@PJEstrada PJEstrada commented Apr 23, 2021

The purpose of getting out media core is to allow the file management to be independent of the annotation UI, and make all the annotations UI's to only receive a single file without consciousness of how that file was fetched.

This is a requirement to have before starting to implement the actual text interface: #45

@PJEstrada PJEstrada self-assigned this Apr 23, 2021
@PJEstrada PJEstrada added enhancement New feature or request pending_work labels Apr 23, 2021
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@PJEstrada PJEstrada mentioned this pull request Apr 23, 2021
@PJEstrada PJEstrada changed the base branch from master to text-ui-v1 April 23, 2021 18:10
@cypress
Copy link

cypress bot commented Apr 23, 2021



Test summary

54 0 0 0Flakiness 0


Run details

Project DiffgramFrontend
Status Passed
Commit 072888c
Started May 13, 2021 8:02 PM
Ended May 13, 2021 8:29 PM
Duration 26:52 💡
OS Linux Debian - 10.9
Browser Chrome 90

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@PJEstrada
Copy link
Contributor Author

PJEstrada commented Apr 23, 2021

Some of the initial refactor things:

  • I'm moving File_list out of the annotation_core component and into media_core. The idea is that annotation_core should not be responsible of the file list management.
  • I'm moving fetch_project_file_list and fetch_single_file into media_core
  • The File_list, current_file, metadata_previous will become data instead of props in media_core.

Some questions to discuss:

  • annotation_core still has some functions related to tasks like fetch_single_task. I would like to move this out of annotation_ccore, but I don't think it's a good idea to give media_core task responsabilitites. What do you think about moving the task fetching to annotation_full. That way annotation_full will have the responsability to fetch the task, get the file data (using media core methods) and based on that, decide which annoation UI to render (text or image). Another option would be to create a separate component just for task fetching/management purposes but I don't feel like this could be a component since there is no real UI rendered from this.
  • I also feel the get_media function in could be something that we can get out of annotation_core. But I still have some hesitancy because there is some special logic related to tasks, the render_mode and the file_view_mode. I'm not 100% sure if the 'history' mode and the render_mode is a relevant concept now. So wanted to discuss if we should remove this code or just refactor it in other functions maybe?

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Moving out of annotation core and into annotation_fuill
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

anthony-sarkis and others added 22 commits May 13, 2021 10:06
For loading context, if we do the block here, then all of the ghostloaders and other stuff for each individual section don't show as well. I see value in this for when we are doing tiling with multiple annotation_cores, but let's address that use case then as it feels like a very different context. Suggesting we leave hook but otherwise leave it up to individual components to handle loading
This is more of a future hook right? If correct then let's leave it empty till fully implemented
Because with multiple datasets, it's quite possible to have an empty query result which is different from new user case. Let's direct (that context) of new user stuff in media core too so it's in the same place
this was an older concept
via computed property,
so to make it cleaner align annotation_core assuming it will be given a prop onlyu
Trying to move as much of this "Created" context out of this component as possible and make it more flexible. Also to allow the component to load prior to getting a file
Annotation core only shows one file at a time now. So no reason for labels_view to be "smart" about loading. rather have it focus on rendering and pass this down from parent.
In new stronger admin vs task context this alert doesn't really make sense
Also as we shift to showing labels in different context
Context that we often don't want to have to do null check on key (eg for task mode), so smoother to just return empty set / array
-> We only commit if new project so might as well put that in same place
-> Fixed conditions so now it's properly on loading and file list
-> Now shows up better in full screen context too
-> Use tooltip buttons so we have the tooltip and consistent feel
-> Use built in vuetify spacer to align, removes need for custom styling, appears cleaning in my opinon
-> Shift icons to better encompass context
Close (closes entire thing)
Close feels ok in either state
-> Use window-maximize and window-restore. I think this makes it more generic, especially if we have other panels in the application. We know what the "up" thing will do, but a first time user may not.
@anthony-sarkis
Copy link
Member

  1. Moved message here so it shows up in full screen too (and just better place for it)
    no files match part of media core

  1. Adjusted buttons to try and make better for first time user and use built in style with spacer
    5a691d4
    restore down with close
    maximize 1

3) Labels are now computed props in the most parent component. Not saying it's exactly right yet, but I think it's a step in a cleaner direction for that concern.

image

@anthony-sarkis anthony-sarkis merged commit db5de93 into text-ui-v1 May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready_to_merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination error on File List
2 participants