Skip to content

[WIP] fix-for-issue-4113#4188

Closed
1160200515 wants to merge 13 commits into
JabRef:masterfrom
1160200515:fix-for-issue-4113
Closed

[WIP] fix-for-issue-4113#4188
1160200515 wants to merge 13 commits into
JabRef:masterfrom
1160200515:fix-for-issue-4113

Conversation

@1160200515

@1160200515 1160200515 commented Jul 10, 2018

Copy link
Copy Markdown

Refs #4113


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change in itself looks fine to me. Good job!

Please add a few test cases to PagesCheckerTest which confirm that the issue is indeed fixed.

@1160200515

Copy link
Copy Markdown
Author

I got it thank you.

Comment thread CHANGELOG.md

### Fixed
- We fixed an issue about improving check for page numbers.
(https://github.com/JabRef/jabref/issues/4113)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No newline here, please.

tobiasdiez and others added 10 commits July 12, 2018 21:54
* Convert IEEE fetcher to new interface

* Move JSON parsing to respective fetcher

* Remove obsolete localization keys
* Fix JabRef#4177: New entry dialog freezes no longer

* Error messages on JavaFX thread
* Make global font size customizable

* Don't be fancy and just use ordinary boolean or

* Add missing localization

* Add missing localization
* Convert CiteSeerX fetcher to new infrastructure

The old implementation first extracted the detail pages for each article matched by the query and then went to every detail page to extract the bibliographic information. Instead, we now parse the COinS information that is already contained in the main result page and thereby reduce the number of requests to 1 per query.

* Add additional test
@tobiasdiez

Copy link
Copy Markdown
Member

It appears that something went wrong and some other commits from master were added to this PR. Can you please rebase/merge the master so that only your changes remain. Thanks.

@1160200515

Copy link
Copy Markdown
Author

I am sorry I don't understand. what should I do ?

@tobiasdiez

Copy link
Copy Markdown
Member

Have a look at the diff here in github. There is a lot of code that is marked as changed although you didn't touch it. These should vanish when use git pull origin/master and then git push.

@1160200515

Copy link
Copy Markdown
Author

I am sorry that I am not familiar with git, so what should I do?

@1160200515

Copy link
Copy Markdown
Author

I have previously synchronized the PR with the master. Do I need to roll back the code?

@LinusDietz

Copy link
Copy Markdown
Member

The problem is that this PR contains all kinds of commits that are unrelated to this PR. @1160200515 It would be great if you could fix this. Probably the easiest thing would be to do this in another branch?

If this is not possible we will have to close this PR, which would be kinda sad, since this would mean that your contribution will not be included into JabRef.

@LinusDietz LinusDietz added component: ui status: waiting-for-feedback The submitter or other users need to provide more information about the issue and removed component: ui labels Sep 20, 2018
@tobiasdiez

Copy link
Copy Markdown
Member

@1160200515 can you please remove the unrelated commits (maybe a simple git pull upstream master is enough), because otherwise we cannot review and include this PR. Please also add tests.

@tobiasdiez tobiasdiez changed the title fix-for-issue-4113 [WIP] fix-for-issue-4113 Oct 9, 2018
tobiasdiez added a commit that referenced this pull request Nov 19, 2018
Superseeds the PR #4188 and fixes #4113. Now page ranges like `R2-R5` are considered to be correct.
@tobiasdiez tobiasdiez mentioned this pull request Nov 19, 2018
Siedlerchr pushed a commit that referenced this pull request Nov 20, 2018
Superseeds the PR #4188 and fixes #4113. Now page ranges like `R2-R5` are considered to be correct.
@tobiasdiez

Copy link
Copy Markdown
Member

Superseeded by #4498

@tobiasdiez tobiasdiez closed this Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: waiting-for-feedback The submitter or other users need to provide more information about the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants