Reverse intensity (Insight) by dominikl · Pull Request #4850 · ome/openmicroscopy

dominikl · 2016-09-19T15:23:23Z

What this PR does

Adds the reverse intensity functionality to Insight.

Testing this PR

In preview and full viewer: Open the color picker for a channel, reverse the intensity by ticking the checkbox in the upper right corner of the color picker.

dominikl · 2016-09-20T07:47:17Z

By the way, I can move the checkbox/button somewhere else, if needed, not much hassle. Couldn't check how it looks like in OMERO.web, as #4839 apparently isn't in the build.

gusferguson · 2016-09-20T09:12:25Z

Tested using OMERO.insight-5.3.0-m4-253-43428df-ice36-b434-win eel user-3 read-only-1 image ID 24763

Location of checkbox appears fine.
Clicking the reverse intensity checkbox in the color picker appears to have the desired effect for both LUTs and plain colours and Preview and Accept buttons function as expected.

But:

Open image in Full Viewer - open Rendering Setting panes

as expected most of time - intermittently getting black user settings thumbnail in Full Viewer Rendering Settings pane - thumbnails in centre pane and Preview pane as expected - can’t reproduce reliably - may be after a number of channel colour changes and saves - see screenshot 1

Open image with imported settings in Full Viewer - open Rendering Settings pane

thumbnails and Preview as expected
use color picker on 3rd channel to select LUT - Green fire blue - click accept
thumbnail in FV RS pane user settings remains unchanged
thumbnail in Preview pane user settings - black
thumbnail in centre pane - unchanged
click save - no changes - screenshot 2
close FV - click refresh in Data Tree - still no update of thumbnails in centre pane or user settings in Preview pane

Re-open same image in FV - open RS pane

select color picker on same third channel - select reverse intensity - click accept
FV image reflects reversed intensity - Save button does not enable
Preview pane does not change
click on main window to get focus - Preview pane updates to show reverse intensity - save not enabled
close full viewer - select another image - go back to previous image - reverse intensity setting has not been saved
screenshots 3 and 4

Log out and log back in - LUT (not reversed intensity) showing in Preview pane and FV but all thumbnails in centre pane and user settings still show import settings - screenshot 5.

dominikl · 2016-09-20T15:42:34Z

Fixed:

Reverse intensity change wasn't recognized as rendering settings modification, that's why the save button wasn't enabled.
Various issues with the color picker when used in full image viewer.

Still don't know where the black thumbnail issues come from, couldn't replicate them either.

jburel · 2016-09-20T21:16:47Z

@dominikl: few more commits pushed to #4823

mtbc · 2016-09-21T06:55:33Z

I assume it's okay that the message mentions LOADING_RENDERING_CONTROL but the switch doesn't.

That's a good point. There are actually quite a lot similar cases where the message of the exception is not really correct. Guess that just slipped in with copy/pastes. Might be worth a separate general clean-up PR. Wouldn't add it to this PR, already rather long and reviewed.

It's fine with me if you prefer to fix these in a separate PR, just pop it on a 5.3 card.

This is more legacy, I will handle it in a separate PR

Just for reference: Trello card - Insight code cleanup

jburel · 2016-09-21T21:22:24Z

issue noticed this evening while looking at the black thumbnail problem.
I start with an image with 2 channels: first is green, second red

Change first to a lut, save etc.
close the viewer
re-open the viewer
try to reset the color to green. Click accept in the color picker.
The image does not get updated. Colors are compared and are the same but in that case a lut was applied. So the "isSameSettings" needs to be reviewed.

jburel · 2016-09-21T21:28:35Z

Since the reverse intensity box is in the color picker
When selected, we should probably change the color of the button to match what is happening (i.e. "reverse color")
Thoughts? cc @will-moore @gusferguson

gusferguson · 2016-09-22T08:46:42Z

Since the reverse intensity box is in the color picker when selected, we should probably change the color of the button to match what is happening (i.e. "reverse color")

