[WIP] File Magic Wizard Issue #380 #7172
Conversation
There was a problem hiding this comment.
Hi guys, I know, it's only a draft, but I took the opportunity to take a first look into your PR and made some quick remarks. It's already a great start, yet some little issues about codestyle and design patterns are open. I put also two questions in the comments. I didn't comment yet on the tests, because they seem still very much work in progress.
Hope I could help a little bit. Happy coding!
| * Here the wizard starts working. The method is activated when the "Start" button is pressed in the wizard interface. | ||
| */ | ||
| private void startWizard() { | ||
| directory = new File(viewModel.getDirectoryProperty().get()); |
| private void startWizard() { | ||
| directory = new File(viewModel.getDirectoryProperty().get()); | ||
|
|
||
| if(!directory.exists()) { |
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import javax.inject.Inject; | ||
| import java.awt.*; |
There was a problem hiding this comment.
Wildcard-imports are not allowed by checkstyle.
Neither is awt...
| import com.airhacks.afterburner.views.ViewLoader; | ||
| import javafx.event.ActionEvent; | ||
| import javafx.fxml.FXML; | ||
| import javafx.scene.control.*; |
There was a problem hiding this comment.
Please set up checkstyle configuration according to our workspace set up guide.
Wildcard imports are not allowed.
| FileInputStream fis = new FileInputStream(checkedFilesListFile); | ||
| ObjectInputStream ois = new ObjectInputStream(fis); |
There was a problem hiding this comment.
Please use meaningful variable names.
| public void updateProgress(BibEntry entry, int progress) { | ||
| double percent; | ||
| if(progress == 0) { | ||
| percent = 0; | ||
| } else { | ||
| percent = (double) progress / numberOfBibEntries; | ||
| progressBar.setProgress(percent); | ||
| } | ||
| Platform.runLater(() -> progressLabel.setText((int) (percent * 100) + "% complete, currently handling " + entry.getAuthorTitleYear(30))); | ||
| } |
There was a problem hiding this comment.
This should be part of the viewModel.
Please use a StringProperty or any other binding
| public void stopRunning() { | ||
| running = false; | ||
| Platform.runLater(() -> progressLabel.setText(Localization.lang("Cancelling, please wait..."))); | ||
| } | ||
|
|
||
| public boolean isRunning() { | ||
| return running; | ||
| } |
There was a problem hiding this comment.
This should be part of the ViewModel.
Use properties.
| int numberOfBibEntries; | ||
| boolean running; | ||
|
|
||
| public FileWizardProgressDialog(int numberOfBibEntries) { |
There was a problem hiding this comment.
Why don't you use our existing background task architecture?
| /** | ||
| * Serializes the list of already checked BibEntries, making the information survive past runtime. | ||
| */ | ||
| public void serializeCheckedFiles(List<String> checkedFilesList, File checkedFilesListFile) { |
| /** | ||
| * Deserializes the list of already checked BibEntry, minimizing redundant work for the Wizard. | ||
| */ | ||
| public List<String> deserializeCheckedFilesList(File checkedFilesListFile) { |
|
Thank you for submitting the PR. To support discussions on the feature, could you please also upload your nice UML diagram from the presentation? Then, the whole functionality gets more clear. |
|
I think, following diagrams are relevant:
|
|
I think the first two steps (in green) should be incorporated in the "Lookup > Search for unlinked local files" feature. There, instead of only suggesting importing the pdf as a new entry, JabRef should also suggest to link the file to a fitting existing entry (by adding a button in the file tree next to the file for which an entry might already exist). |
|
We had a long dicussion at in the devcall about the functionality of your PR. The UML diagrams really helped to understand what is going on. We decided the we follow the principle "separation of concerns". Thus, we keep local disk search separate from online identifier-based search. - The idea of the dialog (refined by Tobias) was put as separate issue at #7288. Nevertheless, thank you for your efforts and we hope, you learned much for your future software engineering endeavors. |
This is the first draft for File Magic Wizard (Issue #380).
The File Wizard should automatically synchronize local files with the corresponding BibEntries of a JabRef Library when executed.
koppor#380
This project is part of the course "software engineering" at Universität Basel.
@flurfis, @yasminmeier, @maxjappert