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

Fix for issue #4652: Add Find Unlinked Files Filter based on Date #7846

Merged
merged 67 commits into from
Jun 28, 2021
Merged

Fix for issue #4652: Add Find Unlinked Files Filter based on Date #7846

merged 67 commits into from
Jun 28, 2021

Conversation

gdrosos
Copy link
Contributor

@gdrosos gdrosos commented Jun 22, 2021

Hello everyone, we made 3 additions to the Find Unlinked Files Function as following:

  • Firstly, for each file we showed to the relevant Dialog near its name the date of the last modification.

  • Secondly, we added a Date-based Filter to show only specific files based on the date of the last modification.

Finally, we made it possible to sort the files based on the date of the last modification.

For future steps, we plan to:

  1. Address the potential requested changes by the reviewers.
  2. Add Unit Tests to the created code where applicable.

Fixes #4652

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

gliargovas and others added 30 commits May 5, 2021 17:48
Change getDisplayText to display the last edited time
The DateRange enum contains the different date range names to be displayed on the newly created "select by date" filter on the unlinked files dialog window.
Create the ViewModel object for the ExternalFilesDate field that will be an applicable search filter for external files.
The method has similar functionality to getDisplayText with the difference that it also returns the last edited date of a file to be displayed.
The file contains the implementation of the functionality for filtering the unlinked files by specified filters.
Create the view model representing a selected sort-by method.
Add the methods for sorting the unlinked local files based on last
edited date.
Import java.util.List that was used, but not imported.
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

some optimizations for the test code, otherwise lgtm

gdrosos and others added 4 commits June 27, 2021 21:16
* Use streams instead of creating new arrays.
* Use assertEquals and assertNotEquals instead of assertArrayEquals and
assertFalse.
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

4 issues I saw to make the changes more simple.

@gliargovas
Copy link
Contributor

Hello @Siedlerchr and @calixtus , we have addressed your suggestions. We are open to any further remarks.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Some more small ideas

Comment on lines 102 to 107
this.dateFilterList = FXCollections.observableArrayList(
DateRange.ALL_TIME,
DateRange.YEAR,
DateRange.MONTH,
DateRange.WEEK,
DateRange.DAY);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.dateFilterList = FXCollections.observableArrayList(
DateRange.ALL_TIME,
DateRange.YEAR,
DateRange.MONTH,
DateRange.WEEK,
DateRange.DAY);
this.dateFilterList = FXCollections.observableArrayList(DateRange.values());

Comment on lines 109 to 112
this.fileSortList = FXCollections.observableArrayList(
ExternalFileSorter.DEFAULT,
ExternalFileSorter.DATE_ASCENDING,
ExternalFileSorter.DATE_DESCENDING);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.fileSortList = FXCollections.observableArrayList(
ExternalFileSorter.DEFAULT,
ExternalFileSorter.DATE_ASCENDING,
ExternalFileSorter.DATE_DESCENDING);
this.fileSortList = FXCollections.observableArrayList(ExternalFileSorter.values());

gdrosos and others added 3 commits June 28, 2021 00:45
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
…sDialogViewModel

Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
@gdrosos
Copy link
Contributor Author

gdrosos commented Jun 27, 2021

Some more small ideas

Thanks @calixtus for the quick response, we commited your suggestions and we rearranged the sequence of DateTime Enums in order to have the correct order when the values() method is invoked

LOGGER.error("Could not retrieve file time", e);
}
LocalDateTime localDateTime = lastEditedTime
.toInstant()
Copy link
Member

Choose a reason for hiding this comment

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

IntelliJ complains here, that toInstant may throw a NPE. This seems possible, since you don't return when a IOException occurs. Is that something we should think about?

Copy link
Contributor Author

@gdrosos gdrosos Jun 27, 2021

Choose a reason for hiding this comment

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

Thanks @calixtus , so maybe we need to place the code in a try catch-block to handle the NPE or in the same try catch block so when an IO Exception occurs the toInstant() method call is omited.

Copy link
Member

Choose a reason for hiding this comment

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

Well, there must be a value returned from this method... Which one, if getLastModifiedTime fails?
Today? null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry you are right, we will return LocalDateTime.now() when an IOException occurs

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Thanks!

@gdrosos
Copy link
Contributor Author

gdrosos commented Jun 27, 2021

Looks good to me now. Thanks!

Thank you very much @calixtus, we also face a problem regarding passing the github action check regarding the creation of the installer and portable version for macOS. Thanks in advance.

@Siedlerchr
Copy link
Member

LGTM as well! You can ignore the mac, the action doesn't work on forks because it requires access to some secrets (and that are not avaiable on forks)

@gdrosos
Copy link
Contributor Author

gdrosos commented Jun 28, 2021

LGTM as well! You can ignore the mac, the action doesn't work on forks because it requires access to some secrets (and that are not avaiable on forks)

Thank you very much, so the only thing left now is to deal with the NPE @calixtus mentioned?

@calixtus
Copy link
Member

As soon as the tests are green, we can merge. Thanks!

@gdrosos
Copy link
Contributor Author

gdrosos commented Jun 28, 2021

Guys me and @gliargovas want to thank you very much for the cooperation and the help you offered us to deal with this issue, everything was great. We are more than happy to be part of this Community!

@calixtus
Copy link
Member

You're welcome. We really appreciate your work you put in JabRef.

@calixtus calixtus merged commit b4b3075 into JabRef:main Jun 28, 2021
@Siedlerchr
Copy link
Member

I can only fully agree with @calixtus, this was excellent work! We hope to see more from you in the future! :)

Siedlerchr added a commit that referenced this pull request Jun 30, 2021
* upstream/main: (26 commits)
  Add unit test to four test classes (#7651)
  Fix IEEE test (#7852)
  New Crowdin updates (#7859)
  Fix markdown syntax of ADRs
  add missing l10n (#7857)
  New Crowdin updates (#7847)
  Bump mockito-core from 3.11.1 to 3.11.2 (#7856)
  Bump checkstyle from 8.43 to 8.44 (#7855)
  Fix for issue #4652: Add Find Unlinked Files Filter based on Date (#7846)
  Fix for entering a backslash in the custom entry preview dialog (#7851)
  Fixed INSPIREFetcherTest
  Fixed TitleFetcherTest
  Ignore baeldung.com and tldrlegal.com from out link checks
  New Crowdin updates (#7845)
  New Crowdin updates (#7843)
  Refactoring and addition of unit tests (#7597)
  CLI option to write XMP metadata to pdfs (#7814)
  Add query validation for web search (#7809)
  change eclipse default output dir (#7842)
  Bump lucene-queryparser from 8.8.2 to 8.9.0 (#7835)
  ...
Siedlerchr added a commit that referenced this pull request Jul 4, 2021
…kflow-for-slr-search

* upstream/main: (31 commits)
  New translations JabRef_en.properties (German) (#7868)
  Fix test "higherTrustLevelWins()" (#7866)
  Change WM_CLASS to jabref (#7858)
  [Bot] Update CSL styles (#7865)
  Add unit test to four test classes (#7651)
  Fix IEEE test (#7852)
  New Crowdin updates (#7859)
  Fix markdown syntax of ADRs
  add missing l10n (#7857)
  New Crowdin updates (#7847)
  Bump mockito-core from 3.11.1 to 3.11.2 (#7856)
  Bump checkstyle from 8.43 to 8.44 (#7855)
  Fix for issue #4652: Add Find Unlinked Files Filter based on Date (#7846)
  Fix for entering a backslash in the custom entry preview dialog (#7851)
  Fixed INSPIREFetcherTest
  Fixed TitleFetcherTest
  Ignore baeldung.com and tldrlegal.com from out link checks
  New Crowdin updates (#7845)
  New Crowdin updates (#7843)
  Refactoring and addition of unit tests (#7597)
  ...

# Conflicts:
#	src/main/resources/l10n/JabRef_en.properties
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find NEW files (PDFs - based on date)
5 participants