Hide Merge Metadata Dialog if there is nothing to merge by Nathaandev · Pull Request #13309 · JabRef/jabref

Nathaandev · 2025-06-12T02:02:52Z

Closes #13262
In this PR, I improved the behavior of the library creation dialog when importing PDF files. Specifically, when a PDF containing only a single column is dragged into a newly created library, the dialog now automatically closes. This fixes the issue where the dialog would remain open unnecessarily, improving the user experience during PDF import.

Steps to test

Create a new library in the application.

Drag'n'drop jablib\build\resources\test\pdfs\minimal.pdf into it

Observe that the dialog closes automatically after the PDF is added.

Screenshots

Dialog appears after dragging jablib\build\resources\test\pdfs\minimal.pdf into the new library

Dialog closes itself because there is only one column.

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.

Nathaandev · 2025-06-12T03:19:10Z

I have no idea of why this changelog test is failing.

calixtus

Im not so sure about the fix. This seems to be a bit hacky to me.
The dialog now choses by itself when to show. There might be a scenario when the dialog should display anyway, that we don't think of now.
I believe the fix should happen where the dialog is called, not in the dialog itself.

calixtus · 2025-06-12T07:08:38Z

I have no idea of why this changelog test is failing.

you put your note in the wrong section, an old release. put your note into the section [unreleased]

subhramit · 2025-06-12T07:10:43Z

I have no idea of why this changelog test is failing.

How to navigate: Have a look at the structure of the changelog and what each heading means. Then try to infer what the test says - "only unreleased touched".

You have added your entry under the 5.0-alpha version heading, when you had to put it under the unreleased section.

koppor · 2025-06-12T08:04:05Z

Im not so sure about the fix. This seems to be a bit hacky to me.

Yeah, but improves workflow while working with JabRef

The dialog now choses by itself when to show. There might be a scenario when the dialog should display anyway, that we don't think of now.

Then we can disable.

I believe the fix should happen where the dialog is called, not in the dialog itself.

This is impossible - is it? @Nathaandev can you check

Because columns are filled dynamically.

Nathaandev · 2025-06-12T19:26:21Z

I got a lot of failing tests this time, but I don't know why.

subhramit · 2025-06-12T20:53:12Z

I got a lot of failing tests this time, but I don't know why.

Try to run JabRef. It probably does not compile.
No need to panic, one step at a time.

Nathaandev · 2025-06-12T21:02:56Z

I got a lot of failing tests this time, but I don't know why.

Try to run JabRef. It probably does not compile. No need to panic, one step at a time.

It does compile, it works normally.

subhramit · 2025-06-12T21:23:13Z

I got a lot of failing tests this time, but I don't know why.

Try to run JabRef. It probably does not compile. No need to panic, one step at a time.

It does compile, it works normally.

Apparently, CI checks are failing because half of the internet is down right now.
https://www.bleepingcomputer.com/news/technology/google-cloud-and-cloudflare-hit-by-widespread-service-outages/
So it's not you - don't worry.

Nathaandev · 2025-06-12T21:24:47Z

I got a lot of failing tests this time, but I don't know why.

Try to run JabRef. It probably does not compile. No need to panic, one step at a time.

It does compile, it works normally.

Apparently, CI checks are failing because half of the internet is down right now. bleepingcomputer.com/news/technology/google-cloud-and-cloudflare-hit-by-widespread-service-outages So it's not you - don't worry.

Ah ok, so I have to wait and later I make another commit or there is another way to make the tests again?

subhramit · 2025-06-12T21:34:15Z

Ah ok, so I have to wait and later I make another commit or there is another way to make the tests again?

In the guide to set up a local workspace, there is a hint on how to run a unit test locally. Checkstyle will already warn you via intelliJ, for openrewrite you can follow the developer faqs if you wish to run locally.

Nathaandev · 2025-06-12T21:53:23Z

Ah ok, so I have to wait and later I make another commit or there is another way to make the tests again?

In the guide to set up a local workspace, there is a hint on how to run a unit test locally. Checkstyle will already warn you via intelliJ, for openrewrite you can follow the developer faqs if you wish to run locally.

Thanks!

Nathaandev · 2025-06-13T04:41:10Z

Im not so sure about the fix. This seems to be a bit hacky to me.

Yeah, but improves workflow while working with JabRef

The dialog now choses by itself when to show. There might be a scenario when the dialog should display anyway, that we don't think of now.

Then we can disable.

I believe the fix should happen where the dialog is called, not in the dialog itself.

This is impossible - is it? @Nathaandev can you check

Because columns are filled dynamically.

I've explored the possibility, but it appears to be impossible. But perhaps I just wasn't good enough so i'm not 100% sure it is impossible. If the current fix still seems a bit hacky for you, I can definitely look into an alternative.

Nathaandev · 2025-06-13T05:06:37Z

@calixtus if the current fix isn't quite right, i'm happy to discuss other approaches.

koppor · 2025-06-13T05:26:11Z

+                    "Could not extract Metadata from: %0",
+                    String.join(", ", viewModel.failedSuppliersProperty())));
+            // Closes the dialog if the there is only one active column.
+            active_columns = viewModel.entriesProperty().get().size() - viewModel.failedSuppliersProperty().get().size();


No snake case in JabRef.

No snake case in JabRef.

Would it be ok if I rename the variable to "activecolumns"?

Would it be ok if I rename the variable to "activecolumns"?

read about snake casing and camel casing. In languages like python, we generally follow snake_casing. In Java, the default convention is camelCasing. So that would make it activeColumns

Would it be ok if I rename the variable to "activecolumns"?

read about snake casing and camel casing. In languages like python, we generally follow snake_casing. In Java, the default convention is camelCasing. So that would make it activeColumns

I understand. whenever I was making my projects, I did not care about camelCasing or snake_casing, I would just give them any name. But it's good to learn that it's important. But what about the constants, is this rule applicable to them as well?

But what about the constants, is this rule applicable to them as well?

No. Constants generally have capital letters, and once we fix case, camel case cannot separate words anymore. So, then, we need underscores, for example, private static final int DEFAULT_RETURN_VALUE = 40.
We use Checkstyle with some custom rules to enforce these. Checkstyle's default configuration is based on Google's style guide, so a lot of these practices are inherited from them.
Logically, code style does not matter, but when it's a large codebase with multiple people working on it and changing code written by others, and adding many lines of their own, it becomes important to have readability and maintainability as a concern, and having a uniform style throughout helps.

But what about the constants, is this rule applicable to them as well?

No. Constants generally have capital letters, and once we fix case, camel case cannot separate words anymore. So, then, we need underscores, for example, private static final int DEFAULT_RETURN_VALUE = 40. We use Checkstyle with some custom rules to enforce these. Checkstyle's default configuration is based on Google's style guide, so a lot of these practices are inherited from them. Logically, code style does not matter, but when it's a large codebase with multiple people working on it and changing code written by others, and adding many lines of their own, it becomes important to have readability and maintainability as a concern, and having a uniform style throughout helps.

Thank you for the patience and for explaining to me. You guy are helping me a lot.

Nathaandev · 2025-06-14T16:09:01Z

@calixtus @koppor Is there anything else I have to do?

Co-authored-by: Subhramit Basu <subhramit.bb@live.in>

Nathaandev · 2025-06-18T05:55:26Z

@Nathaandev code wise looks good to me. Can you provide some screenshots in the description (and tick off the respective mandatory check)?

Done! And I have commited your suggestion as well.

subhramit

I am not sure about the "and the user did not do any manual changes" part of the issue if it would require any special handling, but with the context I could gather, looks good. Requires a second review.

koppor · 2025-06-18T12:52:27Z

I am not sure about the "and the user did not do any manual changes" part of the issue if it would require any special handling,

When the decision to close is made, check if the merged entry is the same (equals should work) as the remaining one.

It could be that a user begins to change something. However, here, it are only 2 seconds or so; thus, this is maybe cosmetics only.

koppor · 2025-06-18T23:21:03Z

Let's see what users will report.

On the first load, I had a long respoonse time from Grobid - and could have typed something in the dialog. The subsequent runs were very fast. Thus, I don't see a risk of loosing data if the dialog is closed.

Thus, I approve. Thank you for spending time with us @Nathaandev

koppor · 2025-06-18T23:28:14Z

