Skip to content

Fix handling of relative-file storage and auto linking#11492

Merged
calixtus merged 6 commits into
mainfrom
fix-rel-path
Jul 16, 2024
Merged

Fix handling of relative-file storage and auto linking#11492
calixtus merged 6 commits into
mainfrom
fix-rel-path

Conversation

@koppor

@koppor koppor commented Jul 15, 2024

Copy link
Copy Markdown
Member

Fixes https://github.com/JabRef/jabref-issue-melting-pot/issues/479

Follow-up to #9113

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • 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.

@koppor koppor enabled auto-merge July 15, 2024 00:21
@koppor koppor marked this pull request as draft July 15, 2024 06:28
auto-merge was automatically disabled July 15, 2024 06:28

Pull request was converted to draft

@koppor koppor marked this pull request as ready for review July 15, 2024 07:19
Comment on lines +183 to +186
Path absolutePath = parentPath.toAbsolutePath();
if (!fileDirs.contains(absolutePath)) {
fileDirs.add(absolutePath);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe we can make a set out of this...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not useful, we need to keep the order and we only have like 3 possible values

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sortedset

@koppor koppor Jul 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sortedset

We need to keep the order.

Sequenced Collection Set is the "right" data type. It checks for containment at each insert. With my approach, only one check needs to be made.

Moreover, we have at most two elements to check. We use an ArrayList where the "forEach" is faster than with a LinkedList.

Changing the return type to a sequencedcollection results in much code changes. Using it internally and then converting to a list results in more CPU cycles than "just" checking two elements.

I can rewrite to return SequencedCollection, write an ADR, add Java comments, link to this text or just leave it.

What should I do?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK, I just switched to LinkedHashSet. This also ensures that the second path is checked for uniqueness.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was just an idea, the code just smelled like a set. no strong opinion on this.

Siedlerchr
Siedlerchr previously approved these changes Jul 15, 2024
@github-actions

github-actions Bot commented Jul 16, 2024

Copy link
Copy Markdown
Contributor

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

@calixtus calixtus added this pull request to the merge queue Jul 16, 2024
Merged via the queue into main with commit a5e1bd6 Jul 16, 2024
@calixtus calixtus deleted the fix-rel-path branch July 16, 2024 10:12
Siedlerchr added a commit to subhramit/jabref that referenced this pull request Jul 19, 2024
* upstream/main:
  Fix NPE when saving preferences (JabRef#11509)
  Switch to stream-based loading (JabRef#11479)
  Save unlinked local files dialog prefs (JabRef#11493)
  Add minimal support for biblatex data annotations (JabRef#11506)
  Fix handling of relative-file storage and auto linking (JabRef#11492)
  New Crowdin updates (JabRef#11504)
  Add missing issue numbers
  CSL4LibreOffice - A [GSoC '24] (JabRef#11477)
  Bump src/main/resources/csl-styles from `b2be5ae` to `fd6cb3e` (JabRef#11501)
  Bump gittools/actions from 1.1.1 to 1.2.0 (JabRef#11500)
  Bump com.kohlschutter.junixsocket:junixsocket-core from 2.9.1 to 2.10.0 (JabRef#11498)
  Bump commons-logging:commons-logging from 1.3.2 to 1.3.3 (JabRef#11499)
  Bump org.jsoup:jsoup from 1.17.2 to 1.18.1 (JabRef#11497)
  Bump com.kohlschutter.junixsocket:junixsocket-mysql from 2.9.1 to 2.10.0 (JabRef#11496)
  Bump org.openrewrite.recipe:rewrite-recipe-bom from 2.14.0 to 2.15.0 (JabRef#11495)
  FAQ updates (JabRef#11486)
  Update Gradle Wrapper from 8.8 to 8.9.
  Fix Chocolate.bib (JabRef#11491)

# Conflicts:
#	src/main/java/org/jabref/gui/openoffice/OOBibBase.java
#	src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java
#	src/main/java/org/jabref/gui/openoffice/StyleSelectDialogView.java
#	src/main/java/org/jabref/gui/openoffice/StyleSelectDialogViewModel.java
#	src/main/java/org/jabref/preferences/JabRefPreferences.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants