Migrate Review field in entry preview to comment#4100
Conversation
Siedlerchr
changed the title
Siedlerchr
added
the
status: ready-for-review
koppor
left a comment
koppor
left a comment
There was a problem hiding this comment.
LGTM. Can we have a test case for the migration?
|
Added a test case |
tobiasdiez
left a comment
tobiasdiez
left a comment
There was a problem hiding this comment.
LGTM and I've only a few minor remarks.
|
|
||
| // Preview | ||
| //public because needed for pref migration | ||
| public static final String PREVIEW_STYLE = "previewStyle"; |
There was a problem hiding this comment.
You can simply add a get/setPreviewStyle method which I would prefer.
| } | ||
|
|
||
| static void upgradePreviewStyleFromReviewToComment() { | ||
| JabRefPreferences prefs = Globals.prefs; |
There was a problem hiding this comment.
You can pass the preferences class as a parameter and then use mock in combination with verify in the tests below (instead of working on the actual preference object).
There was a problem hiding this comment.
I tried around with this but I currently see no way to do this. When I mock the object, calling the migration method on the object will have no effect as it operates directly on the preferences object. I don't like that operating on the real prefs object but
Sure I can mock returns, but that's not the goal of this test.
However, I could remove the dependency on the actual Globals.prefs object by passing a Preferences object as parametre
| "[bibtexkey] - [fulltitle]"}; | ||
| "[bibtexkey] - [fulltitle]"}; | ||
|
|
||
| private final String oldPreviewStyle = "<font face=\"sans-serif\">" |
There was a problem hiding this comment.
Can you please define these as local variables in the test below and try to simplify it a bit by removing unimportant lines.
|
I would vote for merging this, I refactored the tests and the migrations so that they no longer call Globals.prefs. _ More mocking is not really possible. |
| + "</dd>__NEWLINE__<p></p></font>"; | ||
|
|
||
| String previousStyle = prefs.getPreviewStyle(); | ||
| prefs.setPreviewStyle(oldPreviewStyle); |
There was a problem hiding this comment.
I don't see a reason why you shouldn't be able to mock the preferences:
preferences = mock(JabRefPreferences);
when(preferences.getPreviewStyle).thenReturn(oldPreviewStyle);;
verify(preferences).setPreviewStyle(newPreviewStyle);There was a problem hiding this comment.
Got it now, I didn't know that verify is interpreted as "assert" and makes the test fail/succeed
* upstream/master: (22 commits) fix failing architecture test by making test class public again migrate architecture test to junit jupiter Fix build... (#4128) fix checkstyle after merge Migrate Review field in entry preview to comment (#4100) [WIP] Split push to applications in logic and gui (#4110) Fix checkstyle Fix #4115: Don't report journal name as abbreviated when full name = abbreviated name (#4116) Use <kbd>in changelog Groups right click (#4061) Fix open thread prevents shutdown (#4111) Extract v4.x changelog (#4125) Saves and reloads window state on close and on open (#4124) Fix convert to bibtex moves contents of the file field (#4123) Opens the directory of the currently edited file when opening a new file (#4106) Don't run on Swing thread Properly shutdown telemetry client Code cleanup Remove Swing stuff (L&F, font customization) Properly shutdown JabRef (not with System.exit) Replace swing clipboard with JavaFX one ... # Conflicts: # src/main/java/org/jabref/gui/BasePanel.java # src/main/java/org/jabref/gui/JabRefFrame.java # src/main/java/org/jabref/gui/maintable/MainTable.java # src/main/java/org/jabref/gui/search/SearchResultFrame.java # src/main/java/org/jabref/preferences/JabRefPreferences.java
3 participants
Migrate Review field in entry preview to comment
Replace review with comment in every layout file
Fixes #4098