Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ROI Color Palette #415

Merged
merged 19 commits into from
Aug 16, 2022
Merged

ROI Color Palette #415

merged 19 commits into from
Aug 16, 2022

Conversation

barrettMCW
Copy link
Contributor

@barrettMCW barrettMCW commented Jul 14, 2022

I finally got around to fixing this up. I did a fair bit of testing this time, and as far as I can tell it works as intended.

EDIT (Will): testing: the following config is now on merge-ci and this PR should be included, so the ROI color-picker should correspond to those colors:

omero config set omero.web.iviewer.roi_color_palette "[rgb(0,255,0),rgb(100,50,150)],[darkred,red,pink],[#0000FF, #ff99ff]"
omero config set omero.web.iviewer.show_palette_only true

Screenshot 2022-08-02 at 22 31 20

@jburel
Copy link
Member

jburel commented Jul 15, 2022

flake8.main.application   MainProcess    760 INFO     Found a total of 13 violations and reported 1
/omero-iviewer/plugin/omero_iviewer/iviewer_settings.py:43:1: W293 blank line contains whitespace

README.rst Outdated Show resolved Hide resolved
@will-moore
Copy link
Member

I tried the suggested setting from the README:

$ omero config set omero.web.iviewer.roi_color_palette [rgb(0,0,0),blue],[#000000]
bash: syntax error near unexpected token `('

Then with double quotes:

$ omero config set omero.web.iviewer.roi_color_palette "[rgb(0,0,0),blue],[#000000]"

But this doesn't seem to be parsed correctly (just gives 2 black options + plus the default/current yellow):

Screenshot 2022-07-21 at 14 48 16

I wonder if it would be simpler to use json instead of string for the config value. E,g. https://github.com/ome/omero-web/blob/a34be15e664251bfe5decc1aa8f15a7f4801661b/omeroweb/settings.py#L994

Then setting would be like this (and the parsing happens when you start omero-web):

$ omero config set omero.web.iviewer.roi_color_palette '[["rgb(0,0,0)","blue"],["#000000"]]'

@barrettMCW
Copy link
Contributor Author

barrettMCW commented Jul 21, 2022

That would be mostly correct actually. The yellow shouldn't be there, that must have broke when I changed variable names, I'm looking at it now. But with the config you set both the first items are black. The blue isn't displaying because it's the second item, you'd need to show_palette_only to see the blue. I'll fix the yellow and take another shot at the readme. (I tried to use json, but couldn't get it to work no matter what I did because of some type funkyness)

README.rst Outdated
@@ -73,6 +73,18 @@ If you wish to set a threshold for iviewer that is *lower* than for the server:
NB: Z-projection is not supported for tiled images in OMERO
(Images larger than 2048 * 2048 pixels per plane are tiled in iviewer).

OMERO uses Spectrum Color Picker for selecting ROI colors.
Define rows with brackets, and use commas to seperate values.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "seperate" -> "separate"

README.rst Outdated
OMERO uses Spectrum Color Picker for selecting ROI colors.
The roi_color_palette option allows you to specify a grid of colors for users to choose for ROIs.
Define rows with brackets, and use commas to seperate values.
You can only view the first item of each row without enabling show_palette_only.
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully this is a bit clearer suggestion: "By default, only the first color of each row is shown. A full grid is shown when the default color picker is hidden (see below)".

README.rst Outdated

$ omero config set omero.web.iviewer.roi_color_palette "[rgb(0,255,0)],[darkred,red,pink],[#0000FF]"

Hides the default color picker.
Copy link
Member

Choose a reason for hiding this comment

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

"Hides the default color picker." This seems strange when you're referring to the setting below (this isn't a title).
Maybe: "To hide the default color picker (and show a grid for the color palette), set show_palette_only to true".

@will-moore
Copy link
Member

will-moore commented Aug 1, 2022

I tried using $ omero config set omero.web.iviewer.roi_color_palette "[rgb(0,255,0),rgb(100,50,150)],[darkred,red,pink],[#0000FF, #ff99ff]" but the first row got treated as a single string rgb(0,255,0),rgb(100,50,150)

After a bit of testing, it looks like it fails if you have more than 1 rgb value on any row.

@barrettMCW
Copy link
Contributor Author

Apologies, missed the console.log and I'm bad with words. Good catch, I fixed the parsing regex. Thanks for your help

@will-moore
Copy link
Member

omero.web.iviewer.roi_color_palette=[rgb(0,255,0)],[rgb(0,50,150),darkred,rgb(110,150,0),red],[#0000FF, #ff99ff, rgb(100,50,150)]
omero.web.iviewer.show_palette_only=true

It's looking good now, thanks:

Screenshot 2022-08-01 at 16 57 41

All my testing has been local so far. I'll give it a final go on the merge-ci server tomorrow...

@pwalczysko
Copy link
Member

Works fine on merge-ci. Like the intuitiveness of the feature. LGTM

@jburel
Copy link
Member

jburel commented Aug 15, 2022

As discussed this morning, it will be good to test the code without the configuration described above to see if there is any side effects.

@will-moore
Copy link
Member

@jburel I removed the config on merge-ci (for tomorrow), so if you want to test this PR with config, it would be best to do it today.

@jburel
Copy link
Member

jburel commented Aug 16, 2022

Tested without the setting and the viewer works as expected

@jburel jburel merged commit 14d6c9e into ome:master Aug 16, 2022
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.

4 participants