Refactor BibDatabase Migrations#3733
Conversation
|
Why logic.importer.migrations? Sure it would not be in a wrong place, but we put it in a top-level package on purpose as this should be immediately evident that this is caused by JabRef design decisions and not hidden inside logic.importer where standard formats should be put. Not sure what others think. |
|
Agree with @stefan-kolb What has a migration to do with import? |
|
+1 for logic.migrations |
| @@ -1,4 +1,4 @@ | |||
| package org.jabref.migrations; | |||
| package org.jabref.logic.importer.migrations; | |||
There was a problem hiding this comment.
This class has to many UI-dependencies to be in logic.
|
The failing test is probably due to caching on travis and a result from #3713. |
| import org.jabref.logic.importer.ParserResult; | ||
| import org.jabref.logic.migrations.MergeReviewIntoCommentMigration; | ||
|
|
||
| public class MergeReviewIntoComment implements GUIPostOpenAction { |
There was a problem hiding this comment.
Maybe rename that with the action suffix so that it is better distingusishable from the other migration
| import org.jabref.model.entry.BibEntry; | ||
|
|
||
| public class MergeReviewIntoCommentUIManager { | ||
| public class MergeReviewIntoCommentConfirmation { |
There was a problem hiding this comment.
I would add the Dialog suffix to better see that above
| MergeReviewIntoCommentMigration migration = new MergeReviewIntoCommentMigration(); | ||
|
|
||
| migration.performMigration(parserResult); | ||
| if (new MergeReviewIntoCommentConfirmation(basePanel).askUserForMerge(MergeReviewIntoCommentMigration.collectConflicts(parserResult))) { |
There was a problem hiding this comment.
I would split that into assignment and declaration. It's unforunately not so easy distinguishable what what ist, because all is named very similar
Siedlerchr
left a comment
There was a problem hiding this comment.
If you make the class names a bit more distinguishable from each other, I would be happy.
Rest is okay 👍
lenhard
left a comment
There was a problem hiding this comment.
Code looks good and I'll merge. At some point, we can think about removing the FileLinksUpgradeWarning migration completely.
v2.3 is so long back and by removing this we could speed up the startup a little.
This is the followup for #3658 (comment).
I have moved the functionality from org.jabref.migrations to
org.jabref.logic.importer.migrations.The code is called from the UI now in
org.jabref.gui.importer.actions.OpenDatabaseAction.performPostOpenActions(), where also some other similar actions are called.Since the build is broken at the moment due to external resources, I'll have to re-check later if everything is actually fine here.
The general structure should be ready to review anyways.