Skip to content

OpenOffice Refactor subtasks#15380

Merged
subhramit merged 81 commits intoJabRef:mainfrom
anuv-bit:fixes-for-issue-11829
Apr 21, 2026
Merged

OpenOffice Refactor subtasks#15380
subhramit merged 81 commits intoJabRef:mainfrom
anuv-bit:fixes-for-issue-11829

Conversation

@anuv-bit
Copy link
Copy Markdown
Contributor

@anuv-bit anuv-bit commented Mar 20, 2026 •
edited
Loading

Related issues and pull requests

Closes #11829

PR Description

This PR focuses on several subtasks, such as:

Break down large methods like guiActionInsertEntry() - the refactoring technique ‘extract method’ was used on long methods or repeated blocks of code in ‘OOBibBase’, ‘EditMerge’, ‘OOFrontend’, and ‘CSLCitationOOAdapter’ in order to increase readability and modifiability.

Using more constants instead of magic strings - this task focused on replacing repeated magic strings with named constants across OOBibBase.java, OpenOfficePanel.java, and EditMerge.java to improve readability and reduce duplicated elements.

Using modern Java features(streams and Optional chaining) - this task focused on replacing for loops with stream pipelines using flatMap(), filter(), and anyMatch(). Additionally, getBaseList() was refactored to use streams while preserving mutability.

Unification of error handling methods - Instead of throwing runtime exceptions, we standardized the use of logging combined with OOError dialogs, minimizing the use of throwing exceptions without being caught by some other area of code.

Additionally, this PR includes some minor refactorings such as removing unused parameters and duplicate methods.

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

@anuv-bit anuv-bit changed the title Fix-for-issue #11829- subtask: Break down large methods like guiActionInsertEntry() OpenOffice refactoring- subtask: Break down large methods like guiActionInsertEntry() Mar 20, 2026
@github-actions github-actions Bot mentioned this pull request Mar 20, 2026
Open
9 tasks
@github-actions github-actions Bot added status: changes-required Pull requests that are not yet complete status: no-bot-comments and removed status: changes-required Pull requests that are not yet complete labels Mar 20, 2026
@testlens-app

This comment has been minimized.

Sign in to view
@testlens-app

This comment has been minimized.

Sign in to view
@testlens-app

This comment has been minimized.

Sign in to view
@testlens-app

This comment has been minimized.

Sign in to view
@testlens-app

This comment has been minimized.

Sign in to view
@testlens-app

This comment has been minimized.

Sign in to view
@testlens-app

This comment has been minimized.

Sign in to view
@testlens-app

This comment has been minimized.

Sign in to view
subhramit
subhramit reviewed Mar 27, 2026
View reviewed changes
Comment thread jabgui/src/main/java/org/jabref/gui/openoffice/OOBibBase.java Outdated

