Skip to content

Fix requirements#15600

Merged
Siedlerchr merged 4 commits into
mainfrom
fix1
May 4, 2026
Merged

Fix requirements#15600
Siedlerchr merged 4 commits into
mainfrom
fix1

Conversation

@koppor

@koppor koppor commented Apr 20, 2026

Copy link
Copy Markdown
Member

I think, our requirements are not clean. This fixes some issues.

Steps to test

  • see CI passing
  • read through

Checklist

  • I own the copyright of the code submitted and I license it under the MIT license
  • [/] I manually tested my changes in running JabRef (always required)
  • [/] I added JUnit tests for changes (if applicable)
  • [/] I added screenshots in the PR description (if change is visible to the user)
  • [/] I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
  • [/] I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
  • [/] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

koppor and others added 3 commits April 20, 2026 22:07
Resolve duplicate req~ui.dialogs.confirmation.naming~1 by linking
ui-recommendations.md to the canonical definition in ux.md. Drop
unmet "Needs: impl" from the two aspirational search requirements
and add an impl marker for req~ux.disabled-vs-hidden~1 in
OpenUrlAction (the exact example from the requirement).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@koppor

koppor commented May 3, 2026

Copy link
Copy Markdown
Member Author

Does this happen also in main?

Run gradle checkAllModuleInfo

> /home/runner/work/jabref/jabref/jabgui/src/main/java/module-info.java
  
  Please remove the following requires directives (or change to runtimeOnly):
      requires org.kordamp.ikonli.material;

@InAnYan

InAnYan commented May 3, 2026

Copy link
Copy Markdown
Member

Does this happen also in main?

Run gradle checkAllModuleInfo

> /home/runner/work/jabref/jabref/jabgui/src/main/java/module-info.java
  
  Please remove the following requires directives (or change to runtimeOnly):
      requires org.kordamp.ikonli.material;

Yes

@InAnYan

InAnYan commented May 3, 2026

Copy link
Copy Markdown
Member

@Siedlerchr

Copy link
Copy Markdown
Member

Can this be merged?

@koppor koppor marked this pull request as ready for review May 4, 2026 21:00
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix requirements tracing and consolidate duplicate definitions

✨ Enhancement 📝 Documentation

Grey Divider

Walkthroughs

Description
• Add requirement tracing markers to implementations
• Consolidate duplicate requirement definitions in documentation
• Update requirement documentation with implementation status
• Add missing YAML frontmatter to requirements file
Diagram
flowchart LR
  A["Duplicate Requirements<br/>in Documentation"] -->|Consolidate| B["Single Canonical<br/>Definition in ux.md"]
  C["Code Implementations"] -->|Add Tracing| D["Link to Requirements<br/>via Comments"]
  E["Incomplete Requirements<br/>Metadata"] -->|Update| F["Add Status & Frontmatter"]
Loading

Grey Divider

File Changes

1. jabgui/src/main/java/org/jabref/gui/maintable/OpenUrlAction.java ✨ Enhancement +1/-0

Link OpenUrlAction to UX requirement

• Add requirement tracing comment linking implementation to req~ux.disabled-vs-hidden~1
• Comment marks the exact example from the requirement specification

jabgui/src/main/java/org/jabref/gui/maintable/OpenUrlAction.java


2. jabkit/src/main/java/org/jabref/toolkit/JabKitLauncher.java ✨ Enhancement +1/-0

Link JabKit banner to CLI requirement

• Add requirement tracing comment linking CLI banner implementation to
 req~jabkit.cli.banner-shown~1
• Comment documents the implementation of the banner display requirement

jabkit/src/main/java/org/jabref/toolkit/JabKitLauncher.java


3. docs/code-howtos/ui-recommendations.md 📝 Documentation +1/-2

Remove duplicate requirement reference

• Remove duplicate requirement definition req~ui.dialogs.confirmation.naming~1
• Replace with link to canonical definition in ux.md
• Remove "Needs: impl" status from duplicate reference

docs/code-howtos/ui-recommendations.md


View more (4)
4. docs/requirements/cli.md 📝 Documentation +2/-0

Add implementation status marker

• Add "Needs: impl" marker to indicate implementation status required
• Clarifies that the CLI banner requirement needs implementation tracking

docs/requirements/cli.md


5. docs/requirements/files.md 📝 Documentation +3/-0

Add missing YAML frontmatter

• Add YAML frontmatter with parent reference to Requirements
• Ensures proper documentation hierarchy and navigation

docs/requirements/files.md


6. docs/requirements/search-within-library.md 📝 Documentation +3/-0

Add implementation status note

• Add note indicating no implementation is currently linked
• Clarifies implementation status for search requirements

docs/requirements/search-within-library.md


7. docs/requirements/ux.md 📝 Documentation +11/-0

Consolidate and complete UX requirements

• Add requirement ID req~ux.disabled-vs-hidden~1 to disabled vs hidden section
• Add "Needs: impl" marker for disabled vs hidden requirement
• Add new requirement req~ui.dialogs.confirmation.naming~1 for confirmation dialog naming
• Consolidate duplicate requirement definition from ui-recommendations.md

docs/requirements/ux.md


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Remediation recommended

1. Incorrect banner requirement link 🐞 Bug ≡ Correctness
Description
The newly added OpenFastTrace link // [impl->req~jabkit.cli.banner-shown~1] marks the CLI banner
requirement as implemented, but JabKit.run() still prints the banner in non-help scenarios (e.g.,
running the root command with only global flags like -p). This makes requirement tracing
misleading because the documented requirement says the banner is only shown when help is output.
Code

jabkit/src/main/java/org/jabref/toolkit/JabKitLauncher.java[R80-82]

+            // [impl->req~jabkit.cli.banner-shown~1]
            String usageHeader = BuildInfo.JABREF_BANNER.formatted(buildInfo.version) + "\n" + JABKIT_BRAND;
            commandLine.getCommandSpec().usageMessage().header(usageHeader);
Evidence
The requirement explicitly limits banner output to help/no-command cases, but the root command’s
run() prints the banner whenever it executes (unless --version). Because SharedOptions
provides global flags like -p/--porcelain and -d/--debug, the root command can execute without
--help, contradicting the requirement while the PR now claims an implementation link at the
launcher’s usage-header setup.

docs/requirements/cli.md[16-24]
jabkit/src/main/java/org/jabref/toolkit/JabKitLauncher.java[78-92]
jabkit/src/main/java/org/jabref/toolkit/commands/JabKit.java[79-86]
jabkit/src/main/java/org/jabref/toolkit/commands/JabKit.java[251-260]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The PR adds an OpenFastTrace implementation link for `req~jabkit.cli.banner-shown~1`, but the current code path still prints the JabRef banner when the root `jabkit` command runs without help (e.g., `jabkit -p`). This makes the trace misleading and contradicts the requirement text.

### Issue Context
- Requirement: banner should be shown only when help is output / no command given.
- Current behavior: `JabKit.run()` prints the banner on root command execution (unless `--version`). With global flags (`-p`, `-d`) the root command can execute without `--help`.

### Fix Focus Areas
- jabkit/src/main/java/org/jabref/toolkit/JabKitLauncher.java[78-92]
- jabkit/src/main/java/org/jabref/toolkit/commands/JabKit.java[79-86]
- jabkit/src/main/java/org/jabref/toolkit/commands/JabKit.java[251-260]
- docs/requirements/cli.md[16-24]

### What to change
Implement one consistent approach:
1) **Preferred (align behavior to requirement):** ensure the root command does not execute `JabKit.run()` in non-help scenarios (e.g., require a subcommand or explicitly route “no subcommand / only global flags” to usage/help), and remove/adjust the unconditional banner printing in `JabKit.run()`.
2) If the current behavior is intended, **update the requirement text** to match reality and relocate/keep the `[impl->...]` link accordingly.

Finally, keep the `[impl->req~jabkit.cli.banner-shown~1]` marker only on code that truly enforces the requirement’s stated behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@Siedlerchr Siedlerchr enabled auto-merge May 4, 2026 21:00
@Siedlerchr Siedlerchr added this pull request to the merge queue May 4, 2026
@github-actions github-actions Bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label May 4, 2026
Merged via the queue into main with commit 6ee284d May 4, 2026
61 checks passed
@Siedlerchr Siedlerchr deleted the fix1 branch May 4, 2026 21:45
Siedlerchr added a commit that referenced this pull request May 5, 2026
* upstream/main: (775 commits)
  Chore(deps): Bump com.konghq:unirest-modules-gson in /versions (#15682)
  Chore(deps): Bump org.glassfish.jaxb:jaxb-runtime in /versions (#15681)
  Update dependency com.konghq:unirest-modules-gson to v4.9.0 (#15679)
  Integrate with SearchRxiv  (#15373)
  Fix requirements (#15600)
  refactor: less objects during writing (#15677)
  Fix: suppress WARN for empty or blank column name in MainTableColumnModel#parse() (#15576)
  New Crowdin updates (#15676)
  Chore(deps): Bump com.github.ben-manes.caffeine:caffeine in /versions (#15673)
  Fix Nullwarnings - C (Mark bst package as nonnull by default) (#15663)
  Chore(deps): Bump com.github.javaparser:javaparser-symbol-solver-core (#15674)
  Chore(deps): Bump com.github.javaparser:javaparser-core in /versions (#15672)
  New Crowdin updates (#15669)
  Fix OpenRewrite (#15670)
  Udpate heylogs (and fix CHANGELOG.md) (#15671)
  Improve security and prevent shell injection for push2applications (#15628)
  Fix depdency analysis (#15668)
  Always use CI-local "gradle", instead of gradlew (#15667)
  Change OpenRewrite task to use rewriteDryRun (#15664)
  Add small documentation to parameter (#15666)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: to-be-merged PRs which are accepted and should go into the merge-queue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants