Fix labels around border by Rdornier · Pull Request #616 · ome/omero-figure

Rdornier · 2025-02-03T14:48:31Z

Resolves #614, resolves #615

will-moore · 2025-02-03T15:19:54Z

        y = panel['y']
        width = panel['width']
        height = panel['height']
+        border = panel['border']


better to use border = panel.get('border') which won't cause an error if border is missing

will-moore · 2025-02-17T15:34:59Z

The app JS fix looks good.

The PDF labels are shifted OK for single labels in some positions, but where we have multiple labels (e.g top right panel) or labels on the inside corners then it's not looking right.
Also left-vertical labels (bottom-left panel) is too far left.

Screenshot shows app (top-left), PDF with this PR (top-right) and PDF without this PR (bottom). All borders are 15 px:

Rdornier · 2025-02-18T13:59:18Z

Hello Will,

Thanks for you careful review. I've made a fix to correct that.

There is only one remaining thing. When you re-open the figure, with vertical labels on panel with borders, the vertical labels are very far away from the panel. If you move the panel, then the position of the labels are correctly updated.

I found out that this behavior is coming from those two lines

            // need to force update of vertical labels layout
            $('.left_vlabels', self.$el).css('width', 3 * self.$el.height() + 'px');
            $('.right_vlabels', self.$el).css('width', 3 * self.$el.height() + 'px');

If I remove them, this behavior disappear. But I don't know if I'm allowed to remove them, as they should be needed for something, right ?

Rémy.

will-moore · 2025-02-24T15:29:01Z

OK, so this took me quite a bit of digging...

You do need those force update lines... I saw the commit that added them mentioned "fix labels on non-square panesl" or something like that, and if you have a rectangular panel you'll see that they are really needed!

I also noticed that if you simply toggle the border (hide then show) then it fixes the issue, which turns out that simply calling render_labels() again is all that's needed.

Then I tried adding an extra call to render_labels() into the setTimeout() block (in the diff below), and that ALSO fixed the problem!
It took a while to work out that self.$el.height() (in the "problematic" lines that you tried removing) returns 2 different values: When render_labels() is first called on the initial render(), and again after the timeout. The difference in the heights is equivalent to 2 x border width.

It turns out that the first time that render() and render_labels() is called is BEFORE the panel has been added to the DOM. At that point it's just a jQuery object. After the timeout it is then attached to the DOM, and is then subject to different page layout css etc. So, calling render_labels() again gives a different self.$el.height().

If I update the code to avoid calling render() until AFTER the panel is added to the DOM, then it all behaves much better.
I also found that render_labels() is being called lots of times, so I've tried to clean that up too.

With that change, it also turns out that we don't need render_shapes() to be called after a timeout either! It needed that before because Raphael code was unhappy if the elements weren't on the page DOM.

Give this a try!

diff --git a/src/js/views/figure_view.js b/src/js/views/figure_view.js
index 361c3535..4eeff4df 100644
--- a/src/js/views/figure_view.js
+++ b/src/js/views/figure_view.js
@@ -796,7 +796,9 @@
         addOne: function(panel) {
             var page_color = this.model.get('page_color');
             var view = new PanelView({model:panel, page_color:page_color});
-            this.$figure.append(view.render().el);
+            this.$figure.append(view.el);
+            // render works better AFTER attaching element to the DOM
+            view.render();
         },
 
         renderLoadingSpinner: function() {
diff --git a/src/js/views/panel_view.js b/src/js/views/panel_view.js
index 4efa11a6..a5ce5b6a 100644
--- a/src/js/views/panel_view.js
+++ b/src/js/views/panel_view.js
@@ -48,7 +48,6 @@
             if (opts.page_color) {
                 this.page_color = opts.page_color;
             }
-            this.render();
         },
 
         events: {
@@ -373,15 +372,8 @@
 
             // update src, layout etc.
             this.render_image();
-            this.render_labels();
-            this.render_scalebar();     // also calls render_layout()
-
-            // At this point, element is not ready for Raphael svg
-            // If we wait a short time, works fine
-            var self = this;
-            setTimeout(function(){
-                self.render_shapes();
-            }, 10);
+            this.render_scalebar();     // also calls render_layout() -> render_labels()
+            this.render_shapes();
 
             return this;

Rdornier · 2025-02-25T08:00:12Z

Hi @will-moore

Indeed, it's a deep review you did here, thanks for your time !
I've tested it and the bug indeed disappear. I don't see any other bug that could be induced by those changes, at least on what I've tested. So, I think it's good.

Thanks again for finding the solution,
Rémy.

snoopycrimecop · 2025-02-26T00:44:29Z

Conflicting PR. Removed from build OMERO-plugins-push#345. See the console output for more details.
Possible conflicts:

PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

Tom-TBT · 2025-02-26T09:51:40Z

-                self.render_shapes();
-            }, 10);
+            this.render_scalebar();     // also calls render_layout() -> render_labels()
+            this.render_shapes();


the conflict with #605 is coming from here

https://github.com/ome/omero-figure/pull/605/files#diff-fd31cdc90e2bd6fb92184b2b54474aa097d94dcbcc6ebf591b99715c6c0b0744R449-R452

snoopycrimecop · 2025-02-27T00:47:41Z

Conflicting PR. Removed from build OMERO-plugins-push#346. See the console output for more details.
Possible conflicts:

PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-02-28T00:46:57Z

Conflicting PR. Removed from build OMERO-plugins-push#347. See the console output for more details.
Possible conflicts:

PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

Tom-TBT

this PR fixes what it says it fixes. I also tested together with the colorbar PR #605 and there's no interference. Looks good on that front.

But I would like to also fix the "crop page around panel" to account for the border. But this would bring more conflicts as I'm also modifying those parts extensively in the colorbar PR.

