Shared rendering settings #2365
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, it's not possible to share rendering settings since the service methods all limit lookups based on the current user. This test shows the behavior we would like to see: if user "other" does not explicitly generate rendering settings, then none should be created. There may be permissions issues with this that either have to be handled in the clients or be worked around by simplying copying the rendering settings from the image owner (without re-**generating** them)
This may not be correct overall, but it at least lets the previously failing test pass. Where a query was made against `obj.details.owner.id = :user_id` we fall back to using `obj.details.owner.id = some_other_obj.details.owner.id`. Likely this code can be refactored into a single helper method.
|
Pushed another commit that now returns a rendering def during
|
This new test failed before the PixelsImpl method was modified. Now for all groups (not just `rwr--`) the rdef for the owner will be returned if none is found for the current user.
|
Testing set-up:
|
|
Thumbnails View only tests Test 1: private group
Test 2: read-only group
Test 3: read-annotate group
|
|
Thumbnails Tests 1, 2 and 3 work for both insight and web. |
|
Image Viewer: View only tests Test 1: private group
Test 2: read-only group
Test 3: read-annotate group
|
|
Image Viewer Tests 1, 2 and 3 work for both insight and web. |
|
Image Viewer: Save rendering settings tests Test 1: read-only group
Test 2: read-annotate group
|
|
@joshmoore, @will-moore, @chris-allan
Web: not working
Action:review save workflow since we were assuming that the settings were owned by the user looking at the data.. |
|
The logic of the
|
After @jburel's investigation, the remaining issue with this branch looks to be that clients are assuming (rightly) that the rendering def they have in hand is their own. That's always been the case. With this change, that no longer holds. In order to not break the API the intention is: * the old method will throw an exception when users try to save someone else's rdef * the new method will *always* create a new rdef, set it as the current one, and return the new id.
The primary logic from saveCurrentSettings which needs to be avoided are the calls to `new.setId(old.getId())`. `internalSave(boolean saveAs)` uses the saveAs argument to skip over these sections. The added test passes successfully.
To preserve the previous logic in which clients never received RenderingDef instances belonging to other users, we now detect when things have gone amiss and raise an exception. Client developers are responsible for checking the ownership of the RenderingDef in hand and calling the appropriate method.
|
Pushed the suggested logic. |
|
The new settings are created when using Tested with insight only |
|
Added |
In order to not leave clients with NPEs, when saveAs is called we also generate the default 96 thumbnail.
thumbnailExists was broken and: 1) throwing hibernate exceptions on save 2) throwing NPEs during cache lookups This should allow thumbnailExists to work again. We may need a thumbnailExistsByLongestSide method though.
|
Pushed thumbnail generation. |
|
@joshmoore: after recent changes, thumbnails generation does not work anymore cf. Thumbnails View only tests. No Thumbnail available for owner more The settings are correctly saved in DB |
|
Have you pushed your branch anywhere yet? |
|
@joshmoore: not yet reworking some code. |
|
Import a new image, in the blitz log The message generating thumbnail should be different if an error occurred but that is outside the scope of this PR. |
The old method signature was calling itself leading to an infinite recursion. Slightly scary that this wasn't more apparent.
|
Pushed a fix, @jburel. |
|
Retested in all groups and all the workflows which were mentioned in this PR. |
|
But hit a Bug: Workflow:
|
|
@will-moore and @jburel are debugging web atm. It seems that the newly saved settings on the other people's images are not shown in web (as a default or even not at all). Needs to be fixed & retested. |
|
I have found the source of the problem in web. Now I need to investigate why those lines were added in the first place. Problem is in To be discussed with @cneves and @chris-allan implications (authors of the commits) |
Kept specific check if no settings only.
12145 rnd settings
|
I tried to repeat the NPE (#2365 (comment)) , as user-4, and this time the |
|
Problems here in Web. It does not seem to work at all on gretzky. No improvement against yesterday. Workflow:
|
|
I have repeated the Web problem #2365 (comment) with freshly imported images - no changes in behaviour, the bug persists. |
|
When group owner is viewing the same images in Web, no problem there, works as expected. |
|
In Summary: 2 Bugs
|
|
@jburel I wonder why the whole conversation here seems to suggest that the two lines should have been removed, but instead they are being added by your commit ?? Was it supposed to work today after the addition ? |
|
@pwalczysko: I moved its location, which should have allowed to handle the case. |
|
@pwalczysko: found and fixed the problem but I have discovered another serious one. |
Do not delete the owner settings but reset the default for the currently used settings.
12145 rnd settings
|
The reset issue was briefly discussed with @chris-allan on Friday afternoon. |
|
When trying to repeat #2365 (comment), I still get a freeze and Error (after clicking on |
|
@pwalczysko: strange... |
|
|
Merging for inclusion of the rendering setting work in |
This was referenced May 20, 2014
Merged
|
--rebased-to #2539 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See: http://trac.openmicroscopy.org.uk/ome/ticket/12145
This modifies the rendering and thumbnail services to not generate new rendering defs for users viewing someone else's data until a change is requested. This would mean, for example, that for the scenario:
Only when user B modifies the settings in the viewer should a new rendering settings and thumbnail be generated. And in no case should the regeneration do the long re-calculation of min/max, but those should be copied from user A as well.
/cc @jburel @will-moore @chris-allan @pwalczysko @gusferguson @jrswedlow