Closed will-moore closed 8 years ago
Conflicting PR. Removed from build FIGURE-merge#219. See the console output for more details. Possible conflicts:
@bramalingam This is mostly done now and is available on cowfish for testing if you'd like to have a preliminary look? Cheers. Currently I don't limit the loading of planes (just load them all as needed).
Bug: Ellipse drawing in Figure itself (not the ROIs imported from omero) from scratch does not work (the ellipse is drawn without lines, with the markers only)
If you click onto the green button in the dialogue in screenshot below and the image has NO ROIs to import from omero, then you are confronted with an empty white pane (the right-hand pane), i.e. both the button and the Tip vanish without explanation.
Clarify the situation with multipoint line. Note, that this type of ROI comes out of ellipses done via ImageJ and our plugin, but it is NOT supported by Figure. I would suggest it should be supported.
As discussed, the Shapes which are not supported by Figure (e.g. points, polylines as mentioned in https://github.com/ome/figure/pull/165#issuecomment-241433550 etc. ) should still be listed in the list of ROIs to Add to the image in Figure. This is so as not to confuse the user about "Where are my ROIs which I have drawn in OMERO ?"
@pwalczysko Addressed most of your comments:
Last 4 commits update the Model that we're using for Shapes to keep in sync with OMERO.
Ellipse cx, cy, rx, ry -> x, y, radiusX, radiusY,
To test, open a figure that was previously saved with some Ellipses on it. Also try to load ROIs from OMERO. Would be good to test against old OMERO (pre 5.3.0) at some point before release. Also, with https://github.com/ome/figure/pull/162, you should be able to export json from old version and import by pasting into new.
The work on ROIs in figure started before the existence of https://github.com/openmicroscopy/omero-marshal When adding new functionalities, it will make more sense to start using omero-marshal instead of creating another model since we will have 2 models to maintain/synchronize. This not only doubles the amount of coding but also the amount of testing. This will obviously require some refactoring and slow down the release of the new functionality but it is preferable in the long run.
omero-marhsal is already used in https://github.com/waxenegger/ome-ol3-viewer. See https://github.com/waxenegger/ome-ol3-viewer/blob/master/plugin/ome-ol3-viewer/views.py for example.
@jburel All the changes here are dealing with JavaScript data and have nothing to do with converting OMERO model objects to json. We are not creating another model. When the webgateway api (which uses omero_marshal) gets to supporting ROIs and Shapes then no other clients (webclient, ol3-viewer, figure etc) will have to duplicate the shapeMarshal code. Currently OMERO.figure uses the shapeMarshal code that's in webgateway to get json data from OMERO (avoiding code duplication). Making OMERO.figure use omero_marshal would mean we implement the same code in multiple places, increasing maintenance and testing, not reducing it.
I appreciate that some changes are javascript only, but not all, and that also in the long term we will be using the new webgateway API.
Major efforts have been made to try to keep figure, version "agnostic". This is also one of the goals of omero-marshal.
The new API is still under development, omero-marshal can already be used, evaluated in "real-life" applications e.g. pathviewer, ol3-viewer, figure etc.
Yes in the meantime, it might mean more efforts: we had to do that in other part of the platform. More importantly we should use the tools/libraries that we develop i.e. eating our own dog food. This is a good way to make them better e.g. https://github.com/openmicroscopy/omero-marshal/pull/6 Ignoring them in the applications that we develop sends the wrong message to external wishing to build their own app and also prevents from showing how one can interact with the various components of the omero-ecosystem.
Rois loaded from omero should be encoded using omero-marshal then the "rest" will handled the encoded data.
Tested the new commits as per https://github.com/ome/figure/pull/165#issuecomment-241716983. All good, also found a Figure with ellipse drawn previously and looked fine. The only comment:
Point
- it would be more consistent and helpful if hovering on the whole line elicits the tooltipAlso tested "Import from JSON" on cowfish, using JSON exported before the version change above. Worked fine, created https://cowfish.openmicroscopy.org/webmerge/figure/file/14615. Original version of this figure (not saved to VERSION 2 yet) still opens OK: https://cowfish.openmicroscopy.org/webmerge/figure/file/14451/
This allows loading of ROIs from OMERO to add to figure panels as new Shapes.
To test:
Discussed with @bramalingam: various things to improve:
Follow up ideas: