-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add 'Open example library' button to Welcome Tab #13068
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
Conversation
taskExecutor).openFiles(List.of(tmpFile)); | ||
tmpFile.toFile().deleteOnExit(); | ||
} catch (IOException ex) { | ||
throw new RuntimeException(ex); |
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.
Using exceptions for control flow is not recommended. Exceptions should be used for exceptional states, not for normal control flow.
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.
Just add a logger statement here or show an errror dialog
dialogService.showEror...
} | ||
} | ||
} catch (IOException ex) { | ||
throw new RuntimeException(ex); |
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.
same 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.
tmp file is not the right approach.
if (in != null) { | ||
Path tmpFile = Files.createTempFile("example-library", ".bib"); | ||
try { | ||
Files.copy(in, tmpFile, StandardCopyOption.REPLACE_EXISTING); |
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.
Wrong approach.
- Create a new library
- Fill it with contents of
Chocolate.bib
. For that, you need to useorg.jabref.logic.importer.fileformat.BibtexParser#BibtexParser(org.jabref.logic.importer.ImportFormatPreferences)
. Then you can call org.jabref.model.database.BibDatabase#insertEntries(java.util.List<org.jabref.model.entry.BibEntry>) - similar toaddExampleButton.setOnAction(...
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.
Please try to keep the file in jablib
. Report back if you cannot load it.
- Passes each entry in "Chocolate.bib" - Creates an empty database object - Inserts the entries into the database - Wraps the database in BibDatabaseContext - Creates a new library tab with the data loaded from "Chocolate.bib"
@khushp3 please do not request a review before you have addressed the issues risen by the tests |
BibtexParser bibtexParser = new BibtexParser(preferences.getImportFormatPreferences(), fileUpdateMonitor); | ||
List<BibEntry> entries = bibtexParser.parseEntries(in); | ||
|
||
BibDatabase database = new BibDatabase(); | ||
database.insertEntries(entries); | ||
|
||
BibDatabaseContext databaseContext = new BibDatabaseContext(database); |
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.
Seeing this, there is a more simpler way:
BibtexParser bibtexParser = new BibtexParser(preferences.getImportFormatPreferences(), fileUpdateMonitor); | |
List<BibEntry> entries = bibtexParser.parseEntries(in); | |
BibDatabase database = new BibDatabase(); | |
database.insertEntries(entries); | |
BibDatabaseContext databaseContext = new BibDatabaseContext(database); | |
reader = Importer.getReader(inputStream); | |
BibtexParser bibtexParser = new BibtexParser(preferences.getImportFormatPreferences(), fileUpdateMonitor); | |
ParserResult result = bibtexParser.parse(reader); | |
BibDatabaseContext databaseContext = result.getDatabaseContext(); |
I found simlar code in org.jabref.logic.importer.fileformat.BibtexParser
fileHistoryMenu.getItems().addListener((ListChangeListener<MenuItem>) _ -> updateWelcomeRecentLibraries()); | ||
fileHistoryMenu.getItems().addListener((ListChangeListener<MenuItem>) e -> updateWelcomeRecentLibraries()); |
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.
Is this change AI generated? Please keep _
import org.jabref.model.entry.BibEntryTypesManager; | ||
import org.jabref.model.util.FileUpdateMonitor; | ||
|
||
import static org.jabref.gui.edit.automaticfiededitor.AbstractAutomaticFieldEditorTabViewModel.LOGGER; |
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.
Copy and paste issue - see https://devdocs.jabref.org/code-howtos/logging.html how to initialize the logger
Reader reader; | ||
reader = Importer.getReader(in); |
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.
Merge in one line; you can even move it up into the try
braces
@@ -124,7 +134,28 @@ private VBox createWelcomeStartBox() { | |||
stateManager, fileUpdateMonitor, entryTypesManager, undoManager, clipBoardManager, | |||
taskExecutor).execute()); | |||
|
|||
return createVBoxContainer(startLabel, newLibraryLink, openLibraryLink); | |||
Hyperlink openExampleLibraryLink = new Hyperlink(Localization.lang("Open example library")); |
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.
- The string should be
New example library
- Rename "New library" to "New empty library"
- position it one above, so that the order is
- New empty library
- New example library
- Open library
- Changed string names
…d JabRef_en.properties - Correctly imported LOGGER
@@ -62,7 +62,7 @@ public enum StandardActions implements Action { | |||
SKIMMED(Localization.lang("Set read status to skimmed"), IconTheme.JabRefIcons.READ_STATUS_SKIMMED, KeyBinding.SKIMMED), | |||
RELEVANCE(Localization.lang("Relevance"), IconTheme.JabRefIcons.RELEVANCE), | |||
RELEVANT(Localization.lang("Toggle relevance"), IconTheme.JabRefIcons.RELEVANCE), | |||
NEW_LIBRARY(Localization.lang("New library"), IconTheme.JabRefIcons.NEW), | |||
NEW_LIBRARY(Localization.lang("New empty library"), IconTheme.JabRefIcons.NEW), |
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.
The change from 'New library' to 'New empty library' should be consistent with other strings and grouped semantically. Ensure that similar actions use consistent terminology.
I fixed the small test issues (wrong variable order and translation strings) Now, it is good to go. Thank you for the efforts! |
@trag-bot didn't find any issues in the code! ✅✨ |
1 similar comment
@trag-bot didn't find any issues in the code! ✅✨ |
Closes #13014
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)