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

Codomain per channel rnd #4823

Merged
merged 77 commits into from
Oct 10, 2016
Merged

Codomain per channel rnd #4823

merged 77 commits into from
Oct 10, 2016

Conversation

jburel
Copy link
Member

@jburel jburel commented Sep 6, 2016

What this PR does

Re-activate the codomain map context. This was initially per rendering. Part I of the work
This is now per channel (part II). see #4793

Existing methods add/remove/update have been deprecated. Note that they have never been used.

New methods have been added to add/remove/retrieve the codomain contexts associated to a channel
No update method.
Only the ReverseIntensityMapContext has been added.

Testing this PR

Integration test has been updated

Related reading

https://trello.com/c/9iAe3Sg9/95-lut-support

cc @will-moore @dominikl

@joshmoore
Copy link
Member

@jburel
Copy link
Member Author

jburel commented Sep 7, 2016

I was wrestling with travis this am, good that it is now picked up ;-)

@jburel
Copy link
Member Author

jburel commented Sep 22, 2016

@will-moore @dominikl: The last commit should fix #4823 (comment)

@jburel
Copy link
Member Author

jburel commented Sep 23, 2016

@jburel
Copy link
Member Author

jburel commented Sep 23, 2016

The test passes locally, another error probably due to the directory re-organisation.

@jburel
Copy link
Member Author

jburel commented Sep 26, 2016

@dominikl
Copy link
Member

Looks good, thumbnails working again, so are the integration tests, can't see any other problems 👍

@will-moore
Copy link
Member

Everything seems to be working on #4839 so I think this is good to merge.

@will-moore
Copy link
Member

will-moore commented Sep 29, 2016

As reported by @pwalczysko #4839 (comment)
As a non-owner of an image where MY rendering settings are saved WITH Reverse Codomain, resetting to owner's rendering settings doesn't remove Reverse Codomain (owner doesn't have Reverse intensity). Other settings from the owner are applied.

In [3]: rss = conn.getRenderingSettingsService()
In [4]: rv = rss.resetDefaultsByOwnerInSet('Image', [16783])

@will-moore
Copy link
Member

Also, as reported by @pwalczysko, #4839 (comment)
applySettingsToSet() is working for applying Reverse Codomain to the set, but doesn't work for removing the Reverse Codomain from the set.

@jburel
Copy link
Member Author

jburel commented Sep 29, 2016

@will-moore @pwalczysko: The problems reported in #4823 (comment) and #4823 (comment) should now be fixed

@pwalczysko
Copy link
Member

The problems #4823 (comment) seem to be fixed in the backend as verified today using Insight (web PR is not in the build today).

@jburel
Copy link
Member Author

jburel commented Oct 4, 2016

@pwalczysko
Copy link
Member

pwalczysko commented Oct 4, 2016

Reviewing the Rnd workflows again in Insight, I have come accross a bug.

  • open an image in Full Viewer (one of the 56-lei) and tick (in Full Viewer) the checkbox "Reverse intensity" in one channel (do not Save, do not close the Full Viewer)
  • go to the containing dataset in the tree, right-click and Paste and Save
  • observe that all works as expected (all thumbs in central change accordingly)
  • now, still on the dataset, right-click and Set Imported and Save
  • observe that all works as expected except the image whose Full Viewer is open -> this image's thumbnail in central pane remains unchaged by the Set Imported and Save, all the others change as expected.

@dominikl is telling me this is not to do with this PR but with some older PR of his which is in merge.
Note that the bug CANNOT be repeated on eel latest.

@pwalczysko
Copy link
Member

Other than #4823 (comment) all works as expected.

@dominikl
Copy link
Member

dominikl commented Oct 4, 2016

Pretty sure the bug is related to #4862 (comment)

@jburel
Copy link
Member Author

jburel commented Oct 4, 2016

Thanks @pwalczysko for report, so if I understand correctly no work required on this PR but a problem to fix in insight

@pwalczysko
Copy link
Member

This bug #4823 (comment) is gone as of today.

@jburel
Copy link
Member Author

jburel commented Oct 6, 2016

Does it mean that this PR is ready from your pov?

@pwalczysko
Copy link
Member

Yes, ready to go, sorry.

@jburel
Copy link
Member Author

jburel commented Oct 10, 2016

merging this refactoring PR, any improvement/addition will be done in follow up PR if need be

Thanks all for review and tests

@jburel jburel merged commit c631b03 into ome:develop Oct 10, 2016
@jburel jburel deleted the codomain-per-channel-rnd branch October 21, 2016 10:21
@jburel jburel added this to the 5.3.0 milestone Mar 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants