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 the pages being out of order #77

Closed
wants to merge 2 commits into from

Conversation

callisto-jovy
Copy link

This resolves #76.

Instead of iterating over the contents of the directory and inserting them based on a matched predicate, we count the number of matched entries and generate the paths ourselves.

Simply fixed by counting the number of entries matched against the existing predicate and inserting them into the array in a secondary loop.
@callisto-jovy
Copy link
Author

I'm not an experienced JavaScript developer, feel free to make changes.

Copy link
Member

@stweil stweil left a comment

Choose a reason for hiding this comment

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

A lot of the changes in this pull request simply reformat the code. Would you mind changing your PR and keep the original lines and style?

Maybe you can also just use the code from Zotero 6 to fix the issue.

Both suggested changes would make the review easier.

@callisto-jovy
Copy link
Author

Shall I just replace my code with the old Zotero 6 code, using await?

@callisto-jovy
Copy link
Author

Does this now look better @stweil?

@aborel
Copy link
Collaborator

aborel commented Jul 8, 2024

Sorry about my lack of reactivity, I have been unable to read the proposed code until yesterday.
Thanks a lot for reporting the bug, and thanks for the PR! However, I don't think it is evolving in the right direction.

The first commit would have been easier to review without the style changes. In the 2nd one, I don't think returning to Zotero6-compatible code is the way to go.

I have a much simpler fix in mind: since imageListArray is not guaranteed to be in the right order, sorting it before writing imagelist.txt should do the trick. I will push the commit in a minute, but it is best if @stweil or @zuphilip publishes the new version.

@stweil
Copy link
Member

stweil commented Jul 8, 2024

Thanks again for your bug report and your efforts with this pull request. Meanwhile a different fix was applied and released in release 0.7.3, so this one here is no longer needed.

@stweil stweil closed this Jul 8, 2024
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.

Pages in Image-list.txt are not guaranteed to be in order.
3 participants