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

Implemented Imagery selection screen #236

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Serhiy-Shekhovtsov
Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 19, 2017

Description

According to documentation on this screen should in particular:

  • List available DICOM images on local filesystem.
  • Display a preview of an available DICOM image.
  • Allow user to select a given image and start a new “Case.” This image will be used in the identification step next.

All these features has been implemented in this PR(frontend and backend).

  • Now user can see details about currently running case
  • Can see list of directories and preview an image
  • Can start a new case for selected image

start a new case

Reference to official issue

Related to #145 and #202

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:features/start-new-case branch from 6326900 to 91e648c Nov 19, 2017

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov referenced this pull request Nov 19, 2017

Closed

#145 import image series #233

1 of 1 task complete
return Response({'response': "An error occurred: {}".format(e)}, 500)

# drop existing case(s) with candidates and nodules
all_cases = Case.objects.all()

This comment has been minimized.

@lamby

lamby Nov 19, 2017

Contributor

Hang on, is that right? It will essentially make the entire process global whilst we would surely want it per-client?

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 20, 2017

Contributor

This is a valid concern, but it's a way all our modules implemented right now. We can later add authentication and implement per-client separation.

This comment has been minimized.

@isms

isms Nov 21, 2017

Contributor

@lamby Bear in mind the design doc stipulates single standalone computer w/ no auth story to keep everyone focused on the value added parts of the project.


# drop imported images
all_images = ImageSeries.objects.all()
all_images.delete()

This comment has been minimized.

@lamby

lamby Nov 19, 2017

Contributor

FYI you can can do ImageSeries.objects.all().delete()

</div>
</div>
<p>
You can select an other DICOM image and start a new case

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

an other -> another

this.preview.paths = paths
})
EventBus.$on('start-new-case', (filePath) => {

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

I wonder why is this using EventBus? The method to send new case can be invoked from startNewCase directly. If the params filePath is missing, we can simply pass it into this ImageSeries component via the props down in the TreeViews.

I think we should use EventBus sparingly to ensure components are modular and feature-compact. (That is, if I take this component and use it somewhere else, I would want to depend on this component to handle the job without having to worry about emitting an event from the parent component)

My suggestion is to move the Button from the TreeViews into the ImageSeries component and bind the onclick event locally in the image series.

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 20, 2017

Contributor

@louisgv, I am new to Vue.js so may be wrong but it seems like there is no simple way to implement two way binding between one component and it's ground child. We can emit events and catch+emit them on each middleman, but it doesn't seem nice to me. Also we can use DOM method this.$el.dispatchEvent like suggested here. But it's not nice neither.

This comment has been minimized.

@louisgv

louisgv Nov 20, 2017

Contributor

@Serhiy-Shekhovtsov I think I should have commented on the function down there instead of here :p

My thought was, is there a way to make the grand-child component feature complete without the need to rely on the grand-parent component? Since if the grand-child needs anything, the grand-parent can simply pass it down. Thus, we can maintain the uni-directional data flow, and will not have to worry about propagating data upstream, and will also ensure the grand-child has all the feature it need to be stand-alone and re-usable.

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:features/start-new-case branch from 91e648c to df889ae Nov 20, 2017

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 21, 2017

@Serhiy-Shekhovtsov @vessemer I see that this also part of PR #233. How might we best come to a consensus on which to merge..? Or which bits to merge, etc.?

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 21, 2017

@lamby my PR fits with requirements and easier for user: you can see images on the local system by default, click Preview and click Start new case.
While PR #233 requires the user to click Import first, click Preview image, then he has to add an image(by clicking Import) and then Create a case.

From my point of view, those steps are redundant and don't match with requirements. Of course, @vessemer can still update his PR, but I can't see much sense in this. If points is an issue - I have no problem with sharing them between us.

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Nov 21, 2017

@Serhiy-Shekhovtsov, in your PR I'm mainly concern with the Case model usage, as it was mentioned in the review. I guess it will be better if we allow to user switch between Case.objects without losing them. Also, there should be an opportunity to create more than one Case.object for a single ImageSeries.object (in case of several observators). My PR #233 aimed to deal with such case while introducing minimum changes into existing interface.
I also guess that it is not a good idea to introduce an EmitBus listener in such a general component as TreeViews.

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 21, 2017

I guess it will be better if we allow to user switch between Case.objects without losing them. Also, there should be an opportunity to create more than one Case.object for a single ImageSeries.object (in case of several observators).

I think this is needed.. or, putting it another way, I could open another issue to do this separate to this. Let me know :)

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 22, 2017

Switching between cases is good idea. From my point of view, the following changes could improve the #233:

No need to show Import button and require user to click it

Show the list of cases and the TreeView with files by default. User should be able to switch the case or start a new case with minimum amount of clicks.
open image

No need to require user to import image before starting a new case

Nor to show the redundant information. On the backend you can use ImageSeries.get_or_create(uri) method to load existing ImageSeries for a new Case.

Accordion like behavior for files TreeView

Because user should see what file he is previewing now. In current version user can unfold multiple folders same time and it's not clear what DICOM is opened in the viewer at the moment. This is the reason I used EmitBus. If you can see a better way to implement it - I have nothing against it.

If these changes are OK with you guys, @vessemer can update his PR and I will close my in favor of it. Alternatively, I can merge the list of cases from #233 to my PR.

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 23, 2017

@Serhiy-Shekhovtsov @vessemer

To clarify, it isn't really switching between cases it just that the system surely needs to be able to support multiple cases happening independently of each other.. I mean, otherwise we just have introduced a bunch of global variables / state that, well, that hopefully speaks for itself :)

I'd really dearly love for us to get one of #236 or #233 merged and am very happy to award points to both, I'd just need some clarification/confirmation on what exactly what code we should end up.

Did you come to any conclusions on this on another ticket? :) Let me know and I'll "action" stuff ASAP.

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Nov 23, 2017

I've implemented the aforementioned features without additional EmitBus usage, by this commit.
Double the demonstration here:
peek 2017-11-23 02-38

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 23, 2017

Loving the GIF screenshots btw - keep them coming! So, I'm seeing some conflicts here (and on #233 FYI).

@Serhiy-Shekhovtsov Any input from thee? :)

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 24, 2017

Guys, I am closing this PR in favor of #233. Will post my thoughts there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment