-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 the Review Tab into Comments Tab #3658
Merged
Merged
Changes from 23 commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
289c2d2
Rename the Review Tab into Comments Tab
LinusDietz 351b1db
Remove Localization Key for Review
LinusDietz ac341e4
Merge branch 'master' into comment-tab
LinusDietz 07a8965
Use Comment field in the Comment tab
LinusDietz 6a75c6a
Migration for the Review field
LinusDietz af9022a
Merge remote-tracking branch 'origin/comment-tab' into comment-tab
LinusDietz afe5476
Remove Comment Field from General Tab
LinusDietz 995b965
Tests
LinusDietz 056ef9d
Simplify tests
LinusDietz 9d9c5a6
Fix Codacy
LinusDietz 4a6886e
Package refactoring, streamline naming
LinusDietz a5a2994
Also move tests
LinusDietz 37a247c
Merge branch 'master' of https://github.com/JabRef/jabref into commen…
LinusDietz 1e0d56e
Add Confirmation Dialog for Merging Fields
LinusDietz e949025
Localize Tests
LinusDietz c93b490
Fix tests
LinusDietz e405f67
Fix Codacy
LinusDietz 6cca4ab
fixed issue 298
grotepfn b7ca150
updated Changes
grotepfn 38750e8
Update CHANGELOG.md
grotepfn 2d2bb7e
User decides now once for all migrations.
LinusDietz dfeda8d
fix localization
LinusDietz 4828955
Merge pull request #3726 from grotepfn/patch-3
lenhard 5962710
Remove 'no' button
LinusDietz 4c1905d
Merge branch 'master' of https://github.com/JabRef/jabref into commen…
LinusDietz 38e6139
Merge branch 'comment-tab' of https://github.com/JabRef/jabref into c…
LinusDietz b246cfe
Mark Database as changed after the LoadDatabaseMigration
LinusDietz 303ed4d
Getters and Setters
LinusDietz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
.../autosaveandbackup/AutosaveUIManager.java → ...jabref/gui/dialogs/AutosaveUIManager.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
...ui/autosaveandbackup/BackupUIManager.java → ...g/jabref/gui/dialogs/BackupUIManager.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
package org.jabref.gui.autosaveandbackup; | ||
package org.jabref.gui.dialogs; | ||
|
||
import java.nio.file.Path; | ||
|
||
|
31 changes: 31 additions & 0 deletions
31
src/main/java/org/jabref/gui/dialogs/MergeReviewIntoCommentUIManager.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
package org.jabref.gui.dialogs; | ||
|
||
import java.util.List; | ||
import java.util.Optional; | ||
import java.util.stream.Collectors; | ||
|
||
import javax.swing.JOptionPane; | ||
|
||
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.model.entry.BibEntry; | ||
|
||
public class MergeReviewIntoCommentUIManager { | ||
|
||
public boolean askUserForMerge(List<BibEntry> conflicts) { | ||
List<String> bibKeys = conflicts.stream() | ||
.map(BibEntry::getCiteKeyOptional) | ||
.filter(Optional::isPresent) | ||
.map(Optional::get) | ||
.collect(Collectors.toList()); | ||
|
||
int answer = JOptionPane.showConfirmDialog( | ||
null, | ||
String.join(",\n", bibKeys) + " " + | ||
Localization.lang("has/have both a 'Comment' and a 'Review' field.") + "\n" + | ||
Localization.lang("Since the 'Review' field was deprecated in JabRef 4.2, these two fields are about to be merged into the 'Comment' field.") + "\n" + | ||
Localization.lang("By clicking 'Yes' the conflicting fields of these entries will be merged into the 'Comment' field."), | ||
Localization.lang("Review Field Migration"), JOptionPane.YES_NO_OPTION, JOptionPane.WARNING_MESSAGE); | ||
|
||
return 0 == answer; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 0 additions & 7 deletions
7
src/main/java/org/jabref/logic/importer/util/PostOpenAction.java
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
69 changes: 69 additions & 0 deletions
69
src/main/java/org/jabref/migrations/MergeReviewIntoComment.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
package org.jabref.migrations; | ||
|
||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.Collectors; | ||
|
||
import javafx.collections.ObservableList; | ||
|
||
import org.jabref.gui.dialogs.MergeReviewIntoCommentUIManager; | ||
import org.jabref.logic.importer.ParserResult; | ||
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.model.entry.BibEntry; | ||
import org.jabref.model.entry.FieldName; | ||
|
||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class MergeReviewIntoComment implements PostOpenMigration { | ||
public static final Logger LOGGER = LoggerFactory.getLogger(MergeReviewIntoComment.class); | ||
|
||
@Override | ||
public void performMigration(ParserResult parserResult) { | ||
ObservableList<BibEntry> entries = Objects.requireNonNull(parserResult).getDatabase().getEntries(); | ||
|
||
// migrate non-conflicting entries first | ||
entries.stream() | ||
.filter(this::hasReviewField) | ||
.filter(entry -> !this.hasCommentField(entry)) | ||
.forEach(this::migrate); | ||
|
||
// determine conflicts | ||
List<BibEntry> conflicts = entries.stream() | ||
.filter(this::hasReviewField) | ||
.filter(this::hasCommentField) | ||
.collect(Collectors.toList()); | ||
|
||
// resolve conflicts if users agrees | ||
if (!conflicts.isEmpty() && new MergeReviewIntoCommentUIManager().askUserForMerge(conflicts)) { | ||
conflicts.stream() | ||
.filter(this::hasReviewField) | ||
.forEach(this::migrate); | ||
} | ||
} | ||
|
||
private String mergeCommentFieldIfPresent(BibEntry entry, String review) { | ||
if (entry.getField(FieldName.COMMENT).isPresent()) { | ||
LOGGER.info(String.format("Both Comment and Review fields are present in %s! Merging them into the comment field.", entry.getAuthorTitleYear(150))); | ||
return String.format("%s\n%s:\n%s", entry.getField(FieldName.COMMENT).get().trim(), Localization.lang("Review"), review.trim()); | ||
} | ||
return review; | ||
} | ||
|
||
private boolean hasCommentField(BibEntry entry) { | ||
return entry.getField(FieldName.COMMENT).isPresent(); | ||
} | ||
|
||
private boolean hasReviewField(BibEntry entry) { | ||
return entry.getField(FieldName.REVIEW).isPresent(); | ||
} | ||
|
||
private void migrate(BibEntry entry) { | ||
updateFields(entry, mergeCommentFieldIfPresent(entry, entry.getField(FieldName.REVIEW).get())); | ||
} | ||
|
||
private void updateFields(BibEntry entry, String review) { | ||
entry.setField(FieldName.COMMENT, review); | ||
entry.clearField(FieldName.REVIEW); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package org.jabref.migrations; | ||
|
||
import org.jabref.logic.importer.ParserResult; | ||
|
||
public interface PostOpenMigration { | ||
void performMigration(ParserResult parserResult); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of this code should move to the logic package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I told him to move it there, because I want to have the migrations together. The migrations package should probably go into logic, but some of the migration classes are awefully coupled to UI classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, then the location is fine. But at least the new class should be completely decoupled in logic and gui since otherwise the migration of the migration package is getting harder with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobiasdiez I guess you mean this line?
jabref/src/main/java/org/jabref/migrations/MergeReviewIntoComment.java
Line 38 in b246cfe
I can create another Class for this confirmation, but where would you put that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no deep knowledge about the migration code, so the following may be a bit naïve. For me, the expected flow is something like:
actionNeeded
and maybe asks the user for confirmation, then runsperformAction
on the migration action.Not sure how much of this architecture is already in place and how much you are willing to refactor for this PR. My initial comment was triggered by the fact that a new class was created that mixed UI and logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I totally support your point, however this is all a bit tricky, as these PostOpenMigrations are all called before the main GUI is instatiated.
Suggestion: Lets merge this PR now, and I'll refactor the code in a sensible way in a followup PR. (I'm on the train tomorrow for at least 5 hours)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine with me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The follow-up was done at #3733