Skip to content

Fix scalebar position#637

Merged
will-moore merged 21 commits into
ome:masterfrom
Rdornier:fix-scalebar-position
Dec 18, 2025
Merged

Fix scalebar position#637
will-moore merged 21 commits into
ome:masterfrom
Rdornier:fix-scalebar-position

Conversation

@Rdornier
Copy link
Copy Markdown
Contributor

Fixes #613

The margin can be set by the user (default 5%), as well as the unit (% or pixel)

image

@jburel jburel requested a review from Tom-TBT May 9, 2025 12:08
@Tom-TBT
Copy link
Copy Markdown
Contributor

Tom-TBT commented May 13, 2025

Is it necessary to have both the pixel and % option? I would use percentage in most cases, because I don't need to change it when resizing the image. What would be a good use case for pixel?
edit: I guess the use case is the one you show with wide VS tall image. Could that be tackled by saying the % always takes the longest edge?

If there's no good reason for it, I'd keep the % only, because the UI gets crowded. With the colorbar coming, adding more lines means that a scrollbar would appear. Without the option, it could fit in the line above.

I got this
image

with this diff (I did not took care of the logic behind between % and pixel)

diff --git a/src/templates/scalebar_form.template.html b/src/templates/scalebar_form.template.html
index 83028407..cc390136 100644
--- a/src/templates/scalebar_form.template.html
+++ b/src/templates/scalebar_form.template.html
@@ -17,7 +17,7 @@
 <form class="scalebar_form">
     <div class="row gy-2 gx-3 align-items-center" style="--bs-gutter-x: 0.3rem;">
         <div class="col-auto input-group-sm">
-            <label class="col-form-label col-form-label-sm" style="width:50px">Length</label>
+            <label class="col-form-label col-form-label-sm" style="width:40px">Length</label>
         </div>
         <div class="col-auto input-group-sm">
           <input type="number" class="scalebar-length input-sm form-control"
@@ -107,24 +107,38 @@
             <% } %>
         </div>
     </div>
+
     <div class="row gy-2 gx-3 align-items-center" style="--bs-gutter-x: 0.3rem;">
         <div class="col-auto input-group-sm">
-            <label class="col-form-label col-form-label-sm" style="width:50px">Height</label>
+            <label class="col-form-label col-form-label-sm" style="width:40px">Height</label>
         </div>
         <div class="form-group col-auto scalebar_length input-group-sm">
             <input type="number" class="label-text scalebar-height form-control input-sm"
-            placeholder="Height" value="<%= height %>" style="width:52px" />
+            placeholder="Height" value="<%= height %>" style="width:35px" />
         </div>
-        <div class="col-auto input-group-sm" style="margin-right: 10px;">
+        <div class="col-auto input-group-sm" style="margin-right: 5px;">
             <label class="col-form-label col-form-label-sm">px</label>
         </div>

+
+
+        <div class="col-auto input-group-sm">
+            <label class="col-form-label col-form-label-sm" style="width:50px; padding-left: 5px;">Margin</label>
+        </div>
+        <div class="form-group col-auto scalebar_length input-group-sm">
+            <input type="number" class="label-text scalebar-margin form-control input-sm"
+            placeholder="Margin" value="<%= margin %>" style="width:35px" />
+        </div>
+        <div class="col-auto input-group-sm" style="margin-right: 2px;">
+            <label for="percentage" class="col-form-label col-form-label-sm">%</label>
+        </div>
+
         <div class="col-auto input-group-sm">
             <label class="form-check-label">Label</label>
             <input type="checkbox" class="scalebar_label form-check-input" <% if (show_label) print("checked") %> />
         </div>

-        <div class="col-auto btn-group" style="padding: 0; width: 50px;">
+        <div class="col-auto btn-group" style="padding: 0; width: 40px;">
             <button type="button" class="scalebar_font_size btn btn-outline-secondary btn-sm dropdown-toggle" title="Font Size"
             data-bs-toggle="dropdown" style="width:33px; <% if (!show_label) print("display:none") %>">
                 <span class='dropdown_icon'><%= font_size %></span>
@@ -141,30 +155,4 @@
         </div>
     </div>

