Add option to adjust Scalebar width

JulianHn commented 1 year ago

Hey all,

This is a first draft to Fix #503

I have added an additional input field to the scalebar_form panel and reordered the inputs to make space. Adjusting the scalebar in the panel_view is done.

Known Issue: Site load show Empty string passed to getElementbyId(). This will also be returned multiple times when the scalebar is shown / hidden. Unclear where this is coming from

RFC: Is that approach in principle okay? If yes, I will try to track done that getElementbyId() thing and take a look on what needs to be modified in the export routines.

// Julian

imagesc-bot commented 1 year ago

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

https://forum.image.sc/t/scalebar-width-adjustement/78827/4

JulianHn commented 1 year ago

PDF export seems to be working as well with modifactions to Figure_to_Pdf.py: Figure_2023-3-23_13-52-0.pdf Figure_2023-3-23_13-51-44.pdf

I haven't fully understand why the TIFF export breaks with the same parameters ... more investigation to do

JulianHn commented 1 year ago

And with 1cd3f56 exporting tiffs now also works on a first glance:

JulianHn commented 1 year ago

After some further research: The Empty string passed to getElementById() warning is also present in current stable omero-figure and occurs exclusively on Firefox. This seems to be a known issue with Firefox and Bootstrap Forms and Dropdown menus and the general consensus seems to be that it is not an actual problem.

E.g: https://community.wix.com/velo/forum/coding-with-velo/text-input-for-forms-generates-empty-string-passed-to-getelementbyid-error-on-load https://bugzilla.mozilla.org/show_bug.cgi?id=1333427

The second link and some other StackOverflow threads often involve data-targets pointing to # as the culprit. I am wondering if the parts of the dropdown menus for units and font size are responsible?

will-moore commented 1 year ago

Oh, and maybe remove the ? from Label?. I think a checkbox is implicitly boolean.

JulianHn commented 1 year ago

Hey @will-moore thanks for the review. Fitting the everything in two rows was no problem, my original thought was separating "label-logic" from "bar-logic".

All other requested changes are implemented and Enter submission is now working again by applying your patch and fixing a css selector.

The label offset in PDF is now consistent with the one in TIFF, I just used the 5 offset that was already implemented there for both now.

PDF:

TIFF:

I have undrafted the PR, as I think it should now have all features and I have done a bit more UI testing without encountering more issues.

// Julian

will-moore commented 1 year ago

I'm trying to get this included in our daily build, but something's preventing that e.g. https://github.com/snoopycrimecop/omero-figure/commit/ac6d07654af2490e77a6a1b9ba5973f4a102fca6 lists PR 504 JulianHn 'Add option to adjust Scalebar width' (status: action_required) But I'm not sure what that action is? cc @sbesson since I've added "include" label to this PR and the build is green...

sbesson commented 1 year ago

The PR inclusion will be conditional to its status. I suspect the issue came from the default requirement to approve GitHub workflows for first-time contributors. Looking at the actions logs, the initial run four days ago was marked as required action and it looks like you approved it and the GitHub workflow ran successfully 2 days ago.

Regarding the commit, note that https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-plugins-push/ did not run at all on March 27th so this PR did not have a change to be included yesterday. The nightly CI run seems to have successfully included it into the merge build as expected - see https://github.com/snoopycrimecop/omero-figure/commit/0948e32c8c1e5d47bcb094d3dc0c543405e5e14f

will-moore commented 1 year ago

@sbesson Ah - thanks, sorry I'd missed that the CI didn't run yesterday.

will-moore commented 1 year ago

@sbesson the last snoopy merge commit (last night) https://github.com/snoopycrimecop/omero-figure/commit/1cd70919c15ddf29b069074c16da72fb6e430bc8 again failed to merge this PR because action_required again, and I see above that I have to Approve and run again, because 1 commit got added?!

I would have expected to only need to approve once?

will-moore commented 1 year ago

Hmmm - now "Approve and run" gives me a 500 error at https://github.com/ome/omero-figure/pull/504/run_workflows so will try again later...

will-moore commented 1 year ago

@sbesson I'm still getting this PR excluded - just now: https://github.com/snoopycrimecop/omero-figure/commit/c6c6834fae45f9e56d3e032e98240416a250b8e4 this time with (status: startup_failure). I don't see anything in the logs at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-plugins-push/1475/console

sbesson commented 1 year ago

Looks like the latest workflow run hit some upstream service unavailability issue

will-moore commented 1 year ago

@sbesson I can't actually work out how to get to that view of the jobs. If I follow the "details" link from the job I get to https://github.com/ome/omero-figure/actions/runs/4542452995/jobs/8034996020?pr=504 but I can't figure out how to get to https://github.com/ome/omero-figure/actions/runs/4542452995/ to see the error (except by editing the URL)!?

That shows:

Node.js 12 actions are deprecated. Please update the following actions to use Node.js 16: actions/checkout@v2.
For more information see: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/.

cc @jburel

sbesson commented 1 year ago

From https://github.com/ome/omero-figure/actions/runs/4542452995/jobs/8034996020?pr=504, can you click on the Latest #3 drop-down then select Latest attempt #3?

will-moore commented 1 year ago

Thank's to @jburel and https://github.com/ome/omero-figure/pull/505 it looks like the build is passing now...

will-moore commented 1 year ago

@JulianHn - Apologies: one more thing... Could you update the JSON model scalebar section at https://github.com/ome/omero-figure/blob/master/docs/figure_file_format.rst This isn't a new version of the model since it's not a breaking change.

NB: I see that 'Format' page is linked from https://omero-guides.readthedocs.io/en/latest/figure/docs/omero_figure_scripting.html but it would be good to have a link from README here too - since I found it hard to trace just now (but that doesn't have to happen in this PR).

Cheers!

JulianHn commented 1 year ago

Just to be sure: Do I just add the single line to the JSON optional settings specification or is there some documentation of all the options I am missing?

Regarding your NB: I actually do not really see much "how to use" in the current README in general. It's more focused on "how to sysadmin / develop" and the "how to use" is not really linked at all. Should the README be extended in general or do you think a prominent link to the readthedocs would be sufficient.

// Julian

will-moore commented 1 year ago

Yes, just add a single line to the JSON sample.

For user guide etc there is a link from the README to https://github.com/ome/omero-figure/blob/master/SUPPORT.md, which then lists other links (which are both re-directs now), so maybe we just need to move the content of that SUPPORT.md onto the front page README and update the links to make it more prominent. But please don't worry about it in this PR - it was just a note-to-self that the doc was kinda hard to find.

will-moore commented 1 year ago

@JulianHn - apologies this has taken a while to get it deployed on our testing server (I was just testing locally before) but it's there now...

I just noticed that when I opened a previously-saved figure, the scalebars don't show up on the figure because the scalebar JSON has no height. And when I try to generate a PDF it fails:

  File "./script", line 1963, in add_panels_to_page
    self.draw_scalebar(panel, pil_img.size[0], page)
  File "./script", line 1437, in draw_scalebar
    half_height = sb['height'] // 2

This should simply use half_height = sb.get('height', 3) // 2 here and anywhere else it reads the height in the script.

Probably the best way to handle this in the app is to increment the VERSION at https://github.com/ome/omero-figure/blob/master/src/js/models/figure_model.js#L4 to 7.

Then add a block below this section, similar to the handling of addition of scalebar units to version 5: https://github.com/ome/omero-figure/blob/master/src/js/models/figure_model.js#L189 to add a default scalebar.height (of 3 I think it was).

Cheers

pwalczysko commented 1 year ago

@JulianHn Sorry, deleted my comment about font and label discrepancy, as it was not introduced by this PR, and created an issue https://github.com/ome/omero-figure/issues/507 instead.

JulianHn commented 1 year ago

Hey @will-moore,

sorry for the delay - I was on vacation / conference the last 1.5 weeks. For some reason it did not cross my mind to save and reload a figure while testing. I'll take a look and add the necessary changes to this PR.

@pwalczysko: While it might not have been introduced here, I can add the necessary changes in this PR, if you prefer the layout in the Labels case. I'll track #507

// Julian

pwalczysko commented 1 year ago

@pwalczysko: While it might not have been introduced here, I can add the necessary changes in this PR, if you prefer the layout in the Labels case. I'll track #507

Thank you very much for this generous suggestion. Yes, we discussed with @will-moore : Let us go for Labels case, this means the tooltip says Font size everywhere (scalebar and labels) and is the same size and style as in the present state in Labels. Definitely not a blocker of course.

will-moore commented 1 year ago

@sbesson it seems I need to "Approve and run" this PR after every commit to avoid it being excluded by merge-ci? Any way to make this a bit easier?

sbesson commented 1 year ago

This is definitely the expected behavior and I don't think there is a built-in feature to automatically approve every new commit - see https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/ for background. Once the PR of a new contributor is accepted, the approval should no longer be mandatory for future contributions so this is a one-off requirement but it will apply to every new commit of the first PR.

will-moore commented 1 year ago

The loading of pre-existing Figure files is working now. 👍 Just one more comment above for the python script. On second thoughts, I don't think #507 has to be fixed in this PR (can follow-up later).

JulianHn commented 1 year ago

I also missed it :) Your suggested patch in #507 is applied to this tree and tested on Chrome and Firefox on Linux Mint 21. Now all the menus have Bootstrap tooltips setup, the only inconsistency is that the tooltip for the Font Size in the scalebar form has a linebreak for me. But I am guessing this depends on the screen size etc. and I don't think it makes the whole form look inconsistent.

will-moore commented 1 year ago

Thanks @JulianHn - looks good 👍

will-moore commented 1 year ago