Rename the Review Tab into Comments Tab #3658

LinusDietz · 2018-01-22T14:30:51Z

After a discussion with colleagues, it became apparent, that the Review Tab is maybe named a bit misleadingly. After all I know it is meant for comments about this paper, and I guess few really use it for actual peer-reviews.

~~To keep consistency, I have just changed the wording.~~

I wrote a migration and handled conflict cases by asking the user.

Change in CHANGELOG.md described
Manually tested changed features in running JabRef
Test created
Check documentation status (Issue created for outdated help page at help.jabref.org?)

tobiasdiez

At first, I was also confused about the "review" tab. So, I welcome the change. I'm not completely sold on "Comments" - maybe "Notes"? - but I've no strong opinion here.

I think, however, that the field name (in the bib file) should match the tab name and thus would ask you to add a migration step that renames (or at least change the default fieldname and make Review an alias). Moreover, it would be nice if you could rename all the classes involved (e.g. ReviewTab)

LinusDietz · 2018-01-22T14:50:41Z

@tobiasdiez I did not change the field name to ensure backwards compatibility. (otherwise we would have to write a migration)
Is this a good (enough) argument?

Second, the Tab is not an own class, but a Custom Tab, initialized here:

jabref/src/main/java/org/jabref/preferences/JabRefPreferences.java

Line 978 in cf8e6aa

defaults.put(CUSTOM_TAB_FIELDS + "_def2", FieldName.REVIEW);

tobiasdiez · 2018-01-22T15:18:58Z

Yeah, backwards compatibility is a good point but on the other hand I also find it confusing that the content of the "comments" tab is stored in the field review. I don't know how to proceed. Maybe we should indeed write a migration...

LinusDietz · 2018-01-22T17:32:57Z

Well, I just realized that the Comments field is also displayed in the General tab. :/ Thus, is might be that users have both the Review field and the Comments field filled.
Anyways, this makes it clear that the solution as it is right now cannot be used and we definitely have to write a migration if we want to rename this tab.

Proposed migration: Concatenate the Review to the Comment field, afterwards delete the Review field.
The Comment field should go from the General Tab to the Comments tab.

LinusDietz · 2018-02-14T11:20:12Z

So as discussed this PR does the following:

Rename the Review Tab into Comments Tab
Remove the Comment Field from the General Tab
Migrate the Review Field into Comment according to Rename the Review Tab into Comments Tab #3658 (comment)

tobiasdiez · 2018-02-14T12:36:28Z

src/test/java/org/jabref/logic/importer/util/RenameReviewToCommentTest.java

+
+        action.performAction(actualParserResult);
+
+        assertEquals(new ParserResult(Collections.singletonList(expectedEntry)).getDatabase().getEntryByKey("Entry1"),


why not just assertEquals(expectedEntry, actualParserResult.getDatabase().getEntryByKey("Entry1"))?

Same remark applies also to some other tests here.

good remark! (:

lenhard

Simplifying the entry editor by removing tabs is very much appreciated from my point of view. I have one comment regarding class placement and then this is good to go imho.

I suspect, though, that some users will howl because of this. I also have the feeling that @koppor has an opinion ;-)

lenhard · 2018-02-14T13:02:13Z

src/main/java/org/jabref/logic/importer/util/RenameReviewToComment.java

@@ -0,0 +1,39 @@
+package org.jabref.logic.importer.util;


Could you move the complete class into org.jabref.migrations? Because that's what it actually does, it migrates an opened bib file to a newer version of JabRef. I think it makes sense to bundle all this stuff in a dedicated package, because then it gets easier for us to see what sort of conversions are performed on open.

While you're doing that, could you also move the ConvertLegacyExplicitGroups?

Done. I also renamed these actions to what they are: migrations

…t-tab

Improve messages

LinusDietz · 2018-02-16T07:44:32Z

I think this PR is quite ready now

fixed issue 298

lenhard

I have some more usability nitpicking:

When I open a database with entries with review/comment fields a dialog tells me about the problem and I hit yes. Then I end up at the GUI, but nothing is written to the file system and the database is not marked as changed in JabRef. That's weird. Either the changes should make it to the file system directly or the database should be marked as changed.
The dialog informs on what happens if you click yes. But what are the implications when you say no? No migration and JabRef will keep nagging on next startup? Shouldn't this be an inform dialog where the user can only click ok? After all, the user doesn't have a choice, right?

Btw.: Sorry for merging the other PR into this. It was wrongly based on this PR and I didn't notice before merging...

…t-tab

…omment-tab

LinusDietz · 2018-02-16T11:09:48Z

@lenhard I have incorporated your suggestions. The solution I used for remembering the change in the database was to add a field in the ParserResult class and check that when instantiating the BasePanel.

lenhard

Tested locally and it works as expected, but there's still one thing in the code that you have to fix :-)

lenhard · 2018-02-16T12:49:29Z

src/main/java/org/jabref/logic/importer/ParserResult.java

@@ -19,6 +19,7 @@
 import org.jabref.model.metadata.MetaData;

 public class ParserResult {
+    public boolean wasChangedOnMigration = false;


Sorry, but there is no way I am going to sign off a public instance variable such as this. Please follow the Java Conventions and use a getter and a setter.

Yeah I though somebody might complain... I just find it so stupid... The `protection' is just the same with getters and setters...

I know a new book that's about to come out that argues for keeping to conventions when programming Java ;-)

The "protection" is not the same. Getters and setters encapsulate the variable and might perform (in future) addtional operations on them. That's what I liked about the automatic properties of C#. Automatically generated getters and setters.

tobiasdiez · 2018-02-16T13:58:46Z

src/main/java/org/jabref/migrations/MergeReviewIntoComment.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MergeReviewIntoComment implements PostOpenMigration {


I think most of this code should move to the logic package.

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.

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.

@tobiasdiez I guess you mean this line?

jabref/src/main/java/org/jabref/migrations/MergeReviewIntoComment.java

Line 38 in b246cfe

if (!conflicts.isEmpty() && new MergeReviewIntoCommentUIManager().askUserForMerge(conflicts)) {

I can create another Class for this confirmation, but where would you put that?

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:

You have a general migration manager class (in gui), which is getting called as soon as the user opens a database

This manager has a list of possible migrations which are defined in logic and which provide two methods: one for determining if an action is needed and a second one to actually perform the action

The manager first calls actionNeeded and maybe asks the user for confirmation, then runs performAction 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.

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)

That's fine with me!

The follow-up was done at #3733

LinusDietz · 2018-02-17T06:02:16Z

Thanks for all the feedback! I'll merge this now and start with the refactoring in the next PR.

koppor · 2023-09-06T06:38:08Z

@TarryDan Interestingly, your comment and the follow-up are not shown in my web browsers.

Thank you for your long comment and explanation. Especially the "faith" and trust of JabRef not altering data without asking. Back then, the group of developers of JabRef consistet of more progressive developers Now, the more thoughtful voices have more shares.

The workaround by @Siedlerchr DOES NOT WORK. We need to fix code @Siedlerchr

The workaround by @Siedlerchr is as follows: Close the library, revert all changes. Start JabRef. File -> Preferences -> Entry types. Select "Article". Add "Review" as field:

Mark it as optional (AKA non-required) and multiline

Click on "Save"

Repeat for all other types.

Restart JabRef

Then, the field appears again:

Save the library.

Close the library.

Open the library again.

JabRef displays no warning

"Review" moved to comment:

This is a BUG!

@TarryDan Thank you for raising this. We will discuss internally how to proceed with that and then create a public issue. -- We have "preference migration", where it is IMHO "more OK" to more around data. For libraries it is "more difficult" (to unacceptable in my opinion).

Rename the Review Tab into Comments Tab

289c2d2

LinusDietz added type: enhancement status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers entry-editor labels Jan 22, 2018

Remove Localization Key for Review

351b1db

tobiasdiez requested changes Jan 22, 2018

View reviewed changes

LinusDietz removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 22, 2018

LinusDietz changed the title ~~Rename the Review Tab into Comments Tab~~ [WIP] Rename the Review Tab into Comments Tab Jan 22, 2018

LinusDietz and others added 6 commits February 1, 2018 14:31

Merge branch 'master' into comment-tab

ac341e4

Use Comment field in the Comment tab

07a8965

Migration for the Review field

6a75c6a

Merge remote-tracking branch 'origin/comment-tab' into comment-tab

af9022a

Remove Comment Field from General Tab

afe5476

Tests

995b965

tobiasdiez reviewed Feb 14, 2018

View reviewed changes

Simplify tests

056ef9d

LinusDietz changed the title ~~[WIP] Rename the Review Tab into Comments Tab~~ Rename the Review Tab into Comments Tab Feb 14, 2018

LinusDietz added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 14, 2018

tobiasdiez approved these changes Feb 14, 2018

View reviewed changes

LinusDietz mentioned this pull request Feb 14, 2018

Rename review to comments JabRef/user-documentation#180

Merged

Fix Codacy

9d9c5a6

lenhard approved these changes Feb 14, 2018

View reviewed changes

Package refactoring, streamline naming

4a6886e

LinusDietz requested a review from koppor February 14, 2018 13:18

LinusDietz added 2 commits February 14, 2018 14:21

Also move tests

a5a2994

Merge branch 'master' of https://github.com/JabRef/jabref into commen…

37a247c

…t-tab

LinusDietz and others added 7 commits February 15, 2018 15:14

Fix tests

c93b490

Fix Codacy

e405f67

fixed issue 298

6cca4ab

updated Changes

b7ca150

Update CHANGELOG.md

38750e8

User decides now once for all migrations.

2d2bb7e

Improve messages

fix localization

dfeda8d

Merge pull request #3726 from grotepfn/patch-3

4828955

fixed issue 298

lenhard suggested changes Feb 16, 2018

View reviewed changes

LinusDietz added 4 commits February 16, 2018 10:03

Remove 'no' button

5962710

Merge branch 'master' of https://github.com/JabRef/jabref into commen…

4c1905d

…t-tab

Merge branch 'comment-tab' of https://github.com/JabRef/jabref into c…

38e6139

…omment-tab

Mark Database as changed after the LoadDatabaseMigration

b246cfe

lenhard suggested changes Feb 16, 2018

View reviewed changes

tobiasdiez reviewed Feb 16, 2018

View reviewed changes

Getters and Setters

303ed4d

LinusDietz merged commit 8108845 into master Feb 17, 2018

LinusDietz deleted the comment-tab branch February 17, 2018 06:02

LinusDietz mentioned this pull request Feb 17, 2018

Refactor BibDatabase Migrations #3733

Merged

This was referenced Feb 21, 2018

Strange output of "Review Field Migration" JabRef/jabref-koppor#310

Open

Review to Comment migration destroys newlines JabRef/jabref-koppor#311

Closed

Make better label at the "Review Field Migration" dialog JabRef/jabref-koppor#312

Closed

LinusDietz mentioned this pull request Feb 21, 2018

Meaningful button review field migration #3762

Merged

koppor mentioned this pull request Apr 16, 2018

Bibtex parser - Found unbracketed comment #3956

Closed

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 5, 2023

koppor mentioned this pull request Oct 7, 2023

Migrate library fields after confirmation. Store selected migration preference inside library #10370

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename the Review Tab into Comments Tab #3658

Rename the Review Tab into Comments Tab #3658

LinusDietz commented Jan 22, 2018 •

edited

Loading

tobiasdiez left a comment

LinusDietz commented Jan 22, 2018

tobiasdiez commented Jan 22, 2018

LinusDietz commented Jan 22, 2018

LinusDietz commented Feb 14, 2018

tobiasdiez Feb 14, 2018 •

edited

Loading

LinusDietz Feb 14, 2018

lenhard left a comment •

edited

Loading

lenhard Feb 14, 2018

LinusDietz Feb 14, 2018

LinusDietz commented Feb 16, 2018

lenhard left a comment •

edited

Loading

LinusDietz commented Feb 16, 2018

lenhard left a comment

lenhard Feb 16, 2018

LinusDietz Feb 16, 2018 •

edited

Loading

lenhard Feb 16, 2018 •

edited

Loading

Siedlerchr Feb 16, 2018

tobiasdiez Feb 16, 2018

lenhard Feb 16, 2018

tobiasdiez Feb 16, 2018

LinusDietz Feb 16, 2018

tobiasdiez Feb 16, 2018

LinusDietz Feb 16, 2018

tobiasdiez Feb 16, 2018

koppor Nov 8, 2023

LinusDietz commented Feb 17, 2018

koppor commented Sep 6, 2023


		action.performAction(actualParserResult);

		assertEquals(new ParserResult(Collections.singletonList(expectedEntry)).getDatabase().getEntryByKey("Entry1"),

Rename the Review Tab into Comments Tab #3658

Rename the Review Tab into Comments Tab #3658

Conversation

LinusDietz commented Jan 22, 2018 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

LinusDietz commented Jan 22, 2018

tobiasdiez commented Jan 22, 2018

LinusDietz commented Jan 22, 2018

LinusDietz commented Feb 14, 2018

tobiasdiez Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz commented Feb 16, 2018

lenhard left a comment • edited Loading

Choose a reason for hiding this comment

LinusDietz commented Feb 16, 2018

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

lenhard Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz commented Feb 17, 2018

koppor commented Sep 6, 2023

LinusDietz commented Jan 22, 2018 •

edited

Loading

tobiasdiez Feb 14, 2018 •

edited

Loading

lenhard left a comment •

edited

Loading

lenhard left a comment •

edited

Loading

LinusDietz Feb 16, 2018 •

edited

Loading

lenhard Feb 16, 2018 •

edited

Loading