public void guiActionSelectDocument(boolean autoSelectForSingle) throws WrappedTargetException, NoSuchElementException {
final String errorTitle = Localization.lang("Problem connecting");
final String connectionError = Localization.lang("Problem connecting");
Copy link
Copy Markdown
Member

@subhramit subhramit Mar 27, 2026

Choose a reason for hiding this comment

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

Please revert these renamings - since they are function-local, they needn't be more specific

Copy link
Copy Markdown
Contributor

@amanda-d-e amanda-d-e Mar 27, 2026

Choose a reason for hiding this comment

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

Understood, I reverted them. Are there more magic strings that you would like replaced with constants (that I have not already replaced)? My understanding was that not every single string needed to be replaced with a constant, especially in cases where the string is unique and is only used once.

Thank you!

Copy link
Copy Markdown
Member

@subhramit subhramit Mar 27, 2026

Choose a reason for hiding this comment

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

These above were not magic strings. They were already constants.
Yes, ideally we extract constants for strings used more than once.
But there is one other case - when the string reduces readability of the code, or is long.

Copy link
Copy Markdown
Contributor

@amanda-d-e amanda-d-e Mar 28, 2026

Choose a reason for hiding this comment

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

Thank you for the confirmation! To clarify: I was referring to changes made in a previous commit in which I did replace magic strings with constants, not this commit which was just renaming the existing constants.

subhramit
subhramit reviewed Mar 27, 2026
View reviewed changes
Comment thread jabgui/src/main/java/org/jabref/gui/openoffice/OOBibBase.java Outdated
/// @param citationType Indicates whether it is an in-text citation, a citation in parenthesis or an invisible citation.
/// @param citationStyle Indicates style, name and path of citation
/// @param syncOptions Indicates whether in-text citations should be refreshed in the document. Optional.empty() indicates no refresh. Otherwise, provides options for refreshing the reference list.
public void guiActionCSLCitationHandler(List<BibEntry> entries, XTextDocument doc, CitationType citationType, CitationStyle citationStyle,
Copy link
Copy Markdown
Member

@subhramit subhramit Mar 27, 2026

Choose a reason for hiding this comment

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

These are more like class names - please use names denoting actions for methods

subhramit
subhramit requested changes Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

One more iteration of comments. Thanks for keeping patience and following up so far.

Comment thread jablib/src/main/java/org/jabref/logic/openoffice/oocsltext/CSLCitationOOAdapter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/openoffice/oocsltext/CSLCitationOOAdapter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/openoffice/oocsltext/CSLCitationOOAdapter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/openoffice/oocsltext/CSLCitationOOAdapter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/openoffice/oocsltext/CSLCitationOOAdapter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/openoffice/oocsltext/CSLCitationOOAdapter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/openoffice/oocsltext/CSLCitationOOAdapter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/openoffice/frontend/OOFrontend.java
Comment thread jabgui/src/main/java/org/jabref/gui/openoffice/OOBibBaseConnect.java
Comment thread jabgui/src/main/java/org/jabref/gui/openoffice/OOBibBase.java Outdated
List<String> unresolvedKeys;
try {
UnoUndo.enterUndoContext(doc, "Refresh bibliography");
if (!performPreUpdateChecks(errorTitle, odoc, style, getFrontend(doc), fcursor, doc)) {
Copy link
Copy Markdown
Member

@subhramit subhramit Apr 8, 2026

Choose a reason for hiding this comment

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

As mentioned in an earlier comment, this changes semantics - for example, fcursor was earlier being fetched only if doc, odoc passed the earlier checks, etc.

Please revert.

Copy link
Copy Markdown
Contributor

@amanda-d-e amanda-d-e Apr 11, 2026

Choose a reason for hiding this comment

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

Hello @subhramit, I have reverted the extracted method. I think that should be the last of the requested fixes, but please let me know if we are missing anything or if you would like anything else changed.

Thank you for your guidance. This has been a very valuable learning experience for my group and I, and we appreciate the opportunity to work on this issue.

@github-actions github-actions Bot added status: changes-required Pull requests that are not yet complete and removed status: no-bot-comments labels Apr 8, 2026
anuv-bit and others added 2 commits April 8, 2026 11:52
@github-actions github-actions Bot added status: no-bot-comments and removed status: changes-required Pull requests that are not yet complete labels Apr 8, 2026
@anuv-bit anuv-bit force-pushed the fixes-for-issue-11829 branch 2 times, most recently from f675f0c to d363569 Compare April 9, 2026 04:02
subhramit
subhramit approved these changes Apr 18, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

LGTM
Would wait for @pluto-han for a second final look.

Thanks a lot team @anuv-bit @amanda-d-e @sam-r914 for your patience.

@subhramit
Copy link
Copy Markdown
Member

subhramit commented Apr 21, 2026

Letting this in.

@subhramit subhramit added this pull request to the merge queue Apr 21, 2026
@github-actions github-actions Bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label Apr 21, 2026
Merged via the queue into JabRef:main with commit b4778ad Apr 21, 2026
52 checks passed
@pluto-han
Copy link
Copy Markdown
Collaborator

pluto-han commented Apr 21, 2026

Sorry for the late comment, I missed #15380 (review).

Appreciate for your great work and quick follow-ups! @anuv-bit @amanda-d-e @sam-r914

Siedlerchr added a commit that referenced this pull request Apr 22, 2026
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into fixnpe
6845209 
* upstream/main:
  Remove review-to-comment migration (#15609)
  Chore(deps): Bump com.uber.nullaway:nullaway in /versions (#15615)
  Chore(deps): Bump javafx from 26 to 26.0.1 in /versions (#15614)
  Update dependency org.openjfx:javafx-base to v26.0.1 (#15608)
  Chore(deps): Bump org.openrewrite.rewrite from 7.30.0 to 7.31.0 (#15602)
  OpenOffice Refactor subtasks (#15380)
  Chore(deps): Bump org.hisp.dhis:json-tree in /versions (#15606)
  Chore(deps): Bump com.autonomousapps:dependency-analysis-gradle-plugin (#15604)
  Chore(deps): Bump com.autonomousapps:dependency-analysis-gradle-plugin (#15605)
  Chore(deps): Bump org.openrewrite.recipe:rewrite-recipe-bom (#15603)
  Fix hash warning (#15596)
  Update testlens (#15595)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pluto-han pluto-han pluto-han left review comments

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects[bot] qodo-free-for-open-source-projects[bot] left review comments

@subhramit subhramit subhramit approved these changes

+2 more reviewers

@sam-r914 sam-r914 sam-r914 left review comments

@amanda-d-e amanda-d-e amanda-d-e left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@subhramit subhramit

Labels

component: libre-office status: no-bot-comments status: to-be-merged PRs which are accepted and should go into the merge-queue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Project: Refactor and unify JabRef OO components

5 participants

@anuv-bit @pluto-han @amanda-d-e @subhramit @sam-r914