ome / omero-figure

An OMERO.web app for creating Figures from images in OMERO
http://figure.openmicroscopy.org
GNU Affero General Public License v3.0
15 stars 30 forks source link

Dynamic labels with regex #472

Closed Tom-TBT closed 1 year ago

Tom-TBT commented 1 year ago

Dear OME team, this pull request concerns labels, and in particular how special values like time, X/Y/Z or image name are handled.

Before, only "time labels" that were specified like [time-hrs:mins:secs] would be dynamically handled . But that worked only if the text of the label was exactly starting like "[time" and ending with an understood format like "-hrs:mins:secs]".

I changed the behavior so that a regex looks for a value between square brackets in the form [property.format]

I implemented the following properties with the given formats:

image

In my opinion, it simplifies the code, were the "[time" special label was handled in many places and thus was harder to extend. In addition, it permits three new things:


From there, if you want to merge my updates, we need first to:


Finally, I found this issue (https://github.com/ome/omero-figure/issues/318) that could be solved by this PR

Thanks for your thoughts/critics/feedback. I'm also not so experienced in collaborating on Github so I apologize in advance if things are a bit messy here.

Cheers! Tom

Tom-TBT commented 1 year ago

I added the support for channels without breaking the previous behavior. If adding "[channels]" as a new label, will create a colored label for each active channel (old behavior).

But if you add anything else to the name, the label isn't recognized anymore and is kept intact for dynamic labels (replacing [channels] in the label.text by the concatenated active-channel names).


I also tested Figure_To_PDF.py (and fixed mistakes I made).

It's far from being an ideal development workflow, but it was enough for the few changes I made in the script.

jburel commented 1 year ago
flake8.main.application   MainProcess   1266 INFO     Found a total of 63 violations and reported 1
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:1113:14: E261 at least two spaces before inline comment
jburel commented 1 year ago

@Tom-TBT Did you try to expand https://github.com/ome/omero-figure/blob/master/test/integration/test_figure_scripts.py? The tests are run by the GitHub action against an omero-server

Let me know if you need any help

Tom-TBT commented 1 year ago

I haven't tried to expand it, but I agree that I should add examples there. Thanks for pointing that out!

will-moore commented 1 year ago

Hi @Tom-TBT and thanks for the PR. I like the idea of using a regex to dynamically create labels, allowing users to add their own text to the dynamic tokens.

OK, I'll stop for now. Apologies for the evolving ideas during this review - lots to think about! Cheers, Will

Tom-TBT commented 1 year ago

Hi @will-moore, I haven't added the code for compatibility with the previous version yet, but here is my progress:

Regarding testing Figure_To_Pdf.py, my issue was that after replacing the script, I had an error saying that the file hash changed and refused to execute. But it works after reloading OMERO.web and opening a new OMERO.figure tab. For the workflow, I use the cli to upload my script on the server with a console opened in my container:

. /opt/omero/web/venv3/bin/activate
cd omero_figure/scripts/omero/figure_scripts
omero script upload Figure_To_Pdf.py --official

(I haven't found yet the command replacing directly omero/figure_scripts/Figure_To_Pdf.py)

Let me know if you have more ideas or updates in mind! Tom

will-moore commented 1 year ago

Wow, thanks. I'm away next week, so I might not test again till after that.

Try to replace script by getting the ID (only have to do this once) then using replace:

omero script list
omero script replace ID omero/figure_scripts/Figure_To_Pdf.py

I think we need to be conservative with the new features that you're adding and only add what you really need, because once you've released it (and exposed it in the UI) and people are using it (or might be using it) then we have to support it test it, document it etc going forwards. If some of this functionality isn't exposed in the UI (no drop-down option) it could still be tested or used as a "hidden feature" but we wouldn't be considered a breaking change for users if we decided not to support it in future.

will-moore commented 1 year ago

I noticed that the PDF export doesn't format the lengths to the same number of decimal places at in the browser (screenshot showing PDF overlaying the browser also shows the timestamp formatting issue mentioned earlier):

Screenshot 2022-08-01 at 12 28 05

will-moore commented 1 year ago

It would be good to document the new behaviour (E.g. that you can use My time: [time.mins] etc) so that other users get the full benefit of these nice new features. A good place to add this info in in the dialog that currently just describes the markdown behaviour (not sure if many people notice that this is a button anyway).

With the diff below, this could become a general labels Info button (or some other text if you prefer) that shows the dialog. Then you just need to update the dialog itself (title "Label Formatting" instead of "Markdown Formatting") to include a note about the dynamic labels options and an example or two.

Screenshot 2022-08-01 at 12 56 06

$ git diff
diff --git a/omero_figure/static/figure/css/figure.css b/omero_figure/static/figure/css/figure.css
index eec7f13..8bd5b9e 100644
--- a/omero_figure/static/figure/css/figure.css
+++ b/omero_figure/static/figure/css/figure.css
@@ -167,16 +167,6 @@
         padding: 3px 15px;
     }

-    .markdown-info {
-        padding: 3px 12px;
-        color: #aaa;
-        text-align: right;
-        padding-left: 40px;
-        background: url(../images/markdown_light32x20.png) 0% center no-repeat;
-    }
-    .markdown-info:hover {
-        background: url(../images/markdown_dark32x20.png) 0% center no-repeat;
-    }
     .legend-container .markdown-info {
         display: none;
     }
@@ -184,12 +174,6 @@
         display: block;
     }

