Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jul 16, 2024
Merged

Conversation

koppor
Copy link
Member

@koppor koppor commented Jul 15, 2024

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
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
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
Member

Choose a reason for hiding this comment

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

sortedset

Copy link
Member Author

@koppor koppor Jul 16, 2024

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
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
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
Copy link
Contributor

github-actions bot commented Jul 16, 2024

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
21 checks passed
@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