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

Vite #477

Closed will-moore closed 2 months ago

will-moore commented 1 year ago

See testing sheet at https://docs.google.com/spreadsheets/d/17jQ1Hws42ob-2AYWaO7-272tKdXDacfdzZCk-xk62Z4/edit#gid=0

This is a major update of omero-figure to update all dependencies to their latest versions, including:

Vite gives us a JS-based dev server during development (similar to iviewer).

New dev workflow - see README.

Testing:

Since this work impacts all of the JavaScript and html code, ALL that functionality will need testing:

snoopycrimecop commented 1 year ago

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

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

jburel commented 1 year ago

@will-moore I could modify the job in the new CI to test this PR I have installed node 16

will-moore commented 1 year ago

Not really ready for review yet, but removed the Draft status so it's not excluded by merge-ci...

will-moore commented 4 months ago

@Rdornier @Tom-TBT - This big rewrite PR is getting close to being ready so this is a heads-up...

Tom-TBT commented 4 months ago

Hi Will, here are a few issues I spotted:

for(j = 0; j < lineage.length; j++){ should change to for(let j = 0; j < lineage.length; j++){ I don't know why this PR broke it, but that was an error from me, my apologies.

will-moore commented 4 months ago

@Tom-TBT Thanks for the feedback, I'll look at fixing those. The PR "broke" your code because it's now "strict" mode and it no-longer allows global variables to be declared (without the let to restrict it's scope). This prevents accidentally getting global variables overwriting each other etc.

Rdornier commented 4 months ago

Hello Will,

I'm not able to test it with the new development environnement. How do you configure the CORS ? My localhost:4080 is running but there is no communication with localhost:8080.

Thanks, Rémy.

will-moore commented 4 months ago

@Rdornier The various web install pages have info on enabling CORS e.g. https://omero.readthedocs.io/en/stable/sysadmins/unix/install-web/walkthrough/omeroweb-install-centos7-ice3.6.html#setting-up-cors The https://github.com/adamchainz/django-cors-headers should already be installed as a dependency of omero-web.

When you npm run start you should get the usual OMERO.figure entry dialog at localhost:8080 and if you choose "Open Figure" then it'll try to load from http://localhost:4080/figure/list_web_figures/. You should already be logged-in to the webclient so that if you were to open that URL in a new browser it works (returns JSON). You should see that the figure app tries to load it, and if you've got CORs setup right, it'll be able to read the response (which have correct headers):

Screenshot 2024-02-29 at 17 17 04

If not, then you'll get some CORs error. You can ignore the other failed requests before that (for openwith...).

Tom-TBT commented 3 months ago

Hello Will, I tested the PR again today. The previous bug with the Z projection sliders is fixed, but I saw today that I can pass the "top slider" under the "bottom slider" (this is not the case for the previous OMERO-figure version).

will-moore commented 3 months ago

@Tom-TBT - should be fixed now. The <input type='range'/> sliders that we're using now don't have quite the same behaviour as jQuery-UI sliders. I have fixed the sliding handles past each other, but we're still missing the click-handling on the slider (to slide a handle to where you clicked). Hopefully that's not an essential feature, although it probably could be done...

will-moore commented 3 months ago

@Tom-TBT - I have added click-handling so the range sliders (channels and z-projection) should behave the same as the did before with jQuery sliders - The handle nearest to the point you clicked is moved to where you clicked.

pwalczysko commented 3 months ago

Sorry, deleted the comment which was reporting a bug of copy and paste of ROIs. This was caused by me using a smaller screenshot of the same image - in fact, the behaviour is as expected when pasting onto an image of the same size as was the intention.

pwalczysko commented 2 months ago

Screenshot 2024-04-12 at 14 24 57

Firefox, Mac M1 - ^^^ these sliders look strange, the left-hand side min slider

pwalczysko commented 2 months ago

Screenshot 2024-04-12 at 14 38 05

Firefox, Mac, any image ^^^ Edit labels header is a bit squished into the box above it - this is different than without this PR, which is Screenshot 2024-04-12 at 14 39 47

Could a proper spacing be added ?

will-moore commented 2 months ago

@pwalczysko - thanks for the testing.

Slider handle positions (https://github.com/ome/omero-figure/pull/477#issuecomment-2051771296) was actually caused by a different PR giving wrong values for min (-2147483648), so the slider minimum value was very low, causing the handles to overlap at the top. See https://github.com/ome/omero-web/pull/536#issuecomment-2051902359 (fixed now, but will need image panels to be removed or updated since they will still contain the wrong values).

The thick sliders in Firefox is fixed by b7cfdfa above and the Edit Labels spacing is also fixed now.

pwalczysko commented 2 months ago

fixed now, but will need image panels to be removed or updated since they will still contain the wrong values).

You mean panels in pre-existing figures ? i.e. when a figure was created on merge-ci after the https://github.com/ome/omero-web/pull/536 was open, but before your fix https://github.com/ome/omero-web/pull/536#issuecomment-2051902359, then the panels contain wrong values, do i get it right ? Panels updated or created before or after this period should contain correct values ?

pwalczysko commented 2 months ago

The thick sliders in Firefox is fixed by b7cfdfa above and the Edit Labels spacing is also fixed now.

Confirming this fix.

Slider handle positions

Yes, these are fixed. Really not sure about the value remark https://github.com/ome/omero-figure/pull/477#issuecomment-2061886758

pwalczysko commented 2 months ago

Really cannot say that this is caused by this PR, but on merge-ci (ie. not for example on outreach), when I open a Figure it opens in a new tab (expected). But, only on merge-ci, this new tab is not marked with the OMERO logo.

merge-ci (webclient tab leftmost, OMERO.figure tabs to the right of it) Screenshot 2024-04-17 at 19 01 04

outreach server (both webclient tab and figure tab to its right have OMERO logo):

Screenshot 2024-04-17 at 19 09 50

Edit: iviewer displays the same behaviour ^^^, probably ignore.

will-moore commented 2 months ago

The favicon.ico is configured by the omero-web framework itself and not by the apps (iviewer/figure) at https://github.com/ome/omero-web/blob/02bd670ca7628ecb293108f394ef062a66a7beb4/omeroweb/settings.py#L867. I'm not sure why it's not working on merge-ci but it's not related to this PR.

will-moore commented 2 months ago

Re: https://github.com/ome/omero-figure/pull/477#issuecomment-2061886758 - as discussed, you are correct that this was a temporary bug caused by https://github.com/ome/omero-web/pull/536 and won't affect any users.

pwalczysko commented 2 months ago

Re: #477 (comment) - as discussed, you are correct that this was a temporary bug caused by ome/omero-web#536 and won't affect any users.

Understood, thank you.

jburel commented 2 months ago

Merging so we can move forward