I inlined a variable at 95b50f9 (#13309).

Since the code could cause questions while reading, I added some requirements text. I think, our team agrees to have "Requirements" first and not just issues linked in the code? @subhramit

trag-bot · 2025-06-18T23:28:46Z

+                    String.join(", ", viewModel.failedSuppliersProperty())));
+            int activeColumns = viewModel.entriesProperty().get().size() - viewModel.failedSuppliersProperty().get().size();
+            if (activeColumns < ACTIVE_COLUMNS_MINIMUM) {
+                // [impl->req~ux.auto-close.merge-entries~1]


Comment does not provide meaningful information about the code's purpose or reasoning. Implementation details should be documented in requirements, not in code comments.

koppor · 2025-06-18T23:29:54Z

I added to the merge queue to keep things going. We can fix the text as a follow up.

👍

* upstream/main: Make snap available for arm64 as well (#13383) Hide Merge Metadata Dialog if there is nothing to merge (#13309) Fix #13269 : support multi-file import across different formats (#13271) Fix Javadoc in CitaviXmlImporter.java/getPages (#13381)

Nathaandev · 2025-06-22T02:56:13Z

Hello, @koppor @subhramit,

Thanks for the help on this issue. I wanted to ask you if it is ok if I make a Linkedin post about JabRef and this issue, I am looking for my first job as a Junior back-end developer and I would appreciate sharing my learning progress on LinkedIn. Thanks.

koppor · 2025-06-22T05:47:20Z

Thanks for the help on this issue. I wanted to ask you if it is ok if I make a Linkedin post about JabRef and this issue,

Sure! Go ahead! Please use #JabRef in the post :)

Siedlerchr · 2025-06-22T08:59:48Z

@Nathaandev Sure, you can also tag @JabRef on linkedin, our page

Nathaandev added 4 commits June 11, 2025 22:39

Fix-for-issue13262

a1ba49b

Changelog changed

b259980

fix-for-issue13262

b62f644

fix-for-issue13262

d3badcf

calixtus previously requested changes Jun 12, 2025

View reviewed changes

Comment thread jabgui/src/main/java/org/jabref/gui/mergeentries/MultiMergeEntriesView.java Outdated

calixtus changed the title ~~Fix-for-issue13262~~ Hide Merge Metadata Dialog if there is nothing to merge Jun 12, 2025

koppor reviewed Jun 12, 2025

View reviewed changes

Comment thread jabgui/src/main/java/org/jabref/gui/mergeentries/MultiMergeEntriesView.java

Nathaandev and others added 3 commits June 12, 2025 14:06

Changelog changes

6e2ee9e

13262

b0aad6f

Merge branch 'JabRef:main' into fix-for-issue13262

8900148

Hide Merge Metadata Dialog if there is nothing to merge

01bb4ac

Nathaandev requested a review from calixtus June 13, 2025 05:06

koppor requested changes Jun 13, 2025

View reviewed changes

Nathaandev added 2 commits June 13, 2025 02:58

fix-13262

852c464

Rename variable

341ca2f

Nathaandev requested a review from koppor June 13, 2025 17:52

Apply suggestions from code review

ce70ce9

Co-authored-by: Subhramit Basu <subhramit.bb@live.in>

subhramit previously approved these changes Jun 18, 2025

View reviewed changes

subhramit added the status: awaiting-second-review For non-trivial changes label Jun 18, 2025

Merge branch 'main' into fix-for-issue13262

5af3d1d

koppor enabled auto-merge June 18, 2025 23:21

koppor previously approved these changes Jun 18, 2025

View reviewed changes

koppor disabled auto-merge June 18, 2025 23:21

koppor added 2 commits June 19, 2025 01:22

Inline variable

95b50f9

Add requirement

94d6b2f

koppor dismissed stale reviews from subhramit and themself via 94d6b2f June 18, 2025 23:26

koppor removed the status: awaiting-second-review For non-trivial changes label Jun 18, 2025

trag-bot Bot reviewed Jun 18, 2025

View reviewed changes

koppor approved these changes Jun 18, 2025

View reviewed changes

koppor enabled auto-merge June 18, 2025 23:29

koppor added this pull request to the merge queue Jun 19, 2025

Merged via the queue into JabRef:main with commit ef7ecfe Jun 19, 2025
1 check passed

github-actions Bot mentioned this pull request Jun 19, 2025

Do not show "Merge PDF metadata" dialog if there is nothing to merge #13262

Closed

Nathaandev deleted the fix-for-issue13262 branch June 20, 2025 17:34

Uh oh!

Conversation

Nathaandev commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to test

Screenshots

Mandatory checks

Uh oh!

Nathaandev commented Jun 12, 2025

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

calixtus commented Jun 12, 2025

Uh oh!

subhramit commented Jun 12, 2025

Uh oh!

Uh oh!

koppor commented Jun 12, 2025

Uh oh!

Nathaandev commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subhramit commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nathaandev commented Jun 12, 2025

Uh oh!

subhramit commented Jun 12, 2025

Uh oh!

Nathaandev commented Jun 12, 2025

Uh oh!

subhramit commented Jun 12, 2025

Uh oh!

Nathaandev commented Jun 12, 2025

Uh oh!

Nathaandev commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nathaandev commented Jun 13, 2025

Uh oh!

koppor Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Nathaandev Jun 13, 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!

subhramit Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Nathaandev Jun 13, 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!

subhramit Jun 13, 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!

Nathaandev Jun 13, 2025 • edited by subhramit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Nathaandev commented Jun 14, 2025

Uh oh!

Nathaandev commented Jun 18, 2025

Uh oh!

subhramit left a comment • 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 commented Jun 18, 2025

Uh oh!

koppor commented Jun 18, 2025

Uh oh!

koppor commented Jun 18, 2025

Uh oh!

trag-bot Bot Jun 18, 2025

Nathaandev commented Jun 12, 2025 •

edited

Loading

Nathaandev commented Jun 12, 2025 •

edited

Loading

subhramit commented Jun 12, 2025 •

edited

Loading

Nathaandev commented Jun 13, 2025 •

edited

Loading

Nathaandev Jun 13, 2025 •

edited

Loading

Nathaandev Jun 13, 2025 •

edited

Loading

subhramit Jun 13, 2025 •

edited

Loading

Nathaandev Jun 13, 2025 •

edited by subhramit

Loading

subhramit left a comment •

edited

Loading