Implement book cover feature #12198

damgam0288 · 2024-11-17T12:10:56Z

Description

This PR is a first iteration of the feature request for a book cover in JabRef in issue 10120.

Changes

When users add an image as a File for a Book, In-book or Booklet citation with the description "cover", it will appear on the right side of the Preview panel.

Code changes

`BibEntry.java`

getCoverImageFile method - searches the Entry's files for an image with the COVER_TAG in the description
COVER_TAG string field - the text that users must include in the file's description to be detected as the book cover
isCoverable method - checks if given Entry type can have a cover image COVERABLE_TYPES
COVERABLE_TYPES field - a list of BibEntry types that can have a book cover (at the moment, this is Book, In-book and Booklet)

`PreviewViewer.java`

setPreviewText method - edited the HTML to include an img tag for the book cover
getBookCoverURI method - supplies the source for the img tag mentioned above

`BibEntryTest.java`

getCoverImageReturnsEmptyIfNoFiles
getCoverImageReturnsEmptyIfNoImageFiles
getCoverImageReturnsEmptyIfEntryIsNotCoverable
getCoverImageDoesNotReturnImagesWithoutCoverDescription
getCoverImageDoesNotReturnDocumentsWithCoverDescription
getCoverImageReturnsCorrectImage
getCoverImageUpdatesWithChangeToDescription

Notes

I am not sure how to approach allowing the user to toggle the book cover on/off because it is part of the HTML in setPreviewText method.
I have implemented this feature initially by relying on the user to select the image but I would like to use the book cover API as per the issue discussion and would appreciate some advice on how to do this without harming the code-base.

Mandatory checks

I own the copyright of the code submitted and I licence it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
N/A Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
N/A Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

…t to URI

…ion. Add tests for this.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.

You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

damgam0288 · 2024-11-29T11:13:54Z

@subhramit May I have your help please - is there anyway to run CheckRewrite without pushing to the branch?

I am not sure what part of my code is causing it to fail - the report on GitHub says BibEntryTest, but when running it in Gradle locally, it says there's an error in Layout.java.

So I am trying to fix any issues in BinEntryTest by pushing to the branch and checking the report, but I think I am cluttering up the commit history unnecessarily in my attempts to find the problem.

Thank-you! :)

subhramit · 2024-11-29T11:19:45Z

@subhramit Can you help please - is there anyway to run CheckRewrite without pushing to the branch? I am not sure what part of my code is causing it to fail - the report says BibEntryTest - I am trying to fix it but I think I am cluttering up the commit history unnecessarily in my attempts to find the problem.

Run the command .\gradlew rewriteRun
Or double click the gradle task as per https://devdocs.jabref.org/code-howtos/faq.html#failing-openrewrite-tests

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

subhramit · 2024-11-29T11:46:41Z

@damgam0288 right now, checkstyle test is red as well. You can take help from the same FAQ page about what to do. You have to have your IntelliJ configured for the local checkstyle warnings to see which part of the code is causing it.
For openrewrite, running the command/task is sufficient to fix it. You can ping on Gitter as well if you have further issues.

damgam0288 · 2024-11-29T11:50:39Z

@damgam0288 right now, checkstyle test is red as well. You can take help from the same FAQ page about what to do. You have to have your IntelliJ configured for the local checkstyle warnings to see which part of the code is causing it. For openrewrite, running the command/task is sufficient to fix it.

No worries, that checkstyle error was a result of a previous trial-and-error attempt with OpenRewrite - it's an unused import I will fix with my next commit. Thank you so much for the help with OpenRewrite :) - I was using terminal to run gradle so it wasn't working correctly. I found the errors and I'm working on them now.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

subhramit · 2024-11-30T06:05:22Z

Please revert your last commit, and use the gradle sidebar (double click on the task) to fix it. Your JDK is outdated, so using rewriteRun is affecting multiple classes.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

subhramit · 2024-11-30T06:56:57Z

I removed it from the drafts, hope that's fine?

damgam0288 · 2024-11-30T08:03:25Z

I removed it from the drafts, hope that's fine?

Of course, no problem 👍

Sorry I missed your last message but I think I fixed the rewrite issues? Or is it still a problem?

subhramit · 2024-11-30T08:10:29Z

Sorry I missed your last message but I think I fixed the rewrite issues? Or is it still a problem?

No, all good now. Thanks.

damgam0288 · 2024-11-30T09:10:35Z

I am re-writing my tests to suit OpenRewrite, then will re-request review

subhramit · 2024-11-30T09:13:53Z

I am re-writing my tests to suit OpenRewrite, then will re-request review

You don't have to manually "suit" openrewrite when coding. As long as your code works, running the task will refactor it as per openrewrite. You can then just commit and push changes.

damgam0288 · 2024-11-30T11:08:41Z

@subhramit The following test was causing an IndexOutOfBoundsException from OpenRewrite despite executing successfully. I tried my best but I couldn't fix it; I have omitted it for now. It might be a bug because I found this issue on OpenRewrite's repository. Any suggestions on what to do? Thank-you.

@Test
  void getCoverImageUpdatesWithChangeToDescription() {
      List<LinkedFile> files = new ArrayList<>();

      files.add(new LinkedFile("cover", Paths.get("JabRef-icon-128.png"), "PNG image"));
      files.add(new LinkedFile("", Paths.get("JabRef-icon-64.png"), "PNG image"));
      LinkedFile cover1 = files.get(0);
      LinkedFile cover2 = files.get(1);

      BibEntry entry = new BibEntry(StandardEntryType.Book).withField(StandardField.AUTHOR, "value");
      entry.setFiles(files);

      assertEquals(Optional.of(cover1), entry.getCoverImageFile());

      cover1.setDescription("");
      cover2.setDescription("cover");
      entry.setFiles(files);

      assertEquals(Optional.of(cover2), entry.getCoverImageFile());
  }

The error:

Error while visiting src\test\java\org\jabref\model\entry\BibEntryTest.java: java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
  java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:100)
  java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:106)
  java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:302)
  java.base/java.util.Objects.checkIndex(Objects.java:365)
  java.base/java.util.ArrayList.get(ArrayList.java:428)
  org.openrewrite.staticanalysis.EqualsAvoidsNullVisitor.visitMethodInvocation(EqualsAvoidsNullVisitor.java:62)
  org.openrewrite.java.tree.J$MethodInvocation.acceptJava(J.java:3918)
  org.openrewrite.java.tree.J.accept(J.java:59)
  org.openrewrite.TreeVisitor.visit(TreeVisitor.java:250)
  org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:320)
  org.openrewrite.java.JavaVisitor.visitLeftPadded(JavaVisitor.java:1395)
  org.openrewrite.java.JavaVisitor.visitVariable(JavaVisitor.java:1301)
  org.openrewrite.java.tree.J$VariableDeclarations$NamedVariable.acceptJava(J.java:5914)
  org.openrewrite.java.tree.J.accept(J.java:59)
  org.openrewrite.TreeVisitor.visit(TreeVisitor.java:250)
  org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:320)
  ...

koppor

While reviewing, it came clear to me that the "architecture" of the solution is wrong.

isImage is good, the other things maybe not. We need to try out. In your solution, the types with a cover are hard-coded. I would go the extreme other way and show the first image as cover in all cases... Let me describe below:

JabRef offers configurability. For the preview, this is https://docs.jabref.org/setup/preview Read about the syntax at https://docs.jabref.org/collaborative-work/export/customexports.

JabRef should also be "knobless". It should "magically" do something if possible - and not force the user.

Steps:

Understand FileLink(filetype). Implementation is at org.jabref.logic.layout.format.FileLink#format
Add a jpg image to an entry
Adapt JabRef prview preferences to use filelink to show that image
Add a png image to another entry
Adapt JabReff preview preferences to use filelink+png to show that image
Adapt FileLink to support "image" as general file type -- it then searches for all image types and returns the first hit. This makes JabRef "knobless", because JabRef automatically finds the image
Adapt defaults.put(PREVIEW_STYLE, in org.jabref.gui.preferences.JabRefGuiPreferences#JabRefGuiPreferences to contain your adaption
Adapt org.jabref.migrations.PreferencesMigrations#upgradePreviewStyle to migrate to your new style

I think, step 3 is the most challenging

Screenshot of the preferences:

koppor · 2024-11-30T10:41:17Z

src/main/java/org/jabref/gui/preview/PreviewViewer.java

        highlightLayoutText();
        this.setHvalue(0);
    }

+    private String getBookCoverURI() {
+        if (entry != null) {


Is this check really necessary? I would convert it into assert entry != null;

koppor · 2024-11-30T10:43:38Z

src/main/java/org/jabref/gui/preview/PreviewViewer.java

@@ -218,14 +218,35 @@ private void setPreviewText(String text) {
        layoutText = """
                <html>
                    <body id="previewBody">
-                        <div id="content"> %s </div>


No. See general review comments.

damgam0288 and others added 23 commits November 11, 2024 22:22

Add method to check if file is image in LinkedFile.java

bf9388b

Add method to retrieve cover image in BibEntry.java

6aaa9af

Add HTML image for book cover to setPreviewText

aa64848

Remove unnecessary TODO

b74e022

Check EntryType is a book or related item in BibEntry

2100210

Fix book cover size too large in PreviewViewer

c89f3c8

Refactor locations of methods in BibEntry

d7c559b

Remove todo comments. Add comments.

7c82123

Add todo comments for testing new methods

fd3750a

Refactor BibEntry to return cover image file instead of path

0e04bbf

Refactor PreviewViewer to receive cover file from BibEntry and conver…

5e9d764

…t to URI

Add tests for getCoverImageFile method in BibEntry

0f0d2fa

Change cover string to a static field

ab2d3ab

Align book cover to right side in PreviewViewer setPreviewText

da906f7

Fix checkstyle issues

eb06002

Make getCoverImageFile only return image with "cover" in the descript…

23c04d5

…ion. Add tests for this.

Fix poor cover image resizing

c902323

Add test for getCoverImage in BibEntryTest.java

6b789f5

Fix cover image not being displayed when its title has spaces

34647c8

Refactor getBookCoverURI in PreviewViewer

42ac2c9

Merge branch 'JabRef:main' into fix-10120

c561303

Update Changelog for book cover feature issue 10120

a9c5dbd

Merge branch 'main' into fix-10120

aacffed

github-actions bot reviewed Nov 17, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

damgam0288 added 2 commits November 18, 2024 22:55

Fix CI tests failure: change getBookCoverURI

4119b7d

Merge remote-tracking branch 'origin/fix-10120' into fix-10120

8b1a961

This comment was marked as outdated.

Sign in to view

Fix CI tests failure: handle entry field null value

871fcaa

This comment was marked as outdated.

Sign in to view

github-actions bot reviewed Nov 29, 2024

View reviewed changes

Rewrite getCoverImage tests in BibEntryTest.java

a94dac7

github-actions bot reviewed Nov 29, 2024

View reviewed changes

Fix getCoverImage to check files is null or empty

eb0a1b7

github-actions bot reviewed Nov 29, 2024

View reviewed changes

Remove all getCoverImage tests that expect Optional.empty as a result

d0ffcc1

Add BibEntryTest - getCoverImageUpdatesWithChangeToDescription

c7b71d4

github-actions bot reviewed Nov 29, 2024

View reviewed changes

Add OpenRewrite changes to multiple classes

f377939

github-actions bot reviewed Nov 30, 2024

View reviewed changes

Fix missing Junit imports for tests

4f3df63

github-actions bot reviewed Nov 30, 2024

View reviewed changes

Remove unused import in BibEntry

87cc072

subhramit marked this pull request as ready for review November 30, 2024 06:56

Merge branch 'main' into fix-10120

80cdf05

Add tests for getCoverImage in BibEntryTest

b85aab9

koppor requested changes Nov 30, 2024

View reviewed changes

damgam0288 requested a review from subhramit November 30, 2024 11:18

subhramit added the type: feature label Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement book cover feature #12198

Implement book cover feature #12198

damgam0288 commented Nov 17, 2024 •

edited

Loading

github-actions bot left a comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

damgam0288 commented Nov 29, 2024 •

edited

Loading

subhramit commented Nov 29, 2024 •

edited

Loading

github-actions bot left a comment

subhramit commented Nov 29, 2024 •

edited

Loading

damgam0288 commented Nov 29, 2024

github-actions bot left a comment

subhramit commented Nov 30, 2024

github-actions bot left a comment

subhramit commented Nov 30, 2024

damgam0288 commented Nov 30, 2024

subhramit commented Nov 30, 2024

damgam0288 commented Nov 30, 2024

subhramit commented Nov 30, 2024

damgam0288 commented Nov 30, 2024 •

edited

Loading

koppor left a comment

koppor Nov 30, 2024

koppor Nov 30, 2024

Implement book cover feature #12198

Are you sure you want to change the base?

Implement book cover feature #12198

Conversation

damgam0288 commented Nov 17, 2024 • edited Loading

Description

Changes

Code changes

BibEntry.java

PreviewViewer.java

BibEntryTest.java

Notes

Mandatory checks

github-actions bot left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

damgam0288 commented Nov 29, 2024 • edited Loading

subhramit commented Nov 29, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

subhramit commented Nov 29, 2024 • edited Loading

damgam0288 commented Nov 29, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

subhramit commented Nov 30, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

subhramit commented Nov 30, 2024

damgam0288 commented Nov 30, 2024

subhramit commented Nov 30, 2024

damgam0288 commented Nov 30, 2024

subhramit commented Nov 30, 2024

damgam0288 commented Nov 30, 2024 • edited Loading

koppor left a comment

Choose a reason for hiding this comment

koppor Nov 30, 2024

Choose a reason for hiding this comment

koppor Nov 30, 2024

Choose a reason for hiding this comment

damgam0288 commented Nov 17, 2024 •

edited

Loading

`BibEntry.java`

`PreviewViewer.java`

`BibEntryTest.java`

damgam0288 commented Nov 29, 2024 •

edited

Loading

subhramit commented Nov 29, 2024 •

edited

Loading

subhramit commented Nov 29, 2024 •

edited

Loading

damgam0288 commented Nov 30, 2024 •

edited

Loading