Add support for loading .blg warnings in Integrity Check Dialog#12866
Conversation
…d path resolution
| public DialogService getDialogService() { | ||
| return dialogService; | ||
| } |
There was a problem hiding this comment.
The new public method 'getDialogService' returns 'null' if 'dialogService' is not initialized. It should return an Optional to avoid potential NullPointerExceptions.
There was a problem hiding this comment.
Modified to pass DialogService from the constructor parameter
There was a problem hiding this comment.
Do DI by constructor. Getters for singletons are a no go.
| */ | ||
| private void loadBibLogSettingsPane() { | ||
| try { | ||
| FXMLLoader loader = new FXMLLoader(getClass().getResource("BibLogSettingsPane.fxml")); |
There was a problem hiding this comment.
@calixtus Don't we use the afterburner fx stuff here?
| /** | ||
| * | ||
| */ | ||
| public class BibWarning { |
There was a problem hiding this comment.
(quick initial comment)
Can this be a record? 😅
(and no empty JavaDoc please)
There was a problem hiding this comment.
Thanks! I've converted it to a record and removed the empty JavaDoc
| */ | ||
| private Optional<BibWarning> parseWarningLine(String line) { | ||
| // TODO: Support additional warning formats | ||
| Pattern compile = Pattern.compile("^Warning--([a-zA-Z ]+) in ([^\\s]+)$"); |
There was a problem hiding this comment.
Put this into private static final. - and use named groups 😅
There was a problem hiding this comment.
Updated as suggested 👍
|
|
||
| @FXML | ||
| private void onBrowse() { | ||
| FileChooser fileChooser = new FileChooser(); |
There was a problem hiding this comment.
You can use jabref's dialogService for this: dialogService.showFileOpenDialog(fileDialogConfiguration).ifPresent(path -> ...)
and with FileDialogConfiguration offers the Builder pattern.
(see e.g NewLibraryFromPdfAction)
There was a problem hiding this comment.
Thanks, already updated as suggested
| return blgFilePath; | ||
| } | ||
|
|
||
| public void setBlgFilePath(Path path) { |
There was a problem hiding this comment.
The method uses Objects.requireNonNull to check for null, which should be replaced with @nonnull annotation as per the guidelines.
|
|
||
| List<IntegrityMessage> messages = BibWarningToIntegrityMessageConverter.convert(warnings, context); | ||
|
|
||
| assertEquals(1, messages.size()); |
There was a problem hiding this comment.
Assertion statements should not include the message parameter. The method name should already convey the expected behavior.
| metaData.getBlgFilePaths().forEach((user, path) -> { | ||
| stringyMetaData.put(MetaData.BLG_FILE_PATH + "-" + user, Collections.singletonList(path.toString().trim())); | ||
| }); |
There was a problem hiding this comment.
The addition of .blg file paths serialization in org.jabref.logic.exporter.MetaDataSerializer requires corresponding test updates to ensure the new functionality is covered.
There was a problem hiding this comment.
already covered this change in the test method serializeUserSpecificBlgPath() ・ࡇ・
There was a problem hiding this comment.
What does this - do? Path, naming separator?
Maybe this could be a constant?
There was a problem hiding this comment.
I think, the other meta data fields do it, too.
I am OK without constant - follow-up can refactor the whole MetaDataParser ^^
| .put(MetaData.FILE_DIRECTORY_LATEX + '-' + user, Collections.singletonList(path.toString().trim()))); | ||
| metaData.getVersionDBStructure().ifPresent( | ||
| versionDBStructure -> stringyMetaData.put(MetaData.VERSION_DB_STRUCT, Collections.singletonList(versionDBStructure.trim()))); | ||
| metaData.getBlgFilePaths().forEach((user, path) -> { |
There was a problem hiding this comment.
The addition of serializing .blg file paths requires corresponding tests to be updated or added to ensure the new functionality is correctly implemented and verified.
|
|
||
| import javafx.fxml.FXML; | ||
| import javafx.scene.control.TextField; | ||
| import javafx.stage.FileChooser; |
There was a problem hiding this comment.
The import statement for FileChooser is unnecessary as the dialogService is used for file dialogs, which aligns with the project's architecture guidelines.
There was a problem hiding this comment.
I think, the comment of trag bot is correct.
|
Tried out now with a MWE generated by https://github.com/latextemplates/generator-latex-template. Works. |
Thanks @koppor I noticed some of the checks are failing, I’ll take a look and try pushing a fix over this week. Let me know if there’s anything else I should work on :) |
| * Ensures that the trailing semicolon (used as separator in .bib metadata) is handled and stripped properly. | ||
| */ | ||
| @Test | ||
| void parsesUserSpecificBlgPathSuccessfully() throws Exception { |
There was a problem hiding this comment.
The method name 'parsesUserSpecificBlgPathSuccessfully' is not comprehensive enough. It should describe the test case more clearly without needing a @DisplayName annotation.
| private Runnable onBlgPathChanged; | ||
|
|
||
| public void initializeViewModel(BibDatabaseContext context, Runnable onBlgPathChanged) throws JabRefException { | ||
| if (context == null || context.getMetaData() == null) { |
There was a problem hiding this comment.
is it necessary to check for nulls here?
|
|
||
| public Path getInitialDirectory() { | ||
| return bibPath.flatMap(path -> Optional.ofNullable(path.getParent())) | ||
| .orElse(Path.of(System.getProperty("user.home"))); |
There was a problem hiding this comment.
Is this cross-platform ("user.home")
There was a problem hiding this comment.
Not on Windows XP, but this is acceptable: https://stackoverflow.com/q/585534/873282
|
I only meant to leave a single comment here, but I think I accidentally triggered a full review by clicking the wrong button 😅 Also, I noticed that the Trag review has been marked as “in progress” for two days now, not sure if that’s expected or if something got stuck? |
It sometimes does that - pushing another commit refreshes it. |
koppor
left a comment
There was a problem hiding this comment.
some small comments towards a really JabRef-high-quality code 😅
| /** | ||
| * 1. Connects MetaData with the view. | ||
| * 2. Wraps .blg warnings as IntegrityMessages. | ||
| * 3. Supports file browsing and reset actions. | ||
| */ |
There was a problem hiding this comment.
Either html or Markdown - I propose markdown
| /** | |
| * 1. Connects MetaData with the view. | |
| * 2. Wraps .blg warnings as IntegrityMessages. | |
| * 3. Supports file browsing and reset actions. | |
| */ | |
| /// 1. Connects MetaData with the view. | |
| /// 2. Wraps .blg warnings as IntegrityMessages. | |
| /// 3. Supports file browsing and reset actions. |
| this.path.set(resolvedPath.toString()); | ||
| if (metaData.getBlgFilePath(user).isEmpty()) { | ||
| metaData.setBlgFilePath(user, resolvedPath); | ||
| this.lastResolvedBlgPath = Optional.of(resolvedPath); | ||
| } |
| /** | ||
| * Resolves custom or default .blg path for this library. | ||
| * | ||
| * Priority: | ||
| * 1. User-defined path from MetaData | ||
| * 2. Default: same name as .bib file with .blg extension | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Resolves custom or default .blg path for this library. | |
| * | |
| * Priority: | |
| * 1. User-defined path from MetaData | |
| * 2. Default: same name as .bib file with .blg extension | |
| */ | |
| /// Resolves custom or default .blg path for this library. | |
| /// | |
| /// Priority: | |
| /// | |
| /// 1. User-defined path from MetaData | |
| /// 2. Default: same name as .bib file with .blg extension |
| /** | ||
| * Converts {@link BibWarning}s into {@link IntegrityMessage}s for integration with the existing integrity check UI. | ||
| * | ||
| * Notes: | ||
| * - The current IntegrityMessage interface expects a {@link BibEntry} and a {@link Field}, | ||
| * but .blg warnings come from a different source and may not include a field. | ||
| * - For now, we map missing fields to a placeholder (InternalField.KEY_FIELD) to make it compatible with the UI. | ||
| * - This is a minimal MVP solution to reuse the Integrity tab without changing the UI structure. | ||
| * | ||
| * Future direction: | ||
| * - Consider defining a proper interface (e.g., IntegrityMessageWithField / WithoutField) | ||
| * to support warnings without fields cleanly. | ||
| */ |
There was a problem hiding this comment.
Please also convert to Markdown (to have the notes list rendered properly)
| metaData.getBlgFilePaths().forEach((user, path) -> { | ||
| stringyMetaData.put(MetaData.BLG_FILE_PATH + "-" + user, Collections.singletonList(path.toString().trim())); | ||
| }); |
There was a problem hiding this comment.
I think, the other meta data fields do it, too.
I am OK without constant - follow-up can refactor the whole MetaDataParser ^^
| assertEquals(2, messages.size()); | ||
|
|
||
| IntegrityMessage msg1 = messages.getFirst(); | ||
| assertEquals("empty journal", msg1.message()); | ||
| assertEquals(firstEntry, msg1.entry()); | ||
| assertEquals("journal", msg1.field().getName()); | ||
|
|
||
| IntegrityMessage msg2 = messages.get(1); | ||
| assertEquals("empty year", msg2.message()); | ||
| assertEquals(secondEntry, msg2.entry()); | ||
| assertEquals("year", msg2.field().getName()); |
There was a problem hiding this comment.
Is it possible to compare two lists directly? Or are IntegrityMessage objects not good with equals?
| BibLogPathResolver.resolve(metaData, bibPath, user) | ||
| .ifPresent(resolvedPath -> { | ||
| this.path.set(resolvedPath.toString()); | ||
| if (metaData.getBlgFilePath(user).isEmpty()) { | ||
| metaData.setBlgFilePath(user, resolvedPath); | ||
| this.lastResolvedBlgPath = Optional.of(resolvedPath); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The code does not follow the fail-fast principle. It should return early if the condition is not met, instead of nesting logic inside an if statement.
| public void resetBlgFilePath() { | ||
| metaData.clearBlgFilePath(user); | ||
| userManuallySelectedBlgFile = false; | ||
| Optional<Path> resolved = BibLogPathResolver.resolve(metaData, bibPath, user); | ||
| if (resolved.isEmpty()) { | ||
| path.set(""); | ||
| lastResolvedBlgPath = Optional.empty(); | ||
| return; | ||
| } | ||
|
|
||
| Path resolvedPath = resolved.get(); | ||
| path.set(resolvedPath.toString()); | ||
| lastResolvedBlgPath = Optional.of(resolvedPath); | ||
| } |
There was a problem hiding this comment.
The method does not follow the fail-fast principle. It should return early if the condition is not met, instead of nesting logic inside an if statement.
| if (context.getDatabase().getEntryByCitationKey(bibWarning.entryKey()).isEmpty()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The code should follow the fail fast principle by immediately handling invalid states and returning early instead of nesting logic inside else branches. This code should return early if the entry is not found.
| new IntegrityMessage("empty year", secondEntry, FieldFactory.parseField("year")) | ||
| ); | ||
|
|
||
| assertEquals(expectedMessages, actualMessages); |
There was a problem hiding this comment.
The test uses assertEquals to compare lists of objects. It's better to assert the contents of objects directly to ensure more precise validation of the test results.
|
@wanling0000 One last thing: submodules |
|
@trag-bot didn't find any issues in the code! ✅✨ |
Fixed, thanks for the reminder! |
|
@wanling0000 good work! |
Closes #11998
This PR adds support for: Check integrity: Support for BibTeX .blg files
Users can now:
.blgfile from the same folder as the.bibfile;.blgfile via Browse and load BibTeX warnings;.blgpath;.blgwarnings when switching files (i.e., only show current.blgfile's messages);.blgfile path inMetaData(saved along with.bibfile).Change Overview
BibWarning.blgwarningMetaData.get/set/clearBlgFilePath().blgfile pathBibtexLogParser.blglines intoBibWarningobjectsBibWarningToIntegrityMessageConverterIntegrityMessageBibLogPathResolver.blgfile path: user-defined or fallbackMetaDataParserBLG_FILE_PATHin.bibmetadataMetaDataSerializerBibLogSettingsViewModelBibLogSettingsPane.fxmlBibLogSettingsPane.javaIntegrityCheckDialog.java.blgwarnings and refresh table displayIntegrityCheckAction.java.blgwarnings during initial dialog showScreenshots
❓Limitations of .blg Parsing – I would appreciate feedback
Right now, the .blg parser in this PR (BibtexLogParser) only supports a very simple warning format:
But I found that in real .blg files, there are other kinds of warnings that don’t follow this format.
For example, in this GitHub issue: James-Yu/LaTeX-Workshop#2324
This kind of warning does not point to a specific entry or field, so currently it is ignored by the parser.
I’m wondering:
Would it make sense to introduce an interface like IntegrityMessage, or is it better to wait and see more .blg formats before making this change?
At the moment I don’t have many .blg test files, so I’m not sure how common these “fieldless” warnings are.
If you have any suggestions on how to investigate this, I’d be happy to look into it!
Any feedback or thoughts on this would be really helpful, thank you!
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)