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

Library modification appearently triggers reindexing of all PDFs #8420

Closed
2 tasks done
tobiasdiez opened this issue Jan 12, 2022 · 12 comments · Fixed by #8469 or #9166
Closed
2 tasks done

Library modification appearently triggers reindexing of all PDFs #8420

tobiasdiez opened this issue Jan 12, 2022 · 12 comments · Fixed by #8469 or #9166
Labels
bug Confirmed bugs or reports that are very likely to be bugs external files good first issue An issue intended for project-newcomers. Varies in difficulty. ui

Comments

@tobiasdiez
Copy link
Member

JabRef version

5.4 (latest release)

Operating system

Windows

Details on version and operating system

Windows 11

Checked with the latest development build

  • I made a backup of my libraries before testing the latest development version.
  • I have tested the latest development version and the problem persists

Steps to reproduce the behaviour

  1. Have a library with at least one entry having a PDF.
  2. Give JabRef time to generate the index.
  3. Change something in the database.
  4. Go to the background notification tab, where you get something like
    image

Note the last item, from which one gets the impression that the whole index has been rebuild.

However, looking at the code:

indexingTaskManager.updateDatabaseName(tabTitle.toString());

is triggered by the "rename of the tab" (i.e. appending the star to signal the change of the library).
But luckily
public void updateDatabaseName(String name) {
DefaultTaskExecutor.runInJavaFXThread(() -> this.titleProperty().set(Localization.lang("Indexing for %0", name)));
}

only updates the name, and doesn't really create a new index.

Proposed change:

  • Extract some of the logic of updateTabTitle to get a short display name for a database to a helper method.
  • Make the database context a constructor argument of the indexing task manager (to make it clear that each library has its own manager)
  • Then in the indexing task manager listen to changes of the filename (i.e rename) and update the title (using the extracted helper method from above)

cc @btut

Appendix

No response

@tobiasdiez tobiasdiez added bug Confirmed bugs or reports that are very likely to be bugs ui external files labels Jan 12, 2022
@tobiasdiez tobiasdiez added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Jan 12, 2022
GabrielZardoya added a commit to GabrielZardoya/jabref that referenced this issue Jan 22, 2022
@Siedlerchr Siedlerchr reopened this Feb 19, 2022
@LokeshSingh1102
Copy link

Hi,
I'm interested in working on this issue, it's my first code contribution to open-source.

@ThiloteE
Copy link
Member

@LokeshSingh1102 Not sure if you are still interested and also I am not sure, if this issue is still relevant, because there have been some changes to indexing I believe, but if so then here would be my general advice:

Check out https://github.com/JabRef/jabref/blob/main/CONTRIBUTING.md for a start. Also, https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace is worth having a look at. Feel free to ask if you have any questions here on GitHub or also at JabRef's Gitter chat.

Try to open a (draft) pull request early on, so that people can see you are working on the issue and so that they can see the direction the pull request is heading towards. This way, you will likely receive valuable feedback.

@ThiloteE ThiloteE removed the good first issue An issue intended for project-newcomers. Varies in difficulty. label Sep 10, 2022
@ThiloteE
Copy link
Member

@btut is this issue here still relevant?

@btut
Copy link
Contributor

btut commented Sep 11, 2022

Hi! Yes I think this should still be fixed but I would rather not touch the indexing before the lucene-search-backend is merged, otherwise we may have tons of conflicts.

@ThiloteE ThiloteE added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Sep 11, 2022
@btut btut pinned this issue Sep 11, 2022
@ilippert
Copy link
Contributor

JabRef 5.8--2022-09-08--e970954
Linux 5.19.7-200.fc36.x86_64 amd64
Java 18.0.2.1
JavaFX 18.0.2+2

I did not note (but also did not specifically check) this issue for some months. But currently I perceive it.

@btut
Copy link
Contributor

btut commented Sep 16, 2022

Hi @ilippert,
I noticed another issue that re-indexing is done on each startup. Do you mean that or do you have multiple re-indexings happening while working with jabref (not only on startup)?

@ilippert
Copy link
Contributor

I noticed another issue that re-indexing is done on each startup. Do you mean that or do you have multiple re-indexings happening while working with jabref (not only on startup)?

Thank you for the note! just checked on

JabRef 5.8--2022-09-15--853338a
Linux 5.19.8-200.fc36.x86_64 amd64
Java 18.0.2.1
JavaFX 19+11

the re-indexing happens when starting up, not for the library modification.

@btut
Copy link
Contributor

btut commented Sep 18, 2022

I fixed that bug already in the lucene-backend branch but can port it to main sometime soon since the lucene things still need plenty of work.

@Siedlerchr
Copy link
Member

@btut A backport into the main would be great. Can you give us a hint which line(s) are relevant=?

@btut
Copy link
Contributor

btut commented Sep 19, 2022

Should be
4412f6e
And
4b17f10

The second one fixes the re-indexing every time, the first one also adds an entry to the index for pdfs that could not be read (font errors, IO errors) and does not try to re-index them at the next start (since it can be assumed to fail again).

@Siedlerchr
Copy link
Member

Thanks! Should be easy to just cherry pick these commits. I will give it a try tomorrow

@btut
Copy link
Contributor

btut commented Sep 20, 2022

I am not so sure about that, as the lucene branch moves plenty of files (because they had pdf in their name and now more than pdfs are indexed). I have some time later today to handle this.

@ThiloteE ThiloteE unpinned this issue Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs external files good first issue An issue intended for project-newcomers. Varies in difficulty. ui
Projects
Archived in project
6 participants