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

Add button to save current view configuration as default in view mode #7205

Merged
merged 27 commits into from
Aug 14, 2023

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jul 13, 2023

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a dataset in view mode (as admin or dataset manager)
  • Change the view mode configuration in a recognizable way (e.g. set histogram min = 100, max = 200)
  • Use the new button at the bottom of the layer configuration tab to save the new configuration
  • A modal should open up for confirmation. If you cancel the action, no update should be performed.
  • If you confirm the modal, a toast should indicate that the updated succeeded
  • Use the crog-icon next to the dataset name at the top of the info tab to open the dataset settings
  • Go to the view configurations and confirm that the update worked successfully.

TODOs:

Issues:


(Please delete unneeded items, merge only when none are left open)

@MichaelBuessemeyer
Copy link
Contributor Author

@normanrz could you please take a better example image that has the same style as the other images from the docs once this pr is merged?

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice functionality 👍 Please see my suggestions and remarks.

I'll report back after testing 🐒

@daniel-wer
Copy link
Member

Regarding my comment #7205 (comment)

I tested which settings are saved and restored when using the new functionality and it seems to be all-of-them (all dataset-related settings). This is in contrast to the existing configuration UI where only the values you mentioned can be modified. We need to decide whether (a) it's fine that the new functionality is more powerful, (b) the existing UI should be extended to have the same capabilities, or (c) the new functionality should be restricted to only persist the values that can be modified in the existing UI.

I made a screenshot of the settings before. Then, I modified all settings, saved using the new functionality and opened the dataset with a new user:

Before After
settings_before settings_after
settings_before_2 settings_after_2

Apart from that inconsistency, the functionality works very well :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Looking very good, with one small issue remaining.

The layer configuration in the dataset view settings doesn't currently allow to change: brightness, contrast, min, max
I think these should be added to the schema so they can be changed (and documentation in the table at the right should be added, too).

@MichaelBuessemeyer
Copy link
Contributor Author

The layer configuration in the dataset view settings doesn't currently allow to change: brightness, contrast, min, max
I think these should be added to the schema so they can be changed (and documentation in the table at the right should be added, too).

I am pretty sure that brightness and contrast were replaced with min, max, intensityRange and thus they should not be configurable. But you are right with min and max. They are already configurable via the json layer config in the UI and are now also added to the table.

As discussed I reworked the UI and made the intensityRange setting optional / required for color layers.
As this change required some changes in multiple files, it would be great if you could look over them to avoid accidental bugs 🙏

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Thank you for the newest improvements. I have a couple of small remarks, but afterwards this PR should be ready to be merged 👍

Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for the review daniel. I hope everything should be fine now.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 👍

@MichaelBuessemeyer
Copy link
Contributor Author

After I tested the last TODO, I'll merge 👍

Test whether this pr works well together with the new layer ordering pr during cover mode for color layers. #7188

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Aug 3, 2023

Sadly, (but obviously) this pr did not work with #7188 out of the box. I had to add a missing form item that enables the user to manipulate and save the order of the color layers.
As an approach, I used the same dnd lib as used in #7188 to create a list of sortable color layer names via dnd.
Sadly this also implies that I had to add another settings item to the UI. This means by the current layout that a new row only for a single setting needed to be added.

The current version looks like this:
image
(Below the part of the image is a divider and then the json settings input for each layer. I excluded this in the screenshot.)
As the list takes up a lot of space on the screen, I wrapped it in a collapsable and hide it as default. The collapsible is also only extendable when having more than one color layer.

@philippotto @daniel-wer Do you think this UI is okayish enough? / Do you have suggestions? Maybe some horizontal line between the list elements?

@philippotto
Copy link
Member

@philippotto @daniel-wer Do you think this UI is okayish enough? / Do you have suggestions? Maybe some horizontal line between the list elements?

I think, it's alright if it is collapsed by default 👍 Alternatively, one could use the layer order from the JSON, but this is quite indirect (would need a tooltip or something similar) and annoying to change. So, I'd say, stick to your current way :)

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Aug 10, 2023

@philippotto Could you take over the review? Imo you should only check the new color layer order setting I added in the last commits and test whether the feature works on a deployed test instance.

@philippotto
Copy link
Member

Code looks good, but I couldn't change the layer order for this DS: https://updatedefaultlayerconfiginviewmode.webknossos.xyz/datasets/sample_organization/idr0101A-4d/edit
even though it has multiple color layers?

@@ -71,6 +98,7 @@ export const enforceValidatedDatasetViewConfiguration = (
: _.pickBy(layerConfigDefault, (value: any) => value !== null);
}
});
ensureDatasetSettingsHasLayerOrder(datasetViewConfiguration, dataset);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code looks good, but I couldn't change the layer order for this DS: https://updatedefaultlayerconfiginviewmode.webknossos.xyz/datasets/sample_organization/idr0101A-4d/edit
even though it has multiple color layers?

Thanks for finding this bug 🔍.

This line should fix the bug (according to my testing).
The problem was I added the new property colorLayerOrder (see #7188) and that it is expected to be always present in a validated DatasetConfiguration. But the backend does not send this property if it wasn't set by the frontend previously.
Previous to my changes, the property was only enforced during model initialization (see changes in model initialization). But the enforcing of the property with correct values must be done every time when working with a DatasetConfiguration. Thus it made more sense to me, to move the enforcing code to the enforceValidatedDatasetViewConfiguration function.
This covers the bug, as this function is explicitly called in dataset_settings_view.tsx after it is fetched from the backend.
Additionally, the enforceValidatedDatasetViewConfiguration is also enforced for the model initialization as it is part of the getDatasetViewConfiguration API backend call in admin_rest_api.ts which is called during the model initialization to fetch the DatasetConfig from the backend.

Could you please retest the changes (once the CI is done)?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, it still doesn't work for me here? 🤔 Does it work for you?

Copy link
Member

Choose a reason for hiding this comment

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

Great, now it works :)

@philippotto philippotto enabled auto-merge (squash) August 14, 2023 08:15
@philippotto philippotto merged commit 0872f29 into master Aug 14, 2023
1 check passed
@philippotto philippotto deleted the update-default-layer-config-in-viewmode branch August 14, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add button for making view settings default
4 participants