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 DOI resolving by using https #2889

Merged
merged 4 commits into from
Jun 6, 2017
Merged

Fix DOI resolving by using https #2889

merged 4 commits into from
Jun 6, 2017

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Jun 5, 2017

Fix for #2879
I think this is an important issue with small code change. @JabRef/developers Should we do a fix for 3.8,2?

For 3.8.2 https://github.com/JabRef/jabref/blob/v3.8.2/src/main/java/net/sf/jabref/logic/util/DOI.java

  • Change in CHANGELOG.md described
  • Tests created for change
    - [ ] Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
    - [ ] Check documentation status (Issue created for outdated help page at [help.jabref.org](https://github.com/JabRef/help.jabref.org/issues)?)
    - [ ] If you changed the localization: Did you run gradle localizationUpdate?

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 5, 2017
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

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

LGTM, consider to write a more specific changelog message

CHANGELOG.md Outdated
@@ -26,6 +26,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We added integration of the Library of Congress catalog as a fetcher based on the [LCCN identifier](https://en.wikipedia.org/wiki/Library_of_Congress_Control_Number). [Feature request 636 in the forum](http://discourse.jabref.org/t/loc-marc-mods-connection/636)

### Fixed
- We fixed the adding of a new entry from DOI which led to a connection error [#2879](https://github.com/JabRef/jabref/issues/2897)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Improve wording to make clear that now HTTPS is used to protect the privacy of the user.

@Siedlerchr Siedlerchr merged commit ad69741 into master Jun 6, 2017
@Siedlerchr Siedlerchr deleted the fixDoi branch June 6, 2017 07:27
Siedlerchr added a commit that referenced this pull request Jun 6, 2017
* upstream/master:
  Fix DOI resolving by using https (#2889)
  Adjust tests: Ads fetcher returns url, DBLP no longer works with negative operators (#2891)
  Fix loading of preferencesService (#2882)
  Add link to "feature branch workflow"
  Support Annotations Created by Foxit (#2878)
  Fixes jacoco by excluding the fetcher tests from analysis (#2877)

# Conflicts:
#	src/main/java/org/jabref/gui/DefaultInjector.java
@ghost ghost mentioned this pull request Jun 13, 2017
stefan-kolb added a commit that referenced this pull request Jul 20, 2017
* Fix DOI resolving by using https

* Add changelog

* Fix DOI layout test

* Reword changelog

# Conflicts:
#	CHANGELOG.md
#	src/main/java/org/jabref/model/entry/identifier/DOI.java
#	src/test/java/org/jabref/logic/layout/format/DOICheckTest.java
#	src/test/java/org/jabref/model/entry/identifier/DOITest.java
@gmonet
Copy link

gmonet commented Jul 29, 2017

Any binairie install plane? I can't create mine from jabref.install4j because i need a 'Windows key store'. I don't get what it means but I really need this update.
Thanks :)

@matthiasgeiger
Copy link
Member

This fix is already part of 4.0-beta2 or the recent Dev builds which can be downloaded here: https://builds.jabref.org/master

So you don't need to build JabRef on your own.

Best,
Matthias

@gmonet
Copy link

gmonet commented Jul 29, 2017

Thanks for your (amazing) fast answer.
Actually, i was speaking about Jabref 3.8. I tried 4.0-beta 2 but there are still some bugs (especially group display) and it crashes frequently.
The comit 28222e2 fixes DOI's problem for Jabred 3.8 (3.8.3 in the changelog) but it seems it's not distributed yet.

Thanks :)

@jonasstein
Copy link

We are very interested on a 3.8.3 version for Gentoo too.

@stefan-kolb
Copy link
Member

I don't think we will release a 3.8.3. The only active branch and release version will be 4.x.

@jonasstein
Copy link

jonasstein commented Nov 1, 2017

Is there a special reason for that? It does not seem that the 4.x tree will have the stability of 3.8.x in the near future. We have many users who were interested in working on 3.8.2, but need doi. We provide 4.x now for the interested hacker on Gentoo with every new snapshot. But for now for example the snapshot is very broken and 4.0 release freezes when dragging to a group.
Is the procedure for 3.8.2 and 3.8.3 not the same? Are all old developers gone?

@LinusDietz
Copy link
Member

LinusDietz commented Nov 1, 2017

Resources, ancient & hard to maintain technology.

@lenhard
Copy link
Member

lenhard commented Nov 2, 2017

@jonasstein Regarding the freeze when dragging to a group, one thing just came to my mind: What is the look and feel you are using? We had a few Linux users who experienced freezes with the GTK look and feel. Switching to a different look and feel (e.g. plastic) effectively solved their problem. Does that maybe also help in your case? If so, the trivial solution would be to set a different default look and feel on Linux. Maybe the solution is also too hidden in the documentation: https://help.jabref.org/en/Installation#freezes-when-running-jabref

@jonasstein
Copy link

jonasstein commented Nov 2, 2017

It was this one:
selection_383

Which is gtk? I would love a real classic gtk look. The "modern" design may look interesting but are not so ergonomic for office work in my opinion.
I found that bug too and tried also all others. But this did not change the freeze.
selection_384
in the second picture we see also #3391

@lenhard
Copy link
Member

lenhard commented Nov 2, 2017

Ok, too bad. Thanks for testing!

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.

8 participants