Refactor parsed file field #2723
Conversation
Move `getFiles` from `TypedBibEntry` to `BibEntry` (the later already contained a `setFiles` method)
…leHelper` Reuse `ParsedFileField.findIn` methods
tobiasdiez
added
the
status: ready-for-review
| * @param fileName | ||
| * @return The extension (without leading dot), trimmed and in lowercase. | ||
| */ | ||
| public static Optional<String> getFileExtension(String fileName) { |
There was a problem hiding this comment.
Can you add a JavaDoc comment why you implemented that method on your own instead of using Files.getFileExtension(fileName);? We have the Guava libraries and should use them 😇
There was a problem hiding this comment.
I thik he just moved the method, we previously had it in FileUtils
There was a problem hiding this comment.
We nevertheless could use Guava 😇
There was a problem hiding this comment.
Our code is slightly different (extensions are required to have at length > 1) and uses optional instead of empty strings. Thus I think it is not worth using Guava (maybe we can even remove the dependency on guava at some point).
| * @param values The String array. | ||
| * @return The encoded String. | ||
| */ | ||
| public static String encodeStringArray(String[][] values) { |
There was a problem hiding this comment.
Can you add a JavaDoc why this method is public? The other below is private, I miss the symmetry.
There was a problem hiding this comment.
It is public because it is used somewhere else. You are probably right and this method should be private, but I have no idea what "FileListTableModel" does....
| //Extract the path | ||
| Optional<String> oldValue = getField(FieldName.FILE); | ||
| if (!oldValue.isPresent()) { | ||
| return new ArrayList<>(); |
There was a problem hiding this comment.
Why not Collections.emptyList()?
|
|
||
| private static final ParsedFileField NULL_OBJECT = new ParsedFileField("", "", ""); | ||
| public class LinkedFile { |
There was a problem hiding this comment.
Could you add a JavaDoc stating the intension and usage of this class? Also, why no TypedBibEntry is required anymore?
| Optional<File> file = FileUtil.expandFilename(flEntry.getLink(), dirsS); | ||
| if ((!file.isPresent()) || !file.get().exists()) { | ||
| Optional<Path> file = field.findIn(panel.getBibDatabaseContext(), Globals.prefs.getFileDirectoryPreferences()); | ||
| if ((file.isPresent()) && Files.exists(file.get())) { |
There was a problem hiding this comment.
Why did you change the logic here?` Previously it was ! present
There was a problem hiding this comment.
Nice catch! Was probably a copy&paste error on my side. Fixed
| // Include the standard "file" directory: | ||
| List<String> fileDir = databaseContext.getFileDirectories(fileDirectoryPreferences); | ||
| // Include the directory of the BIB file: | ||
| List<String> al = new ArrayList<>(); |
There was a problem hiding this comment.
A better variable name would be nice
Inspired by a discussion with @lenhard in #2692. In detail, the following changes were performed:
FileFieldintoFileFieldWriterandFileFieldParsergetFilesfromTypedBibEntrytoBibEntry(the later already contained asetFilesmethod)isOnlineLinkandfindIntoParsedFileFieldlogic.util.io.FileUtiltomodel.util.FileHelper(idea for a better name?)ParsedFileFieldtoLinkedFilegradle localizationUpdate?