Not sure about this - does the button represent the LUT selected and the reverse intensity is something else - or is the reverse intensity the equivalent of a different LUT?

dominikl · 2016-09-22T09:06:09Z

Yes, can replicate the problem #4850 (comment) . Thanks for the workflow.

RE #4850 (comment) : Was thinking about that, too. I'd also suggest to reverse/flip the color icon in the channel button (and slider?).

jburel · 2016-09-22T13:33:15Z

@gusferguson
the reverse intensity does new_value = 255-value e.g. 10 will become 245. So yes it can be seen as a different color/lut

gusferguson · 2016-09-23T08:43:46Z

@dominikl
Tested using OMERO.insight-5.3.0-m4-389-2cbfd1a-ice35-b438-mac eel user-3
Sorry I misunderstood the comment!
Yes the bug-fix has worked - last commit behaves as expected.

dominikl · 2016-09-23T08:47:14Z

Thanks @gusferguson . Yes, the bug was switching from color to lut and back didn't work. I guess the black thumbnail problem is still there, still a bit a mystery to me.

jburel · 2016-10-06T20:45:55Z

@dominikl: black thumbnail could be due to a race condition.
Anything in the logs?

jburel · 2016-10-10T10:44:37Z

discussed with @dominikl: optimistic lock reported previously see https://trello.com/c/9iAe3Sg9/95-lut-support

Anything more @gusferguson?

jburel · 2016-10-12T11:08:18Z

@pwalczysko: could you do a final round of review on this PR and the corresponding Web one i.e. #4839?
server has been merged, and it will be good to wrap up this work

pwalczysko · 2016-10-12T12:58:36Z

Bug:

Open an image in Full viewer.
Go to another image from the same Dataset. Open Preview.
In Preview, open a channel and check the "Inverse" checkbox, do not save
Copy the Rnd settings
go to another (third) image in Preview, paste the Rnd settings, then Save the Rnd settings
Still on the third image, click Save to All -> okay the dialog and observe that
all the thumbs in central pane change as expected, excelpt for the image whose full viewer is open

pwalczysko · 2016-10-12T13:02:19Z

The default size of thumbs in central pane after you open Insight has changed (became bigger).

This comment is linke to the SPW PR as this is the culprit.

jburel · 2016-10-12T13:04:14Z

@dominikl: note that the backend PR has now been merged.

jburel · 2016-10-12T13:08:06Z

larger thumbnail has been introduced by 926479f see PR #4874

pwalczysko · 2016-10-12T13:13:44Z

@jburel Okay, ta, made a comment on the SPW PR.

pwalczysko · 2016-10-12T13:17:43Z

In summary, one Bug detected #4850 (comment). @dominikl is telling me this is hard to fix, the Bug, although repeatable is a slightly edge workflow case, so not a blocker if cannot be debugged.

Otherwise than that it works nicely.

jburel · 2016-10-12T13:26:07Z

Is this edge case also in the release version?

pwalczysko · 2016-10-12T14:40:07Z

@jburel Yes, the edge case (=bug) is present in 5.2.5 release insight (connected to demo).

jburel · 2016-10-14T08:31:12Z

card for bug https://trello.com/c/X3pLD781/131-bug-insight

dominikl · 2016-10-14T09:16:10Z

Rebased to develop, after #4823 has been merged. Probably a quick re-review necessary.

dominikl · 2016-11-02T11:42:20Z

Spotted (and fixed) another bug. If the channel color is changed from a lookup table to a color, it wasn't set this color but it was set to the color which was specified before the lookup table has been set. I don't think both are related to this PR, but makes sense to fix them here.

I.e. to test last two commits:

Follow @will-moore 's workflow Reverse intensity (Insight) #4850 (comment) ; make sure it's fixed now.
Set a channel to a specific color (remember that color); save rendering settings; set channel to a lookup table; save; now set channel to a different color from the previous one; make sure the channel is set to this color and not the one from the initial step.

