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

Improved progress indication for fulltext-index operations #7981

Merged
merged 7 commits into from
Aug 19, 2021

Conversation

btut
Copy link
Contributor

@btut btut commented Aug 11, 2021

Operations on the full-text index are now more fine-grained. Instead of adding a whole database at once, files are added individually so we can display a more sensible progress in the background-task menu. The progress is now also counted towards the average progress of background processes and therefore shown by the progress indicator at the top right corner.

Peek 2021-08-18 12-04

Fixes #7961.

  • 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 created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

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.

Thanks!

I don't have much time right now, so just a few initial remarks from my side

@calixtus
Copy link
Member

Checkstyle: Unused imports in IndexingTaskManager and PdfIndexer

@btut
Copy link
Contributor Author

btut commented Aug 18, 2021

This is basically a follow up to a feature PR that is not yet part of any release. Shall I add an entry in the Changelog anyways?

@btut btut requested a review from tobiasdiez August 18, 2021 10:07
@calixtus
Copy link
Member

calixtus commented Aug 18, 2021

No. Changelog should only describe changes from one release to the next.

@btut btut added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 19, 2021
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

lgtm

@Siedlerchr
Copy link
Member

Seems like there are some L10n errors

@btut
Copy link
Contributor Author

btut commented Aug 19, 2021

L10n passes now.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I merge to have a better experience for the erlier adapters.

@koppor koppor merged commit 05a0ce9 into JabRef:main Aug 19, 2021
Siedlerchr added a commit that referenced this pull request Aug 20, 2021
* upstream/main: (110 commits)
  Extract PushTo names into model (#8005)
  Refactor processCitation in GrobidService to match processPdf (#8003)
  Improved progress indication for fulltext-index operations (#7981)
  Reordered Pdf-Importer priorities (#8001)
  Implement more pdf importers (#7947)
  Adding icon picker for group dialog issue#6142 (#7776)
  Fix possible NPE in exporter with empty charset (#7979)
  Fix icon color (#7994)
  Bump slf4j-api from 2.0.0-alpha2 to 2.0.0-alpha4 (#7991)
  Bump classgraph from 4.8.112 to 4.8.114 (#7990)
  Bump mariadb-java-client from 2.7.3 to 2.7.4 (#7992)
  Bump jsoup from 1.14.1 to 1.14.2 (#7993)
  New yaml issue template (#7983)
  [Bot] Update CSL styles (#7985)
  Reordered items in main table right-click menu (#7952)
  Fulltext Index: Only index local pdf files (#7980)
  Bump WyriHaximus/github-action-wait-for-status from 1.3 to 1.4 (#7973)
  Bump byte-buddy-parent from 1.11.9 to 1.11.12 (#7974)
  Bump classgraph from 4.8.110 to 4.8.112 (#7975)
  Bump checkstyle from 8.45 to 8.45.1 (#7978)
  ...

# Conflicts:
#	src/main/java/module-info.java
@ilippert
Copy link
Contributor

ilippert commented Aug 30, 2021

looks wonderful on JabRef 5.4--2021-08-23--cef4151
Linux 5.13.12-200.fc34.x86_64 amd64
Java 16.0.2
JavaFX 16+8

however, when indexing two files, the progress indication is not well intelligible for me. I have a file A with +7k attached files, another B with 0.5k attached files. After a while, the progress was half full (A and B both a few hundred), but after B was completed, progress was indicated as "lower" than before.

@btut
Copy link
Contributor Author

btut commented Aug 30, 2021

looks wonderful on JabRef 5.4--2021-08-23--cef4151
Linux 5.13.12-200.fc34.x86_64 amd64
Java 16.0.2
JavaFX 16+8

Yay!

however, when indexing two files, the progress indication is not well intelligible for me. I have a file A with +7k attached files, another B with 0.5k attached files. After a while, the progress was half full (A and B both a few hundred), but after B was completed, progress was indicated as "lower" than before.

The indicator shows the average progress of running tasks. So if 75% of files in B are indexed and 25% the average is 50%. A little later, all files in B are indexed but let's say A is only at 30%. The indicator will show 30% because the task for B is already done and not considered for the average.
I think this is better than counting all tasks towards the progress forever, because given enough tasks that are already done and one task that just started, the average would be very high because of all the finished tasks.
Also, I think the progress indicator is more important so users can tell something is done in the background. IMO, the actual progress of the tasks is not that important.

The current behavior is also how other programs often show progress. Just look at the download-progress indicator of most browsers. If you download multiple files, once the first is done the indicator will jump back, just as it does now in JabRef.

If you have a better idea how to handle this, let me know.

@ilippert
Copy link
Contributor

The indicator shows the average progress of running tasks. So if 75% of files in B are indexed and 25% the average is 50%. A little later, all files in B are indexed but let's say A is only at 30%. The indicator will show 30% because the task for B is already done and not considered for the average.

Ah, ok, get it. Thank you for the explanation.

I think this is better than counting all tasks towards the progress forever, because given enough tasks that are already done and one task that just started, the average would be very high because of all the finished tasks.

I get the idea of showing the current progress relative to what more needs to be indexed.

Also, I think the progress indicator is more important so users can tell something is done in the background. IMO, the actual progress of the tasks is not that important.

I very much agree.

The current behavior is also how other programs often show progress. Just look at the download-progress indicator of most browsers. If you download multiple files, once the first is done the indicator will jump back, just as it does now in JabRef.

If you have a better idea how to handle this, let me know.

how about always use the sum of all files across the ongoing task(s) instead of per library? say to be indexed in A: 10k, already indexed 1k; to be indexed in B 1k, already indexed 0.5k. Then 1.5/11=14%

@btut
Copy link
Contributor Author

btut commented Aug 31, 2021

how about always use the sum of all files across the ongoing task(s) instead of per library? say to be indexed in A: 10k, already indexed 1k; to be indexed in B 1k, already indexed 0.5k. Then 1.5/11=14%

Then what about other background tasks (such as file download)? Their progress is inherently different (Bytes received / File Size).

@ilippert
Copy link
Contributor

Then what about other background tasks (such as file download)? Their progress is inherently different (Bytes received / File Size).

ok, yes, no easy response to that.

I completely accept the incommensurabilities you point to. Thus, you have convinced me there is no simple change.

To reiterate my happiness: the progress indication is great! Thanks!

@btut
Copy link
Contributor Author

btut commented Aug 31, 2021

Yay!

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

Successfully merging this pull request may close these issues.

Fulltext search: Show display of index creation progress
6 participants