Add Hilo Checkbox for LUT Toggling in Channel Slider View

[MinaEnayat]

Hello,
I've implemented a new feature to add a Hilo checkbox to the channel slider view.

This checkbox allows users to rapidly toggle the Hilo LU across all channels.

When the checkbox is checked, the Hilo LUT (hilo.lut) is applied to all channels, and when unchecked, the original colors are restored.

Please review the changes, and feel free to provide any feedback or suggestions.

Mina

@Tom-TBT

[will-moore]

This seems to be working OK, but it is causing a couple of things to go wrong with the Undo/Redo functionality.
I realise that there's no documentation on this, so I really need to write some guidance on how this works and things to watch out for...

The Undo/Redo code listens for any model.set() calls and when an attribute changes then this is added to the Undo queue. So it's really important that you only call model.set() when the user actually edits something. This means you must avoid calling m.set() when you're simply rendering the UI.
If you open a previously-saved Figure with this PR, and click around a few panels to change the selection, then you go to Undo a few times, you'll see that it's selecting the same panels. This is because on selection change, the m.set('hilo_enabled', false); is adding the selected panel to the Undo queue, even though there's no change in the UI.

So, I would simply ignore this step and I think the code will still work OK?

The Undo/Redo code tries to batch together multiple changes that happen "at the same time" (within 10 millisecs of each other). This is so that when you edit multiple panels at once, it's all handled under a single Undo action.
The channels list is really just a single attribute of the panel model, so you need to change the whole channels list of objects each time you change any channel. This is what model.save_channel() does for you.
So, each time you call model.save_channel() you're saving the whole state of all the channels each time.

When you do this several times very quickly, e.g. 3 times for a 3-channel image, all of these changes get bundled into a single Undo action. However, the Undo code doesn't apply those changes in reverse order when you undo (this is a previously hidden bug in the undo code). So, when you hit Undo, it undoes the change in channel 1, then channel 2, then channel 3.
This last Undo for channel 3 reverts it to the previous state (at the point that model.save_channel() was called for the 3rd channel) , which was e.g. : 1: HiLo, 2: HiLo, 3: Red.

So, for a fresh Figure, if you click the Toggle HiLo LUT checkbox for a 3 channel image, then hit Undo, you get this:

With this undo.js bug fix, the issue and the Undo works as expected:

--- a/src/js/models/undo.js
+++ b/src/js/models/undo.js
@@ -193,7 +193,8 @@ export const UndoManager = Backbone.Model.extend({
         // into undo / redo operations to go into our Edit below
         var createUndo = function(callList) {
             var undos = [];
-            for (var u=0; u<callList.length; u++) {
+            // add in reverse order
+            for (var u=(callList.length - 1); u>=0; u--) {
                 undos.push(callList[u]);
             }

Please feel free to include this change in this PR.

However, it feels wrong to make several separate calls to m.save_channel() and all the channels getting saved to the Undo queue each time. Better is to add a m.save_channels(channels) function to handle all at once.

Another bit of info that should be in the docs: It is generally better to call model.save() rather than model.set() because it has the extra behaviour of enabling the Save button for the Figure. In your case this already happens via save_channel() which uses m.save(), but if you made a change in isolation then you'd notice the difference.

I made these changes and it seems to be working (even without the undo fix above):

Here's all the changes I have...

--- a/src/js/models/panel_model.js
+++ b/src/js/models/panel_model.js
@@ -505,6 +505,16 @@
             this.save('channels', chs);
         },
 
+        save_channels: function(channels) {
+            // channels should be a list of objects [{key:value}, {}..]
+            var oldChs = this.get('channels');
+            // Clone channels, applying changes
+            var chs = this.get('channels').map((oldCh, idx) => {
+                return $.extend(true, {}, oldCh, channels[idx]);
+            });
+            this.save('channels', chs);
+        },
+
         toggle_channel: function(cIndex, active ) {

--- a/src/js/views/channel_slider_view.js
+++ b/src/js/views/channel_slider_view.js
@@ -276,19 +276,23 @@ var ChannelSliderView = Backbone.View.extend({
         this.models.forEach(function(m) {
             if (checkboxState && !m.get("hilo_enabled")){
                 // enable Hilo for Panel
-                m.set("hilo_enabled", true);
-                m.set("hilo_original_colors", m.get('channels').map(function(channel) {
-                    return channel.color;
-                }));                
-                m.get('channels').forEach(function(channel, idx) {
-                    m.save_channel(idx, 'color', 'hilo.lut');
-                });                           
+                m.save({
+                    "hilo_enabled": true,
+                    "hilo_original_colors": m.get('channels').map(function(channel) {
+                        return channel.color;
+                    })
+                });
+                let newChs = m.get('channels').map(function(channel, idx) {
+                    return {'color': 'hilo.lut'};
+                });
+                m.save_channels(newChs);
             } else if (!checkboxState && m.get("hilo_enabled")){
                 // remove hilo state
-                m.set("hilo_enabled", false);
-                m.get('channels').forEach(function(channel, idx) {
-                    m.save_channel(idx, 'color', m.get("hilo_original_colors")[idx]);
-                });   
+                m.save("hilo_enabled", false);
+                let newChs = m.get('channels').map(function(channel, idx) {
+                    return {'color': m.get("hilo_original_colors")[idx]};
+                });
+                m.save_channels(newChs);
             }
         })
     },  
@@ -296,9 +300,6 @@ var ChannelSliderView = Backbone.View.extend({
     loadCheckboxState: function() {
         var checkbox = this.$("#hiloCheckbox")[0];
         this.models.forEach(function(m) {
-            if(m.get('hilo_enabled') === undefined ){
-                m.set('hilo_enabled', false);
-            }
             checkbox.checked = m.get('hilo_enabled') || checkbox.checked;
         });

Hope that all makes sense?!

[will-moore]

Forgot to say: Thanks Mina for your first PR!
Also, we'll need a CLA form from you - see https://ome-contributing.readthedocs.io/en/latest/cla.html. Thanks!

[MinaEnayat]

Hello Will,

Thank you for your feedback and suggestions. They were incredibly helpful, especially in loading the images and applying the necessary functionalities.

I also included your bug fix of createUndo in undo.js

Best regards,
Tehmina Enayat

[will-moore]

This is working fine now, thanks.
It seems to me that when you have HiLo LUT enabled for the image panel, you really just want to have a single channel active at a time, similar to the greyscale mode that we support in the other viewers?
I can think of 2 parts of supporting this: one is easier than the other:

• When you enable HiLo, instead of only recording the colours for each channel, you could also record which channels are active. Then, you could also turn off all channels except the first of those that was active. When you turn HiLo off, you can then revert the active/inactive status of all the channels.
• Slightly harder is to enforce that only a single channel is on at a time - this would mean that when you turn one channel on (and HiLo is enabled) then you also turn the other(s) off.

Am I right in thinking this would improve the feature? If so, do you want to try it? (I can give pointers if you need)?

[Tom-TBT]

Hi @will-moore ,
I implemented both. I'm pleased with having only one channel at a time.

But about restoring the active state of the channels, I'm unsure.
It's useful for certain cases, but other times it gets in my way. Ultimately, it's a choice so I'd like your opinion on that (@pwalczysko too).

[will-moore]

@Tom-TBT @MinaEnayat this works nicely and I like it. I think it works well to revert to the previously active channels when you turn HiLo off.
The only tiny tweak I though of now, is that I don't think you need Toggle beside the checkbox. Just HiLo LUT feels like enough, since a checkbox is always used to toggle.

[will-moore]

Looks great, 👍

[pwalczysko]

It's useful for certain cases, but other times it gets in my way. Ultimately, it's a choice so I'd like your opinion on that

imho, it works logically as it is now (return to the active/inactive state as it was before). lgtm, Thank you