-    #labelsTab .markdown-info {
-        height: 18px;
-        margin: 9px 9px 0;
-        float: left"
-    }
-
     /* hide appropriate collapse/hide button */
     .legend-collapsed .collapse-legend{
         display: none;
diff --git a/omero_figure/templates/figure/index.html b/omero_figure/templates/figure/index.html
index 51c2ee3..9f45a3d 100644
--- a/omero_figure/templates/figure/index.html
+++ b/omero_figure/templates/figure/index.html
@@ -1020,7 +1020,9 @@
                     <h5 style="float: left">Add Labels</h5>

                     <button type="button" class="btn btn-link markdown-info"
-                        title="Use Markdown formatting. Click for more info..."></button>
+                        title="Show label formatting options...">
+                        Info
+                    </button>
                     <div class="clearfix"></div>
                     <form class="new-label-form form-inline" role="form">
                     </form>
Tom-TBT commented 1 year ago

Hey @will-moore, thanks a lot for spotting those issues. List of updates in last commits:

will-moore commented 1 year ago

Thanks for those updates. I'm happy to go with "Tips". The dialog looks good, but I actually missed the lower section as I didn't realise the dialog was scrollable when I opened it (common problem of scrollbars being hidden by default).

Screenshot 2022-08-02 at 11 57 09

It's possible to force the scrollbars to be visible via css, but an easier solution is to boost the height a bit so that it shows part of the next line. e.g.

<div class="modal-body" style="max-height:500px">

Part of the reason I didn't realise that text was hidden was because what's shown is almost enough info. The syntaxes section below, that lists all the options is probably not needed, since we show the important options in the drop-down list. Is the support of various alias terms for each property an important feature? Again, it's more functionality to support, test and document, so if users only know a single way of specifying e.g. time.mins then it keeps things simpler.

For the other options like Rotation, that's up to you what you feel is needed/useful, but I would only add what you need now as you can always add more later, but removing stuff is harder.

I would just say something like: "Certain properties can be inserted in the label (within square brackets) and are dynamically updated upon changes. See the drop-down options for supported properties". Then just list 3 or 4 examples in that table to illustrate the syntax and remove the 2nd table, which is harder to understand than the examples section. This is assuming that the combination of drop-down list and the first table of examples can cover all the options we want to support?

Tom-TBT commented 1 year ago

Ok to remove the detailed doc, and I'll make the features findable either in the dropdown or in the example.

About the alias, I could avoid it but I'm more a fan of the "short" version rather than the verbose one. It wasn't a problem when the label was chosen from the list (like [time.hrs:mins:secs], but now that it can be combined with text, I expect it typed manually more often. The second point is that the label text-field is also small, so the short version could spare some scrolling. Because replacing hrs:min:secs by h:m:s would be a breaking change from v5, I would have to implement it in the conversion to v6 (if I go for short). And some might prefer the verbose version for clarity.... That's why I thought alias would be a good compromise.

About the rotation, I thought of adding it because it wasn't much different from x/y/height/width (the difference is the ° symbol) and I wanted to add different "relevant" features. I don't have the use for it but someone else could.

Tom-TBT commented 1 year ago

Hey Will, here are the changes we talked about.

I chose the odd value of 518px for the dialog height to have a line cropped in half. Do you see the same thing on your side? image

will-moore commented 1 year ago

Opened a fix for the "Legend" button issue noticed today: https://github.com/ome/omero-figure/pull/474

will-moore commented 1 year ago

@Tom-TBT Sorry, one more thing. I wonder if you could update https://github.com/ome/omero-figure/blob/master/docs/figure_file_format.rst to mention the changes in version 6 and update the ["time"] label example. Possibly add another label / comment to cover some of the new label options. Thanks.

pwalczysko commented 1 year ago

Thank you @Tom-TBT , very nice addition.

All works as expected. The only thing which I would invite one more thought is the question "How does a user keep an overview, how does he/she know which features are dynamic and which are not ?"

Screenshot Screenshot 2022-08-09 at 11 16 04

Could we maybe explain about the dynamic vs non-dynamic a bit in the paragraph in the screenshot above ? Also, I would remove the last sentence in the screenshot, and add

See the drop-down options for all supported properties. The table below lists some additional properties not included in the drop-down options.

(this adds for all and tries to explain the range of possibilities. I understand that for example rotation is not listed anywhere, but I guess we can live with that discrepancy)

Tom-TBT commented 1 year ago

Hi @pwalczysko,

I would describe a dynamic property as "a property describing the currently displayed image". That works for x/y/z/width/depth/time/channels/rotation, but the logic breaks with image and dataset.

Image and dataset don't really need to change dynamically. The main benefit is that it allows these two workflows:

The other advantage is that it's easier to combine with text and markdown when it's written "[image.name]" rather than "a_very_long_name_for_images_that_people_tend_to_use.tif"

But it raises the question for [key-value] which isn't replaced dynamically, though looking very well like "image" or "dataset" properties (maybe even more interesting than those two)

What should I do?

Either option is fine for me

pwalczysko commented 1 year ago

leave it like this for now

@Tom-TBT would be what is needed for now imho, but thanks for explaining the details.

I think what would help (and that was the scope of my comment) is to explain the general situation to a naive user in most generic terms (of course only as much as possible). The dynamic vs non-dynamic is definitely too complex to even start on this, so I suggest we leave it (I understood, but the naive user would be confused).

We discussed with @will-moore an addition to the text in the "Tips". @will-moore is going to post exact text soon for you to okay it.

will-moore commented 1 year ago

We discussed a different way of describing the dynamic labels that more closely matches the expected workflow...

"The drop-down menu in the 'Add Labels' form allows you to create labels based on Image metadata. Labels created with square brackets indicate dynamic properties. The properties within the square brackets will be dynamically updated upon changes. Additional text can be added outside the square brackets and will not be affected when the labels are displayed. The following table shows some examples, including the use of dynamic properties and markdown together".

Hope that sounds OK to you?

Tom-TBT commented 1 year ago

Yes that works for me. I updated the dialog and made the updates to figure_file_format.rst

will-moore commented 1 year ago

Hi @Tom-TBT - There'll be a link to this PR in the release changelog - for those wanting more info on what's new... I wonder if you could update the description of the PR since a few things changed (as @pwalczysko found when he started testing, e.g. "depth", "name-..." etc.) Thanks.

imagesc-bot commented 7 months ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/misleading-error-message-in-omero-figure/68642/5