-
Notifications
You must be signed in to change notification settings - Fork 491
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
Fix the issue with the thumbnail size #10258
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to at least figure out what's going on with the 3rd known bug, before making this PR. To check whether it's new in 6.1; and to at least get an idea of how difficult it would be to fix.
I understand that we may want to open a new issue for it, if it's something difficult/time-consuming - but we don't even know that yet.
Just to be clear, it's not really "sometimes", as in, it's not a mystery of any kind - it appears to be a very clearly/narrowly defined condition: custom logos don't work unless there is at least one image Datafile in the dataset. |
I have right now some Datasets that have images and still don't show the thumbnail, see: It is still not clear to me exactly why or how this happens, since we have a solution and identified these 2 bugs I thought it would be a good idea just to get rid of them and then come with a separate solution. This way we can start doing research and asses what would it take and why did the 3rd bug is happening. Let's keep this PR, right now I will do some final QA on another PR and then come back to this and do some research. For my testing this not only happens when there is no image on the dataset. |
Everything is happening too fast in that gif, lol 😄 |
That is correct as of now is not clear to me why or when the third bug happens. At first glance doesn't seem as straight forward as we initially thought and part of the reason why I thought on separating this from this issue. 😄 Sorry about the speed I will keep it in mind for next one. ⏩ |
@jp-tosca When you have a moment, could you check and post the output of |
Sadly I deleted that installation already and I don't have it anymore since I am testing another PR and I needed to clear the environment multiple times but I will keep everyone updated tomorrow with my testing about this issue. |
FWIW: It looks like dataverse/src/main/java/edu/harvard/iq/dataverse/ThumbnailServiceWrapper.java Lines 173 to 177 in 61f9d8a
boolean hasDatasetLogo = false; StorageIO storageIO = null; try { storageIO = DataAccess.getStorageIO(dataset); if (storageIO.isAuxObjectCached(DatasetUtil.datasetLogoFilenameFinal)) { // If not, return null/use the default, otherwise pass the logo URL hasDatasetLogo = true; } } catch (IOException ioex) { logger.warning("getDatasetCardImageAsUrl(): Failed to initialize dataset StorageIO for " + dataset.getStorageIdentifier() + " (" + ioex.getMessage() + ")"); } // If no other logo we attempt to auto-select via the optimized, native query-based method // from the DatasetVersionService: if (!hasDatasetLogo && datasetVersionService.getThumbnailByVersionId(versionId) == null) { return null; }
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Just to follow up the Issue for the third bug was created and @qqmyers already submitted a PR for it 😃 |
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
The issue is partly fixed by the following commits, which are already part of the v6.2 release: IQSS#10269 IQSS#10258 Had to add "dataset.setUseGenericThumbnail(false);" line in the DatasetServiceBean to make it work.
What this PR does / why we need it:
It was reported on this issue that a thumbnail will be showing the full resolution image on the search/listing of the datasets. During the testing @landreev discovered that there was an issue also with a discrepancy of the resolution of a published vs a draft dataset.
Which issue(s) this PR closes:
Closes #10220
Suggestions on how to test this:
This PR does not address another issue discovered that sometimes a customized thumbnail will not show, this will be investigated on a separate issue. Probably to test be sure to have some datasets with custom thumbnails from a previous installation.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
It should fix the issues caused to the UI reported on #10220