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 for Indexing stopping while having library modification #8469

Merged
merged 7 commits into from
Feb 18, 2022

Conversation

GabrielZardoya
Copy link
Contributor

@GabrielZardoya GabrielZardoya commented Jan 27, 2022

Fixes issue #8420. Code follows suggested change in original issue by @tobiasdiez: makes database context a constructor for IndexingTabManager, has IndexingTabManager listen for filename changes and updates title using logic from updateTabTitle method in LibraryTab. Feel free to ask me any questions or make/propose changes as needed.

fixes #8420

  • 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 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.

@Siedlerchr Siedlerchr changed the title [WIP] Attempted Fix [#8420] [WIP] Attempted Fix for Indexing stopping while adding Jan 28, 2022
@Siedlerchr
Copy link
Member

You said you encountered some issues, can you explain it a bit better.

@GabrielZardoya
Copy link
Contributor Author

GabrielZardoya commented Jan 30, 2022

You said you encountered some issues, can you explain it a bit better.

I mean it didn't pass some test cases in gradlew check. It looks like it works to solve the problem running it manually, though I'm assuming since there are failed test cases it's breaking something else I'm not aware of.

It fails these tests in particular:

  • testPortAlreadyInUse()
  • fieldDoesNotAcceptUmlauts()
  • NonUTF8EncodingCheckerTest()
  • studyResultsPersistedCorrectly()
  • fetcherResultsPersistedCorrectly()
  • mergedResultsPersistedCorrectly()

@Siedlerchr
Copy link
Member

They are not relevant to your changes and you can ignore them. If the tests on the CI pass its fine. (fetcher test can be ignored if you didn't change a fetcher)

@GabrielZardoya GabrielZardoya marked this pull request as ready for review January 30, 2022 16:16
@GabrielZardoya
Copy link
Contributor Author

GabrielZardoya commented Jan 30, 2022

They are not relevant to your changes and you can ignore them. If the tests on the CI pass its fine. (fetcher test can be ignored if you didn't change a fetcher)

OK, thanks for informing me. It should be good then to review, if you have any issues or questions let me know.

@Siedlerchr Siedlerchr changed the title [WIP] Attempted Fix for Indexing stopping while adding [WIP] Attempted Fix for Indexing stopping while having library modification Jan 30, 2022
@Siedlerchr
Copy link
Member

Okay, then if your PR is ready then remove the WIP and it would be nice if you could explain your changes a bit here in the PR description what the fix is/how it works.

@GabrielZardoya GabrielZardoya changed the title [WIP] Attempted Fix for Indexing stopping while having library modification Attempted Fix for Indexing stopping while having library modification Jan 30, 2022
@GabrielZardoya GabrielZardoya changed the title Attempted Fix for Indexing stopping while having library modification Fix for Indexing stopping while having library modification Jan 30, 2022
@GabrielZardoya
Copy link
Contributor Author

Okay, then if your PR is ready then remove the WIP and it would be nice if you could explain your changes a bit here in the PR description what the fix is/how it works.

OK, let me know if you have any other questions or recommendations.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 30, 2022
@Siedlerchr Siedlerchr requested a review from btut January 31, 2022 10:43
Copy link
Contributor

@btut btut left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@btut
Copy link
Contributor

btut commented Feb 4, 2022

@tobiasdiez could you check if this solves your issue?

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. One suggestion to improve the code a bit. Please also add a change log entry. Otherwise good to go from my side.

GabrielZardoya and others added 2 commits February 13, 2022 11:36
Co-authored-by: Tobias Diez <code@tobiasdiez.com>
@GabrielZardoya
Copy link
Contributor Author

Generally looks good to me. One suggestion to improve the code a bit. Please also add a change log entry. Otherwise good to go from my side.

I updated CHANGELOG.md and committed your suggestion to the PR. Let me know if there is anything else you would recommend.

@tobiasdiez
Copy link
Member

Thanks! There are still some merge conflicts. Could you please take care of them as well?

@GabrielZardoya
Copy link
Contributor Author

Thanks! There are still some merge conflicts. Could you please take care of them as well?

Alright, I believe all the conflicts should be fixed now. Checked to make sure it still fixes the issue and it does after some modifications. Let me know if there is anything you would like to change with the new revisions.

@koppor koppor merged commit d6fec0b into JabRef:main Feb 18, 2022
Siedlerchr added a commit that referenced this pull request Feb 19, 2022
@Siedlerchr
Copy link
Member

Siedlerchr commented Feb 19, 2022

I had to revert the commit because after opening a file the library tab shows untitled! /File open recent
@GabrielZardoya Please look into this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library modification appearently triggers reindexing of all PDFs
5 participants