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

Rename labelpattern to bibtexkeypattern and fix pref issue #1704

Merged
merged 9 commits into from
Aug 11, 2016

Conversation

matthiasgeiger
Copy link
Member

@matthiasgeiger matthiasgeiger commented Aug 9, 2016

As mentioned by @lenhard in #1626 (comment) - this is a follow up PR:

  • Rename label pattern to bibtex key pattern.
    • rename Help file reference (at least until 3.6 is released)
    • rename prefs key
  • Integrate label pattern preferences tree into global preferences tree
  • Possible migration of old key patterns (The first time JabRef is run it is checked whether old patterns are there - if yes, they are migrated. Subsequently migration is skipped.)
  • Change in CHANGELOG.md described internal cleanup
  • Tests created for changes old tests still pass
  • Screenshots added (for bigger UI changes) no UI changes
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@matthiasgeiger
Copy link
Member Author

matthiasgeiger commented Aug 9, 2016

@JabRef/developers Two related questions regarding the migration of LabelPatterns:

  • What happens if old prefs after migration? Delete them?
  • What happens if old and new prefs are there? Keep the new ones?

... and why the hell is this test failing now?!

net.sf.jabref.logic.cleanup.CleanupWorkerTest > cleanupCasesAddsBracketAroundAluminiumGalliumArsenid FAILED

    java.lang.NullPointerException

        at net.sf.jabref.logic.formatter.casechanger.ProtectTermsFormatter.format(ProtectTermsFormatter.java:54)

        at net.sf.jabref.logic.cleanup.FieldFormatterCleanup.cleanupSingleField(FieldFormatterCleanup.java:65)

        at net.sf.jabref.logic.cleanup.FieldFormatterCleanup.cleanup(FieldFormatterCleanup.java:45)

        at net.sf.jabref.logic.cleanup.CleanupWorker.cleanup(CleanupWorker.java:56)

        at net.sf.jabref.logic.cleanup.CleanupWorkerTest.cleanupCasesAddsBracketAroundAluminiumGalliumArsenid(CleanupWorkerTest.java:300)

@koppor
Copy link
Member

koppor commented Aug 9, 2016

Keep the old prefs. They are not consuming much space in the registry. If someone starts an old JabRef version, the old patterns should still be there. It might be that users update from <= 3.5 to a far newer version (4.0?) and want to go back to 3.5, because something does not work on 4.0.

Keep the new ones, if old prefs are there.

I foresee that some users will complain that the patterns change when having used an old version and changed the patterns there. But I think, those guys will be seldom :)

@stefan-kolb
Copy link
Member

Probably just keeping them is the easiest alternative, as long as this does not create any side effects.

@matthiasgeiger matthiasgeiger added this to the v3.6 milestone Aug 10, 2016
@matthiasgeiger matthiasgeiger changed the title [WIP] Rename labelpattern to bibtexkeypattern and fix pref issue Rename labelpattern to bibtexkeypattern and fix pref issue Aug 10, 2016
@matthiasgeiger matthiasgeiger added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 10, 2016
@matthiasgeiger
Copy link
Member Author

Chosen approach is: The first time JabRef is run it is checked whether old patterns are there - if yes, they are migrated. Subsequently migration is skipped. Old prefs are not deleted.

Apart from the changes in the help.jabref.org repo this is good to go from my side.

@@ -79,4 +88,43 @@ public static void upgradeSortOrder() {
}
}

/**
* Migrate LabelPattern configuration from version 3.3-3.5 to new BibtexKeyPatterns
Copy link
Member

Choose a reason for hiding this comment

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

Minor comment: Could we also upgrade from <3.3? We still have users coming from JabRef 2.9.2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be possible. I'll implement the logic that the newest available prefs are migrated to 3.6

@koppor
Copy link
Member

koppor commented Aug 11, 2016

Besides the minor comment: LGTM.

As far as I can see, import/export of preferences doesn't include the bibtex key patterns, does it?


}
if (mainPrefsNode.nodeExists("bibtexkeypatterns")) {
return; //Pref node already exists do not migrate from previous version
Copy link
Member

Choose a reason for hiding this comment

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

Could you move the comment above the statement?

@matthiasgeiger
Copy link
Member Author

Prefs were already reported correctly, regarding this point I was wrong at the Dev call. But I should check whether importing is now working fine.

@matthiasgeiger matthiasgeiger removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 11, 2016
@matthiasgeiger matthiasgeiger changed the title Rename labelpattern to bibtexkeypattern and fix pref issue [WIP] Rename labelpattern to bibtexkeypattern and fix pref issue Aug 11, 2016
@stefan-kolb
Copy link
Member

LGTM 👍

@matthiasgeiger matthiasgeiger added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 11, 2016
@matthiasgeiger matthiasgeiger changed the title [WIP] Rename labelpattern to bibtexkeypattern and fix pref issue Rename labelpattern to bibtexkeypattern and fix pref issue Aug 11, 2016
@matthiasgeiger
Copy link
Member Author

Added further migrations fors 3.0-3.2 and <3.0.

Exporting and importing prefs is working with KeyPatterns.

@oscargus
Copy link
Contributor

Does this fix #1257 ? (Except the 3.2 to 3.3 part...) In that case add a ChangeLog entry and remove the wontfix tag. :-)

@matthiasgeiger
Copy link
Member Author

Added the changelog entry.

... can be merged as soon as https://github.com/JabRef/help.jabref.org/pull/46 is merged - which fixes the failing test.

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.

4 participants