-    <div class="row gy-2 gx-3 align-items-center" style="--bs-gutter-x: 0.3rem;">
-        <div class="col-auto input-group-sm">
-            <label class="col-form-label col-form-label-sm" style="width:50px">Margin</label>
-        </div>
-        <div class="form-group col-auto scalebar_length input-group-sm">
-            <input type="number" class="label-text scalebar-margin form-control input-sm"
-            placeholder="Margin" value="<%= margin %>" style="width:52px" />
-        </div>
-
-        <div class="col-auto  input-group-sm">
-            <input type="radio" name="scalebar-margin-unit" id="pixel" class="scalebar_margin_unit"
-            value="px" <% if (margin_unit == "px") print("checked") %>/>
-        </div>
-        <div class="col-auto input-group-sm" style="margin-right: 10px;">
-            <label for="pixel" class="col-form-label col-form-label-sm">px</label>
-        </div>
-        <div class="col-auto  input-group-sm">
-            <input type="radio" name="scalebar-margin-unit" id="percentage" class="scalebar_margin_unit"
-            value="%" <% if (margin_unit == "%") print("checked") %>/>
-        </div>
-        <div class="col-auto input-group-sm" style="margin-right: 10px;">
-            <label for="percentage" class="col-form-label col-form-label-sm">%</label>
-        </div>
-
-    </div>
-
 </form>

We could also wait for the colorbar PR to be merged and refine the UI then.

@Rdornier
Copy link
Copy Markdown
Contributor Author

Is it necessary to have both the pixel and % option? I would use percentage in most cases, because I don't need to change it when resizing the image. What would be a good use case for pixel?

Beacuse of this example. Different parts of tissues are used in insets, same magnification and same height, to have a nice layout at the end. Users are expected to have the scalebar at the same position as well. For that, I think the pixel option is necessary.

image

@Tom-TBT
Copy link
Copy Markdown
Contributor

Tom-TBT commented May 15, 2025

Ok that's a compelling example, makes sense why you would need both. Functionality wise, lgtm. I'm only concerned about the UI.

I still think it should fit in two lines, but it's getting hard. Maybe aligning radio buttons vertically does the trick:
image

Because it's also "multi panel alignment", I wondered if it could be handled at the figure level (instead of the panel level). But it's not a good idea to scatter the scalebar stuff in different places...

@will-moore
Copy link
Copy Markdown
Member

Thanks for your work on this. I think that we could ONLY support px and not %. The percent is less predictable (we have the same problem with setting out grids with a gap of % width etc. and I think users can do all they need with just px?

@Rdornier
Copy link
Copy Markdown
Contributor Author

Hi,

I try to make things fitting in two lines, but it looks a bit squeezed.

I think that we could ONLY support px and not %.

I gave the choice between px and % in order to be backward compatible. But maybe we'll have to remove % to make UI nicer... I don't know what is the best solution here.

Rémy.

@Tom-TBT
Copy link
Copy Markdown
Contributor

Tom-TBT commented Jul 9, 2025

The Margin input needs to be larger, two digits isn't fitting. With pixels I could expect values larger than 100.

I played around with the CSS. The main change is the flex display for the radio buttons. The left and right margin & padding set to 0 is to get the "units" closer to the input fields, so I could make the Margin input field larger with more space on the left of "Margin" label

diff --git a/src/templates/scalebar_form.template.html b/src/templates/scalebar_form.template.html
index b915cbc0..b7399a9d 100644
--- a/src/templates/scalebar_form.template.html
+++ b/src/templates/scalebar_form.template.html
@@ -107,7 +107,7 @@
             <% } %>
         </div>
     </div>
-    <div class="row gy-2 gx-3 align-items-center" style="--bs-gutter-x: 0.3rem;">
+    <div class="row gy-2 gx-3 align-items-center" style="--bs-gutter-x: 0.3rem; margin-top: 0px;">
         <div class="col-auto input-group-sm">
             <label class="col-form-label col-form-label-sm" style="width:40px">Height</label>
         </div>
@@ -115,28 +115,30 @@
             <input type="number" class="label-text scalebar-height form-control input-sm"
             placeholder="Height" value="<%= height %>" style="width:30px" />
         </div>
-        <div class="col-auto input-group-sm" style="margin-right: 5px;">
+        <div class="col-auto input-group-sm" style="margin-right: 0px; padding-left: 0px;">
             <label class="col-form-label col-form-label-sm">px</label>
         </div>

-        <div class="col-auto input-group-sm">
+        <div class="col-auto input-group-sm" style="margin-left: 6px;">
             <label class="col-form-label col-form-label-sm" style="width:40px">Margin</label>
         </div>
         <div class="form-group col-auto scalebar_length input-group-sm">
             <input type="number" class="label-text scalebar-margin form-control input-sm"
-            placeholder="Margin" value="<%= margin %>" style="width:30px" />
+            placeholder="Margin" value="<%= margin %>" style="width:38px; padding-left: 3px; padding-right: 3px;" />
         </div>

-        <div class="col-auto  input-group-sm">
-            <input type="radio" name="scalebar-margin-unit" id="pixel" style="vertical-align: bottom;" class="scalebar_margin_unit"
-            value="px" <% if (margin_unit == "px") print("checked") %>/>
-            <label for="pixel" class="col-form-label col-form-label-sm" style="margin-bottom: -5px;">px</label><br/>
-            <input type="radio" name="scalebar-margin-unit" id="percentage" style="vertical-align: top;" class="scalebar_margin_unit"
-            value="%" <% if (margin_unit == "%") print("checked") %>/>
-            <label for="percentage" class="col-form-label col-form-label-sm" style="vertical-align: top; margin-top: -8px; font-size: 12px;">%</label><br/>
+        <div class="col-auto input-group-sm" style="display: flex; align-items: center; gap: 0px; flex-direction: column; align-items: flex-start; padding-left: 0px;">
+            <div style="display: flex; align-items: center;">
+                <input type="radio" name="scalebar-margin-unit" id="pixel" class="scalebar_margin_unit" value="px" <% if (margin_unit == "px") print("checked") %> />
+                <label for="pixel" class="col-form-label col-form-label-sm" style="font-size: 13px; margin-bottom: 0; margin-right: 5px; padding: 0px">&nbsp;px</label>
+            </div>
+            <div style="display: flex; align-items: center;">
+                <input type="radio" name="scalebar-margin-unit" id="percentage" class="scalebar_margin_unit" value="%" <% if (margin_unit == "%") print("checked") %> />
+                <label for="percentage" class="col-form-label col-form-label-sm" style="font-size: 12px; margin-bottom: 0; padding: 0px">&nbsp;%</label>
+            </div>
         </div>

-        <div class="col-auto input-group-sm">
+        <div class="col-auto input-group-sm" style="padding-left: 0px;">
             <label class="form-check-label">Label</label>
             <input type="checkbox" class="scalebar_label form-check-input" <% if (show_label) print("checked") %> />
         </div>
@@ -153,7 +155,7 @@
             }); %>
             </ul>
         </div>
-        <div class="form-group col-auto scalebar_text" style="padding-left: 2px;">
+        <div class="form-group col-auto scalebar_text" style="padding-left: 0px;">
             <% if (show_label) print("pt") %>
         </div>
     </div>

Before
image

After
image

@Rdornier
Copy link
Copy Markdown
Contributor Author

Thanks @Tom-TBT for reviewing.
The changes you are proposing fit better in the UI

</div>
<div class="form-group col-auto scalebar_length input-group-sm">
<input type="number" class="label-text scalebar-margin form-control input-sm"
placeholder="Margin" value="<%= margin %>" style="width:38px; padding-left: 3px; padding-right: 3px;" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that we can reduce the width of this input to 30px and it's still wide enough for 2 digits. This fixes an issue I'm seeing with the row being too long and overflowing:

Screenshot 2025-07-28 at 16 34 18

@will-moore
Copy link
Copy Markdown
Member

Apologies for the delayed response. I also just read the comments above and see that @Tom-TBT asked for the Margin input to allow for 3 or more digits!
It feels like the css layout we are using is a little too brittle, so that subtle differences in browsers / defaults causes problems like my overflow screenshot above, but I don't know an easy fix for that.

@will-moore
Copy link
Copy Markdown
Member

Do we want to keep the % option for Margin? Does it have any value other than for backwards compatibility?

We could get around the backwards compatibility issue by calculating the px equivalent (5 % of max(width, height)) when doing the upgrade step and then we just work in px.

The only minor issue I see is if you have a bunch of different sized panels that were previously all 5 % margin, then they would end up with different Margin px values, so that if you then resized them all to be the same size, and aligned them, you'd notice that the scalebars would have different margins. But this would be easily fixed by selecting all and setting the same margin px for all.

Ah - I just tried that and there's a bug (not coming from this PR) that I think I've noticed before...
If you select different values for E.g scalebar Height or Length (or most other attributes) for scalebars of 2 panels, then you select the panels, the form shows a placeholder e.g. Height instead of a value. Now if you change something in the scalebar form (e.g. colour) then ALL the forms inputs are applied to ALL the scalebar attributes, so you get an invalid/null Height (or Length etc) for all the scalebars.
To fix this, we need to update scalebar like channel - with panel_model.save_channel: function(cIndex, attr, value) only saving the changed attributes.

@Rdornier
Copy link
Copy Markdown
Contributor Author

Hi @will-moore

Thanks for your inputs.

I found that we can reduce the width of this input to 30px and it's still wide enough for 2 digits. This fixes an issue I'm seeing with the row being too long and overflowing:

I reduce a bit the input size to keep 3 digits instead of 4.

Ah - I just tried that and there's a bug (not coming from this PR) that I think I've noticed before... [...] To fix this, we need to update scalebar like channel - with panel_model.save_channel: function(cIndex, attr, value) only saving the changed attributes.

Sounds like a good idea but I wasn't able to separate the different attributes. There is a form that is submitted for scalebar lenght, color, position and height and the keyup event prevents me to know which attribute was changed (I don't say it is not possible to separate, but I couldn't find a way to do it). Instead, I added a NaN check so that the corresponding attribute is not updated.

Do we want to keep the % option for Margin? Does it have any value other than for backwards compatibility?

From my side, we can keep px unit only and implement the trick you describe to be backward compatible but this is a subjective opinion. I don't see scenario where % is necessary but maybe @Tom-TBT can add more inputs on that, as he was more in favor of % than px.

@snoopycrimecop
Copy link
Copy Markdown
Member

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

--conflicts

@snoopycrimecop
Copy link
Copy Markdown
Member

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

--conflicts

@snoopycrimecop
Copy link
Copy Markdown
Member

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

--conflicts

@will-moore
Copy link
Copy Markdown
Member

@Tom-TBT Do you have a strong feeling on whether we need to keep % for scalebar margin?

I think I'm generally in favour of doing the simplest thing, instead of trying to support all possible usages. Supporting more options leads to more complex UI etc, which makes it harder for users and developers.

If we can just support px (same as colorbar margin or panel border or font-sizes etc) then let's just do that. The only reason for % is for historical reasons, but if we keep it now then we'll have to support it forever! We have 2 options for the upgrading:

  • As I suggested above, we can calculate what the existing 5% border is in pixels, based on the the size of the panel
  • OR, we can assume that if users never got to choose their scalebar margin before, then they don't really care what the exact margin is and we can just pick a default number e.g. 10 px for all scalebars. This avoids the issue above where every panel will be given a different scalebar margin based on it's current size.

I think I prefer the 2nd option. The default number will also be the margin for all new scalebars. Then, ALL scalebars will be consistently aligned by default, addressing the main issue above. And users can easily choose a different margin if they want. Thoughts?

@Tom-TBT
Copy link
Copy Markdown
Contributor

Tom-TBT commented Sep 24, 2025

Sorry I missed on replying to Rémy. No I'm not strongly in favour of keeping %. I see your arguments and I think pixel alone can handle use cases fine.

Then there's good and bad in both implementation you suggest. But setting it 10px makes it less confusing than an "random" & panel-dependant value.

@Rdornier
Copy link
Copy Markdown
Contributor Author

Rdornier commented Sep 24, 2025

Ok sounds good for me 👍 . Let's just remove % and fix default 10px margin. Thanks for your inputs

@Rdornier
Copy link
Copy Markdown
Contributor Author

Should be done. Please tell me if everything works correctly. Thanks:

}
if (p.scalebar && !p.scalebar.margin_unit) {
p.scalebar.margin_unit = '%';
p.scalebar.margin = 10;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think !p.scalebar.margin should instead check if p.scalebar.margin == undefined because I think if the margin is 0 then !p.scalebar.margin will be True (assuming 0 is a valid margin)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but shouldn't we do the same also with scalebar height (line 284) ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm - possibly, although you could argue that a scalebar height of 0 is not valid, whereas a margin of 0 is possibly valid (but still unlikely).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see... fair enough. Let's correct only the margin

Comment thread src/js/views/scalebar_form_view.js Outdated
json.height = json.height || 3;
json.margin = json.margin || 5;
json.margin_unit = json.margin_unit || '%';
json.margin = json.margin || 10;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this won't support a margin of 0;
I think you want json.margin = json.margin ?? 10; https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

Comment thread docs/figure_file_format.rst Outdated
"margin": 5,
"margin_unit": "%"
"margin": 10,
"margin_unit": "px"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we need margin_unit anywhere now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I forget to check this file.

Comment thread src/js/views/panel_view.js Outdated
sb_json.length = sb.length;
sb_json.height = sb.height;
sb_json.margin = sb.margin;
sb_json.margin_unit = sb.margin_unit;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need margin_unit here now?

@snoopycrimecop
Copy link
Copy Markdown
Member

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

--conflicts

@snoopycrimecop
Copy link
Copy Markdown
Member

snoopycrimecop commented Nov 26, 2025

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

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

@jburel jburel closed this Dec 2, 2025
@jburel jburel reopened this Dec 2, 2025
@snoopycrimecop
Copy link
Copy Markdown
Member

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

--conflicts

@will-moore will-moore added this to the 7.3.0 milestone Dec 15, 2025
@snoopycrimecop
Copy link
Copy Markdown
Member

snoopycrimecop commented Dec 17, 2025

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

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

@Rdornier
Copy link
Copy Markdown
Contributor Author

Conflicts are fixed.
Ready to merge

Copy link
Copy Markdown
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing on new merge-ci after conflicts resolved. Looking good, thanks.

@will-moore will-moore merged commit 88b01ef into ome:master Dec 18, 2025
1 check passed
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.

Scalebar not aligned on images with different width

5 participants