I suggest to proceed in this order:

merge colorbar PR
solve current conflicts
add following changes to handle the "crop page"

diff --git a/src/js/models/panel_model.js b/src/js/models/panel_model.js
index fbef52a5..722e6465 100644
--- a/src/js/models/panel_model.js
+++ b/src/js/models/panel_model.js
@@ -1104,6 +1104,10 @@
             shift = top_labels.reduce(function(prev, l){
                 return prev + (LINE_HEIGHT * l.size);
             }, shift);
+            var border = this.get("border");
+            if (border && border["showBorder"]) {
+                shift = shift + border["strokeWidth"];
+            }

             var shift_cbar = 0;
             var cb = this.get("colorbar");
@@ -1138,6 +1142,10 @@
                 const width = this.measureTextSize(l.text, l.size + "px");
                 shift = Math.max(shift, width);
             }
+            var border = this.get("border");
+            if (border && border["showBorder"]) {
+                shift = shift + border["strokeWidth"];
+            }

             var start = "";
             var end = "";
@@ -1185,6 +1193,10 @@
                 const width = this.measureTextSize(l.text, l.size + "px");
                 shift = Math.max(shift, width);
             }
+            var border = this.get("border");
+            if (border && border["showBorder"]) {
+                shift = shift + border["strokeWidth"];
+            }

             var start = "";
             var end = "";
@@ -1226,6 +1238,10 @@
             shift = bottom_labels.reduce(function(prev, l){
                 return prev + (LINE_HEIGHT * l.size);
             }, shift);
+            var border = this.get("border");
+            if (border && border["showBorder"]) {
+                shift = shift + border["strokeWidth"];
+            }

             var shift_cbar = 0;
             var cb = this.get("colorbar");

snoopycrimecop · 2025-05-14T00:51:08Z

Conflicting PR. Removed from build OMERO-plugins-push#419. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-05-14T05:32:37Z

Conflicting PR. Removed from build OMERO-plugins-push#420. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-05-15T00:40:25Z

Conflicting PR. Removed from build OMERO-plugins-push#421. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-05-16T01:13:22Z

Conflicting PR. Removed from build OMERO-plugins-push#422. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-05-16T08:59:13Z

Conflicting PR. Removed from build OMERO-plugins-push#423. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-05-17T01:02:39Z

Conflicting PR. Removed from build OMERO-plugins-push#424. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-06-16T09:34:34Z

Conflicting PR. Removed from build OMERO-plugins-push#451. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

--conflicts

snoopycrimecop · 2025-08-01T08:06:32Z

Conflicting PR. Removed from build OMERO-plugins-push#491. See the console output for more details.
Possible conflicts:

PR Label on rois #600 Rdornier 'Label on rois'
- src/js/views/panel_view.js
PR Colorbar feature #605 Tom-TBT 'Colorbar feature'
- src/js/views/panel_view.js

~~--conflicts~~ Conflict resolved in build OMERO-plugins-push#531. See the console output for more details.

Rdornier · 2025-09-12T14:24:47Z

Hi @Tom-TBT , "crop page around panel" should be fixed thanks to your above code snippet

will-moore

This is all working fine now. Cropping is good etc.

Rdornier added 3 commits February 3, 2025 15:26

update position of label according to show/hide border

99f6c19

Update labels spacing on PDF/TIFF export

4f90e12

fix flake8 errors

3e104b9

will-moore reviewed Feb 3, 2025

View reviewed changes

Using get() to avoid errors when border is missing

a2369cf

will-moore added the include label Feb 12, 2025

Rdornier added 2 commits February 18, 2025 14:43

fix labels spacing for inner position and multiple labels

38527bf

fix flake8 error

12f7e13

fix render_label call before DOM by will-moore

e559ccd

Tom-TBT reviewed Feb 26, 2025

View reviewed changes

Tom-TBT suggested changes May 13, 2025

View reviewed changes

Merge branch 'ome:master' into fix-labels-on-border

2f65a33

Merge branch 'ome:master' into fix-labels-on-border

83df518

Rdornier added 2 commits September 12, 2025 16:09

fix merge conflicts

31e30ba

fix crop around panel to account for the border, thanks to @Tom-TBT

4df87c2

will-moore approved these changes Sep 23, 2025

View reviewed changes

will-moore merged commit b9486b4 into ome:master Sep 23, 2025

will-moore added this to the 7.3.0 milestone Dec 15, 2025

Conversation

Rdornier commented Feb 3, 2025

Uh oh!

will-moore Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

will-moore commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rdornier commented Feb 18, 2025

Uh oh!

will-moore commented Feb 24, 2025

Uh oh!

Rdornier commented Feb 25, 2025

Uh oh!

snoopycrimecop commented Feb 26, 2025

Uh oh!

Tom-TBT Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

snoopycrimecop commented Feb 27, 2025

Uh oh!

snoopycrimecop commented Feb 28, 2025

Uh oh!

Tom-TBT left a comment

Choose a reason for hiding this comment

Uh oh!

snoopycrimecop commented May 14, 2025

Uh oh!

snoopycrimecop commented May 14, 2025

Uh oh!

snoopycrimecop commented May 15, 2025

Uh oh!

snoopycrimecop commented May 16, 2025

Uh oh!

snoopycrimecop commented May 16, 2025

Uh oh!

snoopycrimecop commented May 17, 2025

Uh oh!

snoopycrimecop commented Jun 16, 2025

Uh oh!

snoopycrimecop commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rdornier commented Sep 12, 2025

Uh oh!

will-moore left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

will-moore commented Feb 17, 2025 •

edited

Loading

snoopycrimecop commented Aug 1, 2025 •

edited

Loading