jburel · 2016-11-03T13:32:47Z

Tested the last 2 commits, both problems are now fixed

jburel · 2016-11-03T13:38:24Z

cf. RFE #4850 (comment)
this is something to probably discuss as a follow. I will add it to the card i.e. https://trello.com/c/SrcTVbw3/135-lut-improvements

One small problem noticed:
In color picker, if a color is selected, it is displayed at the "top", as soon as I tick/untick reverse intensity it becomes white. This is not the case when I select a lut.

jburel · 2016-11-03T13:45:00Z

     */
    private void handleColorPicker(boolean reset, boolean preview, int index,
-            Color newColor, Color oldColor, String newLut, String oldLut) {
+            Color newColor, Color oldColor, String newLut, String oldLut, boolean revInt) {


new parameter to add to javadoc

jburel · 2016-11-03T13:46:27Z

+
+                cb.setReverseIntensity(false);
+                List<CodomainMapContext> cdctx = c.copySpatialDomainEnhancement();
+                for(CodomainMapContext cd : cdctx) {


usually we have space between for/if and bracket

jburel · 2016-11-03T13:47:17Z

+     *            The channel index
+     * @return See above
+     */
+    boolean getReverseIntensity(int w);


maybe use index as in the other method

jburel · 2016-11-03T13:47:29Z

+     * @param revInt
+     *            The reverse intensity flag
+     */
+    void setReverseIntensity(int w, boolean revInt)


maybe use index as in the other method

jburel · 2016-11-03T13:48:31Z

+    /**
+     * Implemented as specified by {@link RenderingControl}.
+     * 
+     * @see RenderingControl#getReverseIntensity(int)


same index

jburel · 2016-11-03T13:48:44Z

+    /**
+     * Implemented as specified by {@link RenderingControl}.
+     * 
+     * @see RenderingControl#setReverseIntensity(int, boolean)


jburel · 2016-11-03T13:49:36Z

 package org.openmicroscopy.shoola.util.ui.colourpicker;

-
 //Java imports


remove //java imports and the other 2

jburel · 2016-11-03T13:51:41Z

+        newColor.lut = model.getLUT();
+        oldColor.lut = model.getOriginalLUT();
+
+        newColor.revInt = model.getReversetIntensity();


method name has an extra t. The method name needs to be fixed.

jburel · 2016-11-03T13:52:52Z

     * @param selectedLUT The selected lookup table
     */
-    public ColourPicker(JFrame owner, Color color, Collection<String> luts, String selectedLUT)
+    public ColourPicker(JFrame owner, Color color, Collection<String> luts, String selectedLUT, boolean revInt)


javadoc missing new param

jburel · 2016-11-03T13:59:15Z

 		String text = fieldDescription.getText();
 		if (text == null) return null;
-		return text.trim();
+		text = text.trim();


StringUtils.isBlank will handle null and length=0

dominikl · 2016-11-03T15:11:44Z

Thanks @jburel . All issues should be fixed now. Will add it to re-review tomorrow.

jburel · 2016-11-03T21:35:58Z

                List<CodomainMapContext> cdctx = c.copySpatialDomainEnhancement();
-                for(CodomainMapContext cd : cdctx) {
+                for (CodomainMapContext cd : cdctx) {
                    if(cd instanceof ReverseIntensityContext) {


space before if

jburel · 2016-11-03T21:37:05Z

-     * @param w The channel
+     * @param index The channel
     * @return See above.
+     * @throws RenderingServiceException    If an error occurred while getting 


while retrieving the contexts

Would it make sense to just say 'If an error occurred', the 'while ...' part basically just repeats what is said in the method description anyway?

makes sense

jburel · 2016-11-03T21:37:36Z

 	 * Returns the list of the levels.
 	 * 
 	 * @return See above.
+	 * @throws RenderingServiceException   If an error occurred while getting 


while retrieving the values

jburel · 2016-11-03T21:39:25Z

 //Third-party libraries

+
+import org.apache.commons.lang.StringUtils;


same here could remove //Third-party etc.

jburel · 2016-11-03T21:42:47Z

-		if(text.length()==0)
+		if (StringUtils.isBlank(text))
 		    return null;
 		return text;


I will still return text.trim()

dominikl · 2016-11-04T09:44:31Z

Last commit is javadoc only, no functional implication, so testing can still go ahead today if needed.

jburel · 2016-11-07T12:13:41Z

Problem noticed has been fixed and changes in doc done.
Merging Thanks

jburel added the develop label Sep 19, 2016

mtbc reviewed Sep 21, 2016

View reviewed changes

dominikl force-pushed the reverse_intensity_2 branch from f61a0c7 to e5bb0a4 Compare September 22, 2016 10:29

pwalczysko mentioned this pull request Oct 12, 2016

Spw grid view (Insight) #4874

Merged

dominikl force-pushed the reverse_intensity_2 branch from e5bb0a4 to c16c91c Compare October 14, 2016 09:14

dominikl closed this Oct 14, 2016

jburel reviewed Nov 3, 2016

View reviewed changes

dominikl added 3 commits November 3, 2016 14:38

Some code cleanup

c5a8c55

Fixed 'paintpot preview' problem with ColourPicker

7ce3ac7

Also changed the channel index parameter to 'index' in implementation

7a6281e

jburel reviewed Nov 3, 2016

View reviewed changes

Just some more javadoc fixes

7d1ec18

dominikl closed this Nov 4, 2016

dominikl reopened this Nov 4, 2016

jburel merged commit 378fd3a into ome:develop Nov 7, 2016

jburel added this to the 5.3.0 milestone Mar 29, 2017

dominikl deleted the reverse_intensity_2 branch May 3, 2017 09:28

		package org.openmicroscopy.shoola.util.ui.colourpicker;


		//Java imports

		//Third-party libraries


		import org.apache.commons.lang.StringUtils;

Conversation

dominikl commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Testing this PR

Uh oh!

dominikl commented Sep 20, 2016

Uh oh!

gusferguson commented Sep 20, 2016

Uh oh!

dominikl commented Sep 20, 2016

Uh oh!

jburel commented Sep 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburel commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jburel commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gusferguson commented Sep 22, 2016

Uh oh!

dominikl commented Sep 22, 2016

Uh oh!

jburel commented Sep 22, 2016

Uh oh!

gusferguson commented Sep 23, 2016

Uh oh!

dominikl commented Sep 23, 2016

Uh oh!

jburel commented Oct 6, 2016

Uh oh!

jburel commented Oct 10, 2016

Uh oh!

jburel commented Oct 12, 2016

Uh oh!

pwalczysko commented Oct 12, 2016

Uh oh!

pwalczysko commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jburel commented Oct 12, 2016

Uh oh!

jburel commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwalczysko commented Oct 12, 2016

Uh oh!

pwalczysko commented Oct 12, 2016

Uh oh!

jburel commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwalczysko commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jburel commented Oct 14, 2016

Uh oh!

dominikl commented Oct 14, 2016

Uh oh!

dominikl commented Nov 2, 2016

Uh oh!

jburel commented Nov 3, 2016

Uh oh!

jburel commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dominikl commented Sep 19, 2016 •

edited

Loading

jburel commented Sep 21, 2016 •

edited

Loading

jburel commented Sep 21, 2016 •

edited

Loading

pwalczysko commented Oct 12, 2016 •

edited

Loading

jburel commented Oct 12, 2016 •

edited

Loading

jburel commented Oct 12, 2016 •

edited

Loading

pwalczysko commented Oct 12, 2016 •

edited

Loading

jburel commented Nov 3, 2016 •

edited

Loading

jburel Nov 3, 2016 •

edited

Loading

jburel Nov 3, 2016 •

edited

Loading