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

Add option to clear recent libraries #10027

Merged
merged 10 commits into from
Jun 25, 2023
Merged

Conversation

dkokkotas
Copy link
Contributor

@dkokkotas dkokkotas commented Jun 22, 2023

Resolves #10003

The current pull request serves as a new feature proposal, that provides the option to clear recent libraries' history. Essentially, a new menu item is included within the File --> Recent Libraries section that allows the user to erase any library previously opened.

Step by Step

  1. User opens a library
  2. User clicks on File section
    a. Selects Recent Libraries menu
    b. Clicks on Clear Recent Libraries option
  3. Afterwards, the recent libraries will be successfully cleared, and the Recent Libraries menu will be disabled until the user opens a new library.

About the implementation

  • Enhanced FileHistoryMenu class with the new menu item option, that serves the erasing of the recent libraries' history.
  • Added unit test within FileHistoryMenuTest class to validate the expected behavior of the operation.
clear-recent-libraries-option.mov

Mandatory checks

  • 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@dkokkotas
Copy link
Contributor Author

@HoussemNasri

As you may see I purposely let the unit test failing, as it is not clear to me why while setting three menu items:

  • test.bib file
  • Menu Separator
  • clearRecentLibraries item,

the actual added items are in total two, excluding the test.bib file.

@HoussemNasri
Copy link
Member

HoussemNasri commented Jun 23, 2023

getItems() returns the menu items inside FileHistoryMenu. I don't know why it's returning 2 instead of 3, assuming the separator is considered a menu item, but it's the wrong approach anyway. For example, if we added a new menu item to FileHistoryMenu then we need to update the test cases for the clearing of recent files, but they are irrelevant.

Usually, we would have the view and view state in separate classes. That way we can test the view model which holds the state of the UI component. But in this case, FileHistoryMenu doesn't follow the MVVM pattern.

We don't test the view because it is usually easy to validate, by just looking at it. We kind of assume that if the view model is in the correct state then the UI should reflect correctly.

What I suggest is to expose the FileHistory object and test that. You can add the test to FileHistoryMenuTest since it doesn't have a view model.

@dkokkotas
Copy link
Contributor Author

but it's the wrong approach anyway.

Certainly, it must not be considered. I just included that because it was unclear to me what causes the failure.

@dkokkotas dkokkotas marked this pull request as ready for review June 23, 2023 18:14
@dkokkotas
Copy link
Contributor Author

dkokkotas commented Jun 24, 2023

@HoussemNasri
Following up to our previous communication, I made some further adjustments in both FileHistoryMenu and
FileHistoryMenuTest classes.

Note : Failing test is not related to to the current code.

void recentLibrariesAreCleared() {
fileHistoryMenu.newFile(Path.of(BIBTEX_LIBRARY_PATH.concat("bibtexFiles/test.bib")));

fileHistoryMenu.clearRecentLibraries.getOnAction().handle(null); // retrieves the event handler
Copy link
Member

Choose a reason for hiding this comment

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

How about calling FileHistoryMenu#clearLIbrariesHistory directly? Doing so would make the transition to MVVM in the future easier because view models can't access views which in this case the button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved


public void clearLibrariesHistory() {
history.clear();
setDisable(true);
Copy link
Member

Choose a reason for hiding this comment

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

This would disable the button when recent libraries are empty, but I don't see any code that would re-enable it when recent library item is added.

Copy link
Contributor Author

@dkokkotas dkokkotas Jun 24, 2023

Choose a reason for hiding this comment

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

I assume that FileHistoryMenu#newFile(Path file) invocation within the OpenDatabaseAction class, will re-enable the item as the aforementioned method declares : setDisable(false). To my understanding, the menu Recent Libraries shall be disabled unless the user opens a library.

I also tested it manually and the system's behavior is as expected. When we open a new library the Recent libraries menu will be correctly enabled and the recent libraries will be listed.

Copy link
Member

@HoussemNasri HoussemNasri left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Works here.

Other JabRef actions emit a notification of the successful execution. I missed that. However, I think, we should not increase the number of notifications, but more reduce them. Therefore, it is OK for me as is.

@koppor koppor merged commit b0fba18 into JabRef:main Jun 25, 2023
12 checks passed
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.

Add the option to clear recent libraries
3 participants