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.

Sign up for GitHub

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

Jump to bottom

Fix for issue custom api key always saved even on cancel #11977

Merged
merged 13 commits into from
Oct 16, 2024
Merged

Fix for issue custom api key always saved even on cancel #11977

merged 13 commits into from
Oct 16, 2024

Conversation

u7663394
Copy link
Contributor

@u7663394 u7663394 commented Oct 16, 2024 •
edited
Loading

This PR fixes an issue where changes made to the "Custom API key" table in the Web Search tab of the preferences are always saved, even when the user clicks Cancel.

Fix Implemented:
Modified the WebSearchTabViewModel class:

  • Removed the unnecessary editableApiKeys property.
  • Changed apiKeys from a ListProperty to an ObservableList.
  • Initialized apiKeys with a deep copy of the preferences' API keys in setValues().
  • Ensured that changes are only saved back to the preferences in storeSettings().
  • Simplified the code by eliminating redundant property declarations and assignments.

Closes #11925

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • 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.

Made changes in api key and press Cancel:
Make change

Discard now:
Discard

@u7663394 u7663394 mentioned this pull request Oct 16, 2024
Closed
@koppor koppor requested review from calixtus and Siedlerchr October 16, 2024 08:02
@Siedlerchr Siedlerchr changed the title Fix for issue 11925 Fix for issue custom api key always saved even on cancel Oct 16, 2024
calixtus
calixtus requested changes Oct 16, 2024
View reviewed changes
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.

One nitpick, otherwise looks good to me

src/main/java/org/jabref/gui/preferences/websearch/WebSearchTabViewModel.java Outdated Show resolved Hide resolved
LoayGhreeb
LoayGhreeb requested changes Oct 16, 2024
View reviewed changes
src/main/java/org/jabref/gui/preferences/websearch/WebSearchTabViewModel.java Outdated
Comment on lines 168 to 171
// Save changes from tempApiKeys back to the actual settings
preferences.getImporterPreferences().getApiKeys().clear();
preferences.getImporterPreferences().getApiKeys().addAll(apiKeys);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The preferences are already saved at line 191.

Copy link
Member

Choose a reason for hiding this comment

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

Loay 'Eagleeyes' Ghreeb

src/main/java/org/jabref/gui/preferences/websearch/WebSearchTabViewModel.java Outdated
Comment on lines 144 to 152
// Initialize tempApiKeys with a deep copy of the actual API Keys, so changes in the UI do not directly affect the actual settings
tempApiKeys.set(FXCollections.observableArrayList(
preferences.getImporterPreferences().getApiKeys().stream()
.map(apiKey -> new FetcherApiKey(apiKey.getName(), apiKey.shouldUse(), apiKey.getKey()))
.collect(Collectors.toList())
));
// Set the UI-bound apiKeys property to tempApiKeys
apiKeys.set(tempApiKeys);

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can do this directly on the apiKeys list without the need for tempApiKeys.

Also, change the apiKeys list to an ObservableList instead of a ListProperty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback. I have simplified the code by directly working with the apiKeys.

I have also changed apiKeys from a ListProperty to an ObservableList and deleted redundant code about saving perfermance.

I have edited and updated PR Description above. Thank you!

LoayGhreeb
LoayGhreeb reviewed Oct 16, 2024
View reviewed changes
src/main/java/org/jabref/gui/preferences/websearch/WebSearchTabViewModel.java Outdated
@@ -56,7 +56,8 @@ public class WebSearchTabViewModel implements PreferenceTabViewModel {
private final BooleanProperty grobidEnabledProperty = new SimpleBooleanProperty();
private final StringProperty grobidURLProperty = new SimpleStringProperty("");

private final ListProperty<FetcherApiKey> apiKeys = new SimpleListProperty<>();
// ObservableList to store API keys for editing in the UI
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the comment

LoayGhreeb
LoayGhreeb reviewed Oct 16, 2024
View reviewed changes
src/main/java/org/jabref/gui/preferences/websearch/WebSearchTabViewModel.java Outdated
@@ -138,7 +139,11 @@ public void setValues() {
grobidEnabledProperty.setValue(grobidPreferences.isGrobidEnabled());
grobidURLProperty.setValue(grobidPreferences.getGrobidURL());

apiKeys.setValue(FXCollections.observableArrayList(preferences.getImporterPreferences().getApiKeys()));
// Initialize apiKeys with a deep copy of the actual API Keys
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the comment

LoayGhreeb
LoayGhreeb reviewed Oct 16, 2024
View reviewed changes
src/main/java/org/jabref/gui/preferences/websearch/WebSearchTabViewModel.java Outdated
// Initialize apiKeys with a deep copy of the actual API Keys
apiKeys.setAll(preferences.getImporterPreferences().getApiKeys().stream()
.map(apiKey -> new FetcherApiKey(apiKey.getName(), apiKey.shouldUse(), apiKey.getKey()))
.collect(Collectors.toList()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be simplified

Suggested change
.collect(Collectors.toList()));
.toList());

Copy link
Member

Choose a reason for hiding this comment

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

Careful, Collectors.toList() creates a new mutable array lust, .toList() an immutable one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OKay, so I reckon we'll continue using .collect(Collectors.toList()) to obtain a mutable list, since apiKeys needs to be mutable to allow modifications in the UI ?

I am a bit unsure about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we need an immutable list here.

@calixtus
Copy link
Member

Please test it once more to make sure that to list stuff is now done right. 😅

@u7663394
Copy link
Contributor Author

Please test it once more to make sure that to list stuff is now done right. 😅

Sure, I have tested both versions, and they both work properly. Both support the normal operation of save and cancel.

@calixtus calixtus added this pull request to the merge queue Oct 16, 2024
Merged via the queue into JabRef:main with commit 758dab4 Oct 16, 2024
23 checks passed
ExrosZ pushed a commit to ExrosZ/jabref that referenced this pull request Oct 17, 2024
@u7663394 @ExrosZ-Alt
Fix for issue custom api key always saved even on cancel (JabRef#11977)
e8a682a 
* Added temporary property to fix the issue.

* Added comments.

* Modified CHANGELOG.md

* Rename variable and modify comments.

* Use apiKeys directly and change to ObservableList

* Remove comments.

* Get an immutable list.
ExrosZ pushed a commit to ExrosZ/jabref that referenced this pull request Oct 17, 2024
@u7663394 @ExrosZ-Alt
Fix for issue custom api key always saved even on cancel (JabRef#11977)
dad9cd7 
* Added temporary property to fix the issue.

* Added comments.

* Modified CHANGELOG.md

* Rename variable and modify comments.

* Use apiKeys directly and change to ObservableList

* Remove comments.

* Get an immutable list.
ExrosZ pushed a commit to ExrosZ/jabref that referenced this pull request Oct 21, 2024
@u7663394 @ExrosZ-Alt
Fix for issue custom api key always saved even on cancel (JabRef#11977)
d36a65d 
* Added temporary property to fix the issue.

* Added comments.

* Modified CHANGELOG.md

* Rename variable and modify comments.

* Use apiKeys directly and change to ObservableList

* Remove comments.

* Get an immutable list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LoayGhreeb LoayGhreeb LoayGhreeb approved these changes

@calixtus calixtus calixtus approved these changes

@Siedlerchr Siedlerchr Awaiting requested review from Siedlerchr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Web search preferences "Custom API key" table modifications not discarded
3 participants
@u7663394 @calixtus @LoayGhreeb