Fix for issue 13132: Show entry column only when not empty by Vane-Arellano · Pull Request #13180 · JabRef/jabref

Vane-Arellano · 2025-05-28T18:36:51Z

DRAFT PR, NEED FEEDBACK TO ENSURE THIS IS A DESIRED SOLUTION

This PR contains changes to PdfMergeDialog.java, where before adding the entry column, it validates if the entry coming to the dialog is empty. If it is not empty, it will create the entry column as usual.

Screenshot related to the solution implemented:
Test with the Lego4TOSCA-Composable-Building-Blocks-for-Cloud-Applications.pdf

Test with the multiple-meta-data.pdf

Mandatory checks

I own the copyright of the code submitted and I license 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 (if change is visible to the user)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
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.

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Vane-Arellano · 2025-05-28T18:43:05Z

@koppor thanks for your support under this issue, I made this solution to not show the empty column, hope it aligns with the requirements of the project, but if not, please let me know so I can come up with another solution:)

koppor

The solution is kind of a "quick hack".

Works, but better is follows: A second method without entry is needed.

Create two helper methods:

Create the initial dialog (initDialog)
Create method filling the remainder (finishDialog)

Use these methods in both constructors.

The constructor with entry does

dialog.addSource(Localization.lang("Entry"), entry);

Inbetween.

Much code, but a more clean interface at the caller side.

koppor · 2025-05-28T21:31:04Z

 jablib/src/main/resources/csl-styles
 jablib/src/main/resources/csl-locales
+jablib/src/main/abbrv.jabref.org
+jablib/src/main/resources/csl-locales


This is line 11 - remove it.

Vane-Arellano · 2025-05-29T16:22:39Z

The solution is kind of a "quick hack".

Works, but better is follows: A second method without entry is needed.

Create two helper methods:

Create the initial dialog (initDialog)

Create method filling the remainder (finishDialog)

Use these methods in both constructors.

The constructor with entry does
dialog.addSource(Localization.lang("Entry"), entry);
Inbetween.

Much code, but a more clean interface at the caller side.

Thanks! working on that

create helper methods initDialog and finishDialog

koppor · 2025-05-29T18:11:49Z

     * @param preferences the preferences to use. Full preference object is required, because of current implementation of {@link MultiMergeEntriesView}.
     * @param taskExecutor the task executor to use when the multi merge dialog executes the importers.
     */
+


No new empty line (which IDE do you use?)

I'm using IntelliJ

koppor · 2025-05-29T18:12:26Z

+     * </ul>
     *
-     * @param entry the entry to merge with
+     * @param entry the entry to merge with (only for {@link #createMergeDialog(BibEntry, Path, GuiPreferences, TaskExecutor)})


No need to state the caller - because one does not know about future usages and IDEs offer the capabilities to see the known callers.

koppor · 2025-05-29T18:16:49Z

+     * Two entry points are provided:
+     * <ul>
+     *   <li>{@link #createMergeDialog(BibEntry, Path, GuiPreferences, TaskExecutor)} to merge a known entry with extracted data.</li>
+     *   <li>{@link #createMergeDialog(Path, GuiPreferences, TaskExecutor)} to extract all data from the PDF without a preexisting entry.</li>
+     * </ul>


This is more a class comment - either refactor the whole description to be as a class comment - or remove this text.

Just removed this one, is the
"
* Internally, this is split into two helper methods:
* < ul>
* < li>{@code initDialog} sets up the dialog with preferences and title.< /li>
* < li>{@code finishDialog} populates the dialog with all available import sources.< /li>
* </ ul>
"

also consider class comment?

Can be removed completely, because the code is so short and one can read from the code.

koppor · 2025-05-29T18:17:01Z

 import org.jabref.model.entry.BibEntry;

 public class PdfMergeDialog {
-


Please keep this empty line.

Vane-Arellano · 2025-05-29T22:20:22Z

Made required changes, please let me know if there's anything else needed

trag-bot · 2025-05-29T22:21:23Z

-        MultiMergeEntriesView dialog = new MultiMergeEntriesView(preferences, taskExecutor);
+        MultiMergeEntriesView dialog = initDialog(preferences, taskExecutor);
+
+        dialog.addSource(Localization.lang("Entry"), entry);


The method addSource is being passed a potentially null entry, which could lead to a NullPointerException. Consider using Optional to handle the possibility of null values.

koppor

Thank you for the follow-up.

I think, leaving JavaDoc as is, is OK. A follow-up can move it to the class, but I think, we save this loop in this context.

koppor · 2025-05-29T22:59:01Z

@Vane-Arellano Do you have time and energy to add a line to CHANGELOG.md?

Vane-Arellano · 2025-05-30T04:09:49Z

@Vane-Arellano Do you have time and energy to add a line to CHANGELOG.md?

@koppor sure can do! :) thanks for all your guidance in this issue btw
Should I place it under unreleased and then changed?

trag-bot · 2025-05-31T07:17:05Z

@trag-bot didn't find any issues in the code! ✅✨

koppor · 2025-05-31T07:18:14Z

@Vane-Arellano Do you have time and energy to add a line to CHANGELOG.md?
@koppor sure can do! :) thanks for all your guidance in this issue btw Should I place it under unreleased and then changed?

Yes, that was right! - My answer wasn't sent somehow.

I fixed the CHANGELOG.md entrfy to be user-facing and not programmer-facing. Now, its good to go. Thank you for adressing the review comments.

Vane-Arellano added 2 commits May 27, 2025 10:24

Avoid showing the Entry column when has no fields

2334fa7

Merge branch 'main' into fix-for-issue-13132

5c86c0a

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Vane-Arellano changed the title ~~Fix for issue 13132~~ Fix for issue 13132: Show entry column only when not empty May 28, 2025

koppor requested changes May 28, 2025

View reviewed changes

Create additional constructor for dialogs with empty entry

138275e

create helper methods initDialog and finishDialog

Vane-Arellano requested a review from koppor May 29, 2025 17:09

koppor requested changes May 29, 2025

View reviewed changes

Revert comment changes

d712974

Vane-Arellano requested a review from koppor May 29, 2025 22:19

trag-bot Bot reviewed May 29, 2025

View reviewed changes

koppor marked this pull request as ready for review May 29, 2025 22:57

koppor previously approved these changes May 29, 2025

View reviewed changes

Add changes to CHANGELOG.md

6d73bf7

Vane-Arellano dismissed koppor’s stale review via 6d73bf7 May 31, 2025 05:08

Merge branch 'main' into fix-for-issue-13132

9393a4c

koppor reviewed May 31, 2025

View reviewed changes

Comment thread CHANGELOG.md Outdated

Update CHANGELOG.md

2002e66

koppor approved these changes May 31, 2025

View reviewed changes

koppor enabled auto-merge May 31, 2025 07:17

koppor added this pull request to the merge queue May 31, 2025

Merged via the queue into JabRef:main with commit ea22869 May 31, 2025
1 check passed

github-actions Bot mentioned this pull request May 31, 2025

Do not show empty column in Merge Entries #13132

Closed

		import org.jabref.model.entry.BibEntry;

		public class PdfMergeDialog {

Uh oh!

Conversation

Vane-Arellano commented May 28, 2025

Mandatory checks

Uh oh!

Vane-Arellano commented May 28, 2025

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

koppor May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Vane-Arellano commented May 29, 2025

Uh oh!

koppor May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Vane-Arellano May 29, 2025

Choose a reason for hiding this comment

Uh oh!

koppor May 29, 2025

Choose a reason for hiding this comment

Uh oh!

koppor May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Vane-Arellano May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor May 29, 2025

Choose a reason for hiding this comment

Uh oh!

koppor May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Vane-Arellano commented May 29, 2025

Uh oh!

trag-bot Bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

koppor commented May 29, 2025

Uh oh!

Vane-Arellano commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

trag-bot Bot commented May 31, 2025

Uh oh!

koppor commented May 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Vane-Arellano May 29, 2025 •

edited

Loading

Vane-Arellano commented May 30, 2025 •

edited

Loading