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

Preserve Z/T changes #3718

Closed
wants to merge 2 commits into from
Closed

Preserve Z/T changes #3718

wants to merge 2 commits into from

Conversation

dominikl
Copy link
Member

Fixes Ticket 12709 and also some occasionally appearing problems that thumbnails are not in sync with the rendering settings (unfortunately mixed that altogether in one commit).

Note: With this PR Z/T changes are silently stored, i. e. the user's last selected Z/T frame will be stored in the rendering settings automatically as soon as he moves away from the image (selects a different image, close the image viewer, or just switch from the preview pane to the general pane). Whereas 'real' rendering setting changes (to the channels) are discarded, when moving away from the preview panel.
Is this an acceptable behaviour? Can't treat Z/T changes as 'real' rendering setting changes, because this leads to whole lot of inconsistencies with respect to undo/redo, save, copy/paste, etc.

Test:
For the scope of the ticket:

  • Select an image with multiple Z frames. Change the currently viewed Z frame in the preview pane, switch to the general pane (at this point the current Z frame should be stored and also the thumbnail updating), open the SplitViewFigure dialog (make sure your selected Z frame is shown), create the figure (make sure the figure also shows your selected Z frame).
  • Do the same again but now modifiy the Z frame in the full image viewer.

General:

  • Ideally some more rendering settings workflows (copy/paste, undo/redo, etc.) should be tested as well.

}
} catch (Exception e) {
MetadataViewerAgent.getRegistry().getLogger()
.warn(this, "Could not save renderign settings");
Copy link
Member

Choose a reason for hiding this comment

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

rendering

@dominikl
Copy link
Member Author

Forgot to /cc @gusferguson and @jburel on this, as this would be a noticeable change in user experience.

@will-moore
Copy link
Member

Seems to be working fine with the Split-view figure always respecting the latest Z-section.
Also, the general rendering settings behaviour is unchanged although I haven't done a full test of every scenario.
I think I'm OK with the auto-save of Z-section, but agree that @gusferguson and @jburel should probably check that this doesn't break any other workflows, since it is a significant change.

@gusferguson
Copy link

@dominikl @will-moore @jburel @pwalczysko

Tested using OMERO.insight-5.1.0-452-936e5ef-ice35-b481-mac_Java7+ trout user-3 read-annotate-1.

Behaves as expected and split-view figure uses currently viewed z-section.

This is quite a substantial change in behaviour as it effectively makes the Z-section selection a "super-rendering setting" i.e. the only rendering setting modality that is saved automatically when changed.

I would break it down as follows:

  • Split View Figure uses selected Z section - essential
  • thumbnail updates to show selected Z-section - expected
  • viewer opens with Z-section selected in Preview displayed - intuitive
  • auto save of Z-section section selection to rendering settings i.e. persists beyond that view/session - not so intuitive
  • no ability to actively Save Z-section selection - as exists for other rendering settings changes - dissonant
  • reset to Imported resets Z-section to default - good
  • what do we do with T-points - the same thing or leave them as they are?

I can't see any other workflows that are affected by this change. but it will cause a Insight-Web discrepancy that would need to be addressed before release in my opinion.

What are y'all's thoughts?

@jburel
Copy link
Member

jburel commented Apr 23, 2015

Thanks @gusferguson for the report.
Major changes in behaviour and we do not want to introduce discrepancy between Web/insight.
Further discussions required.
moving it to breaking

@jburel
Copy link
Member

jburel commented Apr 23, 2015

Added card. To be discussed during next client meeting
https://trello.com/c/3H67wdIJ/337-viewer-mouse-wheel-and-save-rendering

@jburel
Copy link
Member

jburel commented May 4, 2015

Comment added to card. @dominikl to look at it on his return.

@dominikl
Copy link
Member Author

Closing. Will open a new PR for just enabling the save button on z/t change.

@dominikl dominikl closed this May 15, 2015
@dominikl dominikl deleted the z_changes branch May 25, 2015 12:52
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.

None yet

6 participants