Code cleanups#2141
Conversation
| // try to avoid ending up with the ugly Metal L&F | ||
| Plastic3DLookAndFeel lnf = new Plastic3DLookAndFeel(); | ||
| Plastic3DLookAndFeel.setCurrentTheme(new SkyBluer()); | ||
| MetalLookAndFeel.setCurrentTheme(new SkyBluer()); |
There was a problem hiding this comment.
Why did you change that to Metal as we want to avoid it here?
There was a problem hiding this comment.
Apparently Plastic is inheriting the method from Metal.
There was a problem hiding this comment.
I can change it back though. Was also a bit surprised.
|
|
||
| for (BibEntry entry : bp.getDatabase().getEntries()) { | ||
| // Please explain what this line, and therefore the whole method, is doing (if it is doing something) | ||
| EntryTypes.getType(entry.getType(), bibDatabaseMode).ifPresent(entry::setType); |
There was a problem hiding this comment.
I think, it is because the types are objects. If one switches from bibtex to biblatex, the types get different objects even though the string representation is the same. This loop ensures that the type objects match the type of the database.
There was a problem hiding this comment.
OK! I'll remove the comment.
|
I just provided the comment on the unclear method - I think, @stefan-kolb can approve that. The static method for Other than that: LGTM |
tobiasdiez
left a comment
There was a problem hiding this comment.
LGTM, just a small remark.
Also please remove the other comment before merging.
| */ | ||
| public void removeChild() { | ||
| // NPE if this is ever called | ||
| child.setParent(null); |
There was a problem hiding this comment.
Could you please fix it by wrapping everything in a if( child != null)
There was a problem hiding this comment.
Isn't it better that the author of the faulty code fixes it when the implementation is finished? I do not really see how this code was ever approved...
There was a problem hiding this comment.
Oh, yes. The problem is actually passing null to a metod with an Objects.requireNonNull as the first call... So clearly that code was never executed.
There was a problem hiding this comment.
Isn't it better that the author of the faulty code fixes it
The nice thing about open source code is that there is no "your / mine code", everyone is responsible for all the code. I can't count how many times I already fixed some small bugs which somebody else created and similarly many others (including you) cleaned up my mess.
Please just add the if statement there and then merge this in directly (ready-for-review removed for this reason).
There was a problem hiding this comment.
The first comment was not really relevant here. Again, the thing is not that child becomes null, although that will also break the code, but that whenever child.setParent(null) is called there will be an NPE as the code for setParentreads (second comment):
protected void setParent(T parent) {
this.parent = Objects.requireNonNull(parent);
}
Hence, yes, I can add the if statement, but that is not really solving the problem I pointed out in the comment.
There was a problem hiding this comment.
And, no, I have no idea how the code is supposed to actually work, but I can notice that the removeChild was never called as a call to it would throw an NPE in setParent. (And I have also fixed other persons bugs. This one I cannot really see why it was ever merged to begin with though. I would exepect even half finished merged code to not throw NPEs on every possible call to it.)
|
As I added the if-statement (kept the comment though, as described above), I merge this now. |
* upstream/master: Code cleanups (#2141) Remove obsolete string in .mailmap Update .mailmap and AUTHORS Implementation of autosave & backup feature (#2118) Make all files selectable in file chooser dialogs (#2094) Fix group drag and drop (#2161) Change donation badge and point to https://donations.jabref.org # Conflicts: # src/main/java/net/sf/jabref/logic/util/OptionalUtil.java
Removed some unused arguments.
Used constants from top level definition.
I also added some comments to consider during the review.