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

show clipped paper snapshot in fix metadata dialog #4645

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

nschneid
Copy link
Contributor

@nschneid nschneid commented Feb 16, 2025

Re: #4604 This PR extends the paper metadata "Fix data" dialog to show the snapshot of the title/author block followed by a string list of all the edited author names, so the user can easily check if they match. There is a link to the PDF in case the snapshot is missing/insufficient.

image

Copy link

github-actions bot commented Feb 16, 2025

Build successful. Some useful links:

This preview will be removed when the branch is merged.

@nschneid nschneid requested a review from mbollmann February 17, 2025 01:30
@nschneid nschneid marked this pull request as ready for review February 17, 2025 01:41
@mjpost
Copy link
Member

mjpost commented Feb 19, 2025

This is a nice idea. Comments:

  • The rendered author string doesn't update on author reordering
  • Have you tested this on mobile? I'm worried the dialog will be too big
  • The thumbnail and the rendered text need some kind of header and label to demarcate them

Even I was confused at what appeared to be a random name string at the bottom. I think some kind of colored box or well with a label should be displayed to demonstrate that this is the read-only, newly-rendered name string, and that the image displayed is for verification purposes and comes from the paper.

@mjpost
Copy link
Member

mjpost commented Feb 19, 2025

Hmm, looking a lot better, but

  • the name string still doesn't update when names are reordered
  • the PDF on mobile also doesn't scale to width
  • there's a training semicolon

@nschneid
Copy link
Contributor Author

Checked on mobile: the snapshot gets cut off on the right, but clicking on it or clicking the PDF link is a workaround. I guess another option is to make it horizontally scrollable (not sure how).

@mjpost
Copy link
Member

mjpost commented Feb 19, 2025

Ideally it would just scale to the enclosing div width. I bet @mbollmann knows how to make that happen.

@nschneid
Copy link
Contributor Author

Ideally it would just scale to the enclosing div width. I bet @mbollmann knows how to make that happen.

The problem I'm running into is how to crop the image based on its full resolution and then resize the image to fill the space. The cropping techniques I am trying (to remove the top 50px of the image) are computing 50px of the resized image. I guess I could crop as a percentage but only if all the snapshots are the same height.

I think the other issues are fixed.

Copy link
Member

@mbollmann mbollmann left a comment

Choose a reason for hiding this comment

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

Unrelated (?) to this change, when I preview this in Firefox’s responsive design mode, the name boxes look like this even for relatively large displays — below is an emulated iPad display:

image

Hadn’t we fixed this at some point to reduce margins and the width of these boxes on smaller displays, or am I hallucinating that?

Comment on lines 384 to 386
<div style="height: 150px; text-align: center; overflow: hidden; position: relative;">
<a id="paperSnapshot" href=""><img id="paperSnapshotImg" src="" style="position: absolute; left: 0; top: -50px;"></a>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

I played around with this a bit and think something like this might work:

<div style="max-height:150px;" class="overflow-hidden w-100">
  <a id="paperSnapshot" href="https://aclanthology.org/thumb/2024.tacl-1.3.jpg">
    <img id="paperSnapshotImg" src="https://aclanthology.org/thumb/2024.tacl-1.3.jpg" class="w-100" style="transform: translate(0, 5%) scale(1.25);">
  </a>
</div>

Copy link
Member

Choose a reason for hiding this comment

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

I played quite a bit but eventually had too many hours into the project and was happy to have something working. This kind of design isn't my forte. I'll try pushing up this change on this PR.

@mbollmann
Copy link
Member

Hmm, not quite ideal yet. I added the scaling because I thought it makes better use of the space, by eliminating margins, but it looks like we’d just need more vertical space in that case.

image

@mjpost
Copy link
Member

mjpost commented Feb 22, 2025

we could add another CGI script to generate a better cropped JPEG on the fly. Come to think of it, it might be worth changing the thumb thumbnails to work the same way.

@nschneid
Copy link
Contributor Author

I can think of 3 options:

  1. Modify thumbnail generation to chop off the top 50px. Then the rest of the sizing/cropping for the dialog could be done dynamically.

  2. Generate 2 thumbnails per paper, one cropped to focus on the title-author block. The current size (large enough to include the abstract) is still useful for GH issues.

  3. Keep the thumbnails as is and use 2 display modes in the dialog:

    • in desktop mode, display the full width of the snapshot with 50px truncated at the top and visible height truncated to 150px
    • in mobile mode, size the snapshot down to width=100% and say "Click to expand". Clicking opens the snapshot in a modal dialog along the lines of this one

@mbollmann
Copy link
Member

If we do it on the server, we can probably just use ImageMagick. I tried running magick thumb.jpg -trim thumb_trimmed.jpg and it looks just fine.

@nschneid
Copy link
Contributor Author

nschneid commented Mar 2, 2025

@mjpost How easy would it be to run ImageMagick on the server as suggested above? I think that would solve the scaling issues.

@mjpost
Copy link
Member

mjpost commented Mar 2, 2025 via email

@nschneid
Copy link
Contributor Author

nschneid commented Mar 2, 2025

"On the fly" = generating thumbnails on demand? I don't know how easy that would be to implement. I was thinking that the margin-trimmed thumbnails could simply replace the current thumbnails.

@mjpost
Copy link
Member

mjpost commented Mar 4, 2025

Okay, I added a trimmed thumbnail so you can just link to that. The easiest option is just to make another file. It's another ~12 GB on the server. Peanuts. It's running sequentially so might take overnight to build them all.

You can see an example here:
.

@nschneid
Copy link
Contributor Author

nschneid commented Mar 4, 2025

Which looks better—width always at 100%, or max-width of 100%?

For https://preview.aclanthology.org/dialog-snapshot/2020.semeval-1.100/, the former cuts off the last author row on desktop, and is also more pixelated.

Option A:
image

Option B:
image

@mjpost
Copy link
Member

mjpost commented Mar 4, 2025

It seems better to have all the info there, but I like the centered look better

@nschneid
Copy link
Contributor Author

nschneid commented Mar 4, 2025

Option C:
image

@mjpost
Copy link
Member

mjpost commented Mar 4, 2025

I like (c).

Do you want me to generate higher-res icons for the preview? I did smaller ones for the thumbnails, but it would make sense to have higher-res ones here.

The command used to build the thumbnails is:

    convert $file -background white -flatten -resize x600^ -gravity North -crop 600x600+0+10 -colorspace Gray $outfile

I then run convert -trim on this file to produce what you wanted. But we could do this differently.

@nschneid
Copy link
Contributor Author

nschneid commented Mar 4, 2025

Higher-res is worth a try. Then we could do min-width: 80%; max-width: 100%

@mjpost
Copy link
Member

mjpost commented Mar 4, 2025

Okay trying 1024. 2021.lchange-1.8 should be working. Let me know if you want higher.

@nschneid
Copy link
Contributor Author

nschneid commented Mar 4, 2025

https://preview.aclanthology.org/dialog-snapshot/2021.lchange-1.8/ - the resolution looks good but the trimming is now wrong. If it was fixed at 50px then that has to be scaled to match the new resolution I guess.

@mjpost
Copy link
Member

mjpost commented Mar 5, 2025

Hmm. It's higher-res but not trimmed at all. Does this look better? https://aclanthology.org/thumb/2021.lchange-1.8-trimmed.jpg

This was produced with convert 2021.lchange-1.8.pdf[0] -background white -flatten -colorspace Gray -trim -resize x${DIM}^ -gravity north -extent ${DIM}x${DIM} 2021.lchange-1.8-trimmed.jpg

@nschneid
Copy link
Contributor Author

nschneid commented Mar 5, 2025

Looks OK to me—slightly pixelated/truncated at the top, but very readable:

image

Do you want to update all the other thumbnails and we can do some more spot checks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants