-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix some spotbugs issues #3060
Fix some spotbugs issues #3060
Conversation
@@ -58,7 +58,7 @@ private Globals() { | |||
} | |||
|
|||
// Key binding preferences | |||
public static KeyBindingRepository getKeyPrefs() { | |||
public static synchronized KeyBindingRepository getKeyPrefs() { |
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.
Why synchronized here?
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.
Incorrect lazy initialization
https://stackoverflow.com/a/6782690/3450689
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.
Refs item 3 of effective java, doesn't it?
@@ -2144,7 +2144,9 @@ public void action() { | |||
if ((focused != null) && (focused instanceof FieldEditor) && focused.hasFocus()) { | |||
// User is currently editing a field: | |||
// Check if it is the preamble: | |||
if ((preambleEditor != null) && (focused == preambleEditor.getFieldEditor())) { | |||
|
|||
FieldEditor f = (FieldEditor) focused; |
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.
Bad variable name
@@ -150,8 +150,8 @@ public void openFiles(List<Path> filesToOpen, boolean raisePanel) { | |||
Path file = iterator.next(); | |||
for (int i = 0; i < frame.getTabbedPane().getTabCount(); i++) { | |||
BasePanel basePanel = frame.getBasePanelAt(i); | |||
if ((basePanel.getBibDatabaseContext().getDatabaseFile().isPresent()) | |||
&& basePanel.getBibDatabaseContext().getDatabaseFile().get().equals(file)) { | |||
if ((basePanel.getBibDatabaseContext().getDatabasePath().isPresent()) |
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.
Path equals file?
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.
This was the bug I fixed
@@ -163,16 +163,22 @@ public String toString() { | |||
} | |||
|
|||
@Override | |||
public boolean equals(Object other) { | |||
if (this == other) { | |||
public boolean equals(Object o) { |
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.
other is a pretty good name for the parameter imho
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 renamed it to be consistent with the other implementations we have.
And there other is used as the name of the casted instance, see the line below
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.
Set a better name for the FieldEditor
as metioned by Stefan and then this is ready to go.
* upstream/master: Add warning message if group with same name is already present (#3077) Fix #3062: Ctrl + F works again Fix del/copy/paste key trigger main table action in search bar (#3070) Fix markdown Update gradle from 4.0.1 to 4.0.2 Fix #3045 Update Transformer plugin Reimplement MappedList using a backigList (#3069) Fix importing preferences after resetting without restarting (#3065) Fix some spotbugs issues (#3060) Import dialog when fetch (#3025) Adapt CircleCI build Adapt CI script Closes #3027 and updates install4j to v7
Some few issues, mostly some issues with equal:
https://spotbugs.github.io/
Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run
gradle localizationUpdate
?