Skip to content
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

Implements #1664: group based on aux file #3444

Merged
merged 9 commits into from
Jan 28, 2018
Merged

Implements #1664: group based on aux file #3444

merged 9 commits into from
Jan 28, 2018

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Nov 20, 2017

This PR implements #1664 by adding a group that contains all entries referenced in a given aux-file. So far the code is only a prototype, but it already works really well. What is missing is that changes in the aux file are automatically recognized and handled (this should be easy to accomplish). The user interface is very very basic and only contains a text field where the user can specify the path to the aux file (in light of the upcoming rewrite of the groups dialog in javafx #2643, I don't want to spent to much time at ui stuff).

I have two questions:

  • The implementation so far is against our architecture. The new group (TexGroup) is in the model but uses the AuxParser from the logic. Any ideas how to resolve this issue? More and more, I don't like the strict rule that model is not allowed to access logic - how I'm supposed to create "intelligent" objects in the model if they are not allowed to use advanced stuff from the logic package?

  • The file path right now is stored as an absolute path. Is this ok? If not, what should be the "base" of a relative path? The bib file?


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@Siedlerchr
Copy link
Member

Regarding the File Directory stuff: We have a method: getFirstExistingFileDir which gives you the file directoy (either main file idr, bib file or library setting=

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Nov 21, 2017

@Siedlerchr thanks! Do you think it makes sense to apply the same rules for the aux files as for pdf files when it comes to relative paths?

public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
if (!super.equals(o)) return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This equals method looks weird. Why not use instanceof?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I don't want subgroups to be considered equal to this class?
(The equals method is, by the way, mostly auto-generated by intellj

@lenhard
Copy link
Member

lenhard commented Nov 22, 2017

The architecture is not set in stone, we can always decide to change it. Having said that, I still think that the current separation of model and logic makes sense and I think that the tests are useful. Through the tests, we always get a notification about changes that easily turn into circular dependencies and we have an opportunity to discuss them. If we don't have that, then the code will just silently turn into a big ball of mud with circular dependencies everywhere.

Looking at the code, I don't really see an architectural problem. Right now, you are computing the Set<String> keysUsedInAux on demand in a call to contains(). You can just do that when you create the group and pass the set into the constructor, dependency problem solved.

When you want to recognize changes in the aux file, you will need to monitor the file (which is functionality that is located in the higher layers anyway). Then, you could add a setter to the group that resets the keys when a file change happens.

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Jan 15, 2018

@lenhard I now used your approach: the TeX group class only relies on interfaces (defined in model), while the concrete implementation of these interfaces resides in logic.

This PR is now ready-for-review. We should really consider using a dependency injection framework...most of the line changes are just propagating the file monitor from Globals to the TeX group.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 15, 2018
@tobiasdiez tobiasdiez changed the title [WIP] Implements #1664: group based on aux file Implements #1664: group based on aux file Jan 15, 2018
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobiasdiez Yes, applying a dependency injection framework might help to simplify the code. But even if we apply one, we're still going to need interfaces and we'll have to stay clear of circular dependencies. If anything, this problem is going to get bigger with our move to Java 9. With jigsaw, Java will no longer compile the code if there are circular dependencies between modules (if I recall everything correctly).

Apart from that, you're still having some dependency issues here that need fixing ;-)

* This {@link FileUpdateMonitor} does nothing.
* Normally, you want to use {@link DefaultFileUpdateMonitor} except if you don't care about updates.
*/
public class DummyFileUpdateMonitor implements FileUpdateMonitor {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the FileUpdateMonitor interface is in model, why don't you just put this class into the exact same package? It would solve the dependency problems that are still breaking the build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good observation! I moved the file and so fixed the build.

Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this locally now and it works for me.

This is good to be merged in my point of view.

@tobiasdiez tobiasdiez merged commit cfb424f into master Jan 28, 2018
@tobiasdiez tobiasdiez deleted the auxGroup branch January 28, 2018 18:37
@tobiasdiez
Copy link
Member Author

In order to somewhat keep the momentum up and reduce the number of open PRs, I'll merge this now.

Siedlerchr added a commit that referenced this pull request Feb 6, 2018
* upstream/master: (155 commits)
  Update DEVELOPERS
  Update README.md
  [WIP] File link deletion dialog improvements (#3690)
  Update guava (#3692)
  Fix some FX  Thread issue (#3691)
  Fixed typo, mayor to major (#3685)
  add Office xml test for latex free fields (#3680)
  Migrate importer tests to JUnit5 (#3665)
  fix typo
  Update slf4j-api from 1.8.0-beta0 -> 1.8.0-beta1
  Update richtext from 0.8.1 -> 0.8.2
  Update junit from 5.1.0-M1 -> 5.1.0-M2
  Update checkstyle from 8.7 -> 8.8
  Export all fields with their latex free equivalent (#3675)
  try around with exckluding tag (#3676)
  Fixes #3648: Chained modifiers work again (#3670)
  Update gradle from 4.4.1 to 4.5 and tweak .gitattributes
  Fix 2633 saving creates new database (#3674)
  New translations JabRef_en.properties (French)
  Implements #1664: group based on aux file (#3444)
  ...

# Conflicts:
#	CHANGELOG.md
#	CONTRIBUTING.md
#	build.gradle
#	gradle/wrapper/gradle-wrapper.jar
#	gradle/wrapper/gradle-wrapper.properties
#	scripts/syncLang.py
#	src/main/java/org/jabref/FallbackExceptionHandler.java
#	src/main/java/org/jabref/Globals.java
#	src/main/java/org/jabref/JabRefMain.java
#	src/main/java/org/jabref/cli/ArgumentProcessor.java
#	src/main/java/org/jabref/cli/JabRefCLI.java
#	src/main/java/org/jabref/collab/EntryChange.java
#	src/main/java/org/jabref/collab/EntryDeleteChange.java
#	src/main/java/org/jabref/collab/FileUpdateListener.java
#	src/main/java/org/jabref/collab/StringAddChange.java
#	src/main/java/org/jabref/collab/StringChange.java
#	src/main/java/org/jabref/collab/StringNameChange.java
#	src/main/java/org/jabref/collab/StringRemoveChange.java
#	src/main/java/org/jabref/gui/BasePanel.java
#	src/main/java/org/jabref/gui/ClipBoardManager.java
#	src/main/java/org/jabref/gui/DefaultInjector.java
#	src/main/java/org/jabref/gui/DialogService.java
#	src/main/java/org/jabref/gui/EntryTypeDialog.java
#	src/main/java/org/jabref/gui/JabRefFrame.java
#	src/main/java/org/jabref/gui/actions/IntegrityCheckAction.java
#	src/main/java/org/jabref/gui/collab/ChangeScanner.java
#	src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java
#	src/main/java/org/jabref/gui/copyfiles/CopyFilesTask.java
#	src/main/java/org/jabref/gui/customjfx/CustomJFXPanel.java
#	src/main/java/org/jabref/gui/desktop/JabRefDesktop.java
#	src/main/java/org/jabref/gui/entryeditor/DeprecatedFieldsTab.java
#	src/main/java/org/jabref/gui/entryeditor/EntryEditor.css
#	src/main/java/org/jabref/gui/entryeditor/EntryEditor.java
#	src/main/java/org/jabref/gui/entryeditor/FieldsEditorTab.java
#	src/main/java/org/jabref/gui/entryeditor/OptionalFields2Tab.java
#	src/main/java/org/jabref/gui/entryeditor/OptionalFieldsTab.java
#	src/main/java/org/jabref/gui/entryeditor/OtherFieldsTab.java
#	src/main/java/org/jabref/gui/entryeditor/RequiredFieldsTab.java
#	src/main/java/org/jabref/gui/entryeditor/SourceTab.java
#	src/main/java/org/jabref/gui/entryeditor/UserDefinedFieldsTab.java
#	src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationTabController.java
#	src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationTabViewModel.java
#	src/main/java/org/jabref/gui/exporter/ExportAction.java
#	src/main/java/org/jabref/gui/exporter/ExportFileFilter.java
#	src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java
#	src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java
#	src/main/java/org/jabref/gui/fieldeditors/EditorValidator.java
#	src/main/java/org/jabref/gui/fieldeditors/FieldEditors.java
#	src/main/java/org/jabref/gui/fieldeditors/FileListEditorTransferHandler.java
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java
#	src/main/java/org/jabref/gui/fieldeditors/MapBasedEditorViewModel.java
#	src/main/java/org/jabref/gui/groups/GroupDialog.java
#	src/main/java/org/jabref/gui/groups/GroupSidePane.java
#	src/main/java/org/jabref/gui/groups/GroupTreeController.java
#	src/main/java/org/jabref/gui/help/AboutDialogViewModel.java
#	src/main/java/org/jabref/gui/help/HelpAction.java
#	src/main/java/org/jabref/gui/importer/ImportFileFilter.java
#	src/main/java/org/jabref/gui/importer/ImportFormats.java
#	src/main/java/org/jabref/gui/maintable/MainTableSelectionListener.java
#	src/main/java/org/jabref/gui/preftabs/AppearancePrefsTab.java
#	src/main/java/org/jabref/gui/preftabs/EntryEditorPrefsTab.java
#	src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java
#	src/main/java/org/jabref/gui/search/GlobalSearchBar.java
#	src/main/java/org/jabref/gui/util/BindingsHelper.java
#	src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java
#	src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java
#	src/main/java/org/jabref/gui/util/FileUpdateListener.java
#	src/main/java/org/jabref/gui/worker/CitationStyleToClipboardWorker.java
#	src/main/java/org/jabref/logic/bibtexkeypattern/BibtexKeyGenerator.java
#	src/main/java/org/jabref/logic/bibtexkeypattern/BracketedPattern.java
#	src/main/java/org/jabref/logic/citationstyle/CitationStyle.java
#	src/main/java/org/jabref/logic/citationstyle/CitationStyleCache.java
#	src/main/java/org/jabref/logic/citationstyle/CitationStyleGenerator.java
#	src/main/java/org/jabref/logic/exporter/BibTeXMLExporter.java
#	src/main/java/org/jabref/logic/exporter/ExportFormats.java
#	src/main/java/org/jabref/logic/exporter/IExportFormat.java
#	src/main/java/org/jabref/logic/exporter/MSBibExporter.java
#	src/main/java/org/jabref/logic/exporter/ModsExporter.java
#	src/main/java/org/jabref/logic/exporter/OpenDocumentSpreadsheetCreator.java
#	src/main/java/org/jabref/logic/exporter/OpenOfficeDocumentCreator.java
#	src/main/java/org/jabref/logic/exporter/TemplateExporter.java
#	src/main/java/org/jabref/logic/importer/ImportFormatReader.java
#	src/main/java/org/jabref/logic/importer/WebFetchers.java
#	src/main/java/org/jabref/logic/importer/fetcher/IacrEprintFetcher.java
#	src/main/java/org/jabref/logic/integrity/FieldCheckers.java
#	src/main/java/org/jabref/logic/integrity/IntegrityCheck.java
#	src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java
#	src/main/java/org/jabref/logic/shared/DBMSProcessor.java
#	src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java
#	src/main/java/org/jabref/logic/util/FileType.java
#	src/main/java/org/jabref/logic/util/JavaVersion.java
#	src/main/java/org/jabref/logic/util/io/CiteKeyBasedFileFinder.java
#	src/main/java/org/jabref/logic/util/io/FileFinder.java
#	src/main/java/org/jabref/logic/util/io/FileUtil.java
#	src/main/java/org/jabref/logic/util/io/RegExpBasedFileFinder.java
#	src/main/java/org/jabref/model/auxparser/AuxParserResult.java
#	src/main/java/org/jabref/model/database/BibDatabaseContext.java
#	src/main/java/org/jabref/model/entry/FieldProperty.java
#	src/main/java/org/jabref/model/pdf/FileAnnotation.java
#	src/main/java/org/jabref/model/util/FileUpdateListener.java
#	src/main/java/org/jabref/preferences/CustomExportList.java
#	src/main/java/org/jabref/preferences/JabRefPreferences.java
#	src/test/java/org/jabref/TestArchitectureTests.java
#	src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java
#	src/test/java/org/jabref/gui/importer/EntryFromFileCreatorManagerTest.java
#	src/test/java/org/jabref/logic/bibtexkeypattern/BibtexKeyGeneratorTest.java
#	src/test/java/org/jabref/logic/bibtexkeypattern/MakeLabelWithDatabaseTest.java
#	src/test/java/org/jabref/logic/exporter/MsBibExportFormatTest.java
#	src/test/java/org/jabref/logic/importer/ImportFormatReaderTestParameterless.java
#	src/test/java/org/jabref/logic/importer/fetcher/ArXivTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/AstrophysicsDataSystemTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/CrossRefTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/DBLPFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/DiVATest.java
#	src/test/java/org/jabref/logic/importer/fetcher/DoiFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/FulltextFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/GvkFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/IacrEprintFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/IsbnFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/IsbnViaChimboriFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/IsbnViaEbookDeFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/MedlineFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/MrDLibFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/SpringerLinkTest.java
#	src/test/java/org/jabref/logic/importer/fetcher/TitleFetcherTest.java
#	src/test/java/org/jabref/logic/importer/fileformat/BibtexParserTest.java
#	src/test/java/org/jabref/logic/importer/fileformat/FreeCiteImporterTest.java
#	src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java
#	src/test/java/org/jabref/logic/shared/DBMSConnectionTest.java
#	src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java
#	src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java
#	src/test/java/org/jabref/logic/shared/DBMSTypeTest.java
#	src/test/java/org/jabref/logic/shared/SynchronizationTestEventListener.java
#	src/test/java/org/jabref/logic/shared/SynchronizationTestSimulator.java
#	src/test/java/org/jabref/logic/shared/TestConnector.java
#	src/test/java/org/jabref/logic/util/BracketedPatternTest.java
#	src/test/java/org/jabref/logic/util/io/CiteKeyBasedFileFinderTest.java
#	src/test/java/org/jabref/logic/util/io/RegExpBasedFileFinderTests.java
#	src/test/java/org/jabref/testutils/category/FetcherTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants