Fix: Prevent overwriting of imported preferences on save #11926

JxCl-L · 2024-10-12T07:00:33Z

This PR addresses the issue where imported preferences were inadvertently overwritten when clicking the save button after an import.

Changes implemented:

Added a 'justImported' flag to PreferencesDialogViewModel
Modified storeAllSettings() to check this flag before saving
Updated importPreferences() to set the flag after successful import
Provided user feedback when attempting to save after import

These changes prevent unintended overwrite of imported settings without requiring extensive changes to the PreferencesTab interface or its implementations. It improves user experience by clearly communicating the state of preferences after import.

Closes #10939

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.

…abRef#10939)

InAnYan

Great work! Thanks for taking a years long unsolved issue!

The solution looks simple and I think it works, but probably we need something smarter.

I think immediately closing the preferences dialog is a better way (#10939 (comment)). Because after user imported settings there are no actions to do in preferences... After all, new values are not inserted into fields, and if user starts to modify something, then it won't be changed, because justImported is true.

So I think there are two ways to solve this problem:

Immediately close the window after import.
Or set all values to the fields (IMHO, this is nice UX, but it's too hard to do this in JabRef, so the first approach is better now).

I wonder what other maintainers think

InAnYan · 2024-10-12T07:40:05Z

src/main/java/org/jabref/gui/preferences/PreferencesDialogViewModel.java

@@ -174,6 +177,12 @@ public void storeAllSettings() {
            return;
        }

+        if (justImported) {
+            dialogService.notify(Localization.lang("Preferences were just imported. No additional changes to save."));


I think no additional message should be displayed, as nothing special happened. Also that would introduce a new translation string.

InAnYan · 2024-10-12T07:40:58Z

src/main/java/org/jabref/gui/preferences/PreferencesDialogViewModel.java

@@ -174,6 +177,12 @@ public void storeAllSettings() {
            return;
        }

+        if (justImported) {
+            dialogService.notify(Localization.lang("Preferences were just imported. No additional changes to save."));
+            justImported = false;


And this is not necessary 😄 , as no code will check it, because the function immediately returns after that.

Oops, Git review moment.

By this in my comment I meant justImported = false

koppor

I think, this does not fully solve the issue.

Scenario:

I as user import the preference. Assume the AI is disabled
I as user activate AI
I press save
I open the preference dialog again
I switch to the AI tab
AI is deactivated, but it should be activated

I think, the issue is to understand, why setValues() does not work.

To reproduce:

Get a saved preferences
Imort
Save
Check the diff to the saved preferences
Think

InAnYan · 2024-10-12T08:18:38Z

Yes, Oliver right now described the situation I mentioned: user decided to change a parameter after importing. But that wouldn't be applied because justImported is true

InAnYan · 2024-10-12T08:20:15Z

The reason why setValues() are not called is because that's toooo much work.

You see, preferences are just a Map<String, Object>. And that's it, it's very plain, but our settings properties are not plain, they are structured. And you would have to manually set all of them in the importing logic.

Thus I propose just to immediately close the window 😄 . Maybe it's not exactly the user would expect, but it will solve the issue correctly

koppor · 2024-10-12T11:31:25Z

The reason why setValues() are not called is because that's toooo much work.

It is called

    public void setValues() {
        memoryStickProperty.setValue(preferences.getInternalPreferences().isMemoryStickMode());

        for (PreferencesTab preferencesTab : preferenceTabs) {
            preferencesTab.setValues();
        }
    }

I don't understand, why it does not work. Therefore, the question to replicate the issue by createing an exported preference, changing something, importing it, exporting again and see what is changed.

…lues() with updateAllTabs() in PreferencesDialogViewModel.importPreferences

…ogViewModel.importPreferences

koppor

Did you test the changes systematically?

The code seems to be written by an AI, isn't it?

koppor · 2024-10-16T08:52:31Z

src/main/java/org/jabref/gui/preferences/PreferencesDialogViewModel.java

@@ -103,7 +103,8 @@ public void importPreferences() {
                     .ifPresent(file -> {
                         try {
                             preferences.importPreferences(file);
-                             setValues();
+//                             setValues();


Remove comment

koppor · 2024-10-16T08:54:38Z

src/main/java/org/jabref/gui/preferences/PreferencesDialogViewModel.java

+            if (tab instanceof PreferenceTabViewModel) {
+                ((PreferenceTabViewModel) tab).setValues();
+            }


Why the cast? -- org.jabref.gui.preferences.PreferencesTab#setValues is available in the interface.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

JxCl-L · 2024-10-16T21:10:35Z

I apologize for the premature submission. This PR is still a work in progress, and our members haven't fully tested the changes yet. I converted this into a draft PR for its current status. Thank you for your patience as I continue to develop and verify this solution.

koppor · 2024-10-16T22:21:18Z

I apologize for the premature submission. This PR is still a work in progress, and our members haven't fully tested the changes yet. I converted this into a draft PR for its current status. Thank you for your patience as I continue to develop and verify this solution.

As stated above: If you test, follow the steps at #11926 (review)

I think, your PR could work - you just need to fix checkstyle and remove the cast.

…Values() in importPreferences() to avoid imported preferences overwritten when clicking "Save" but still don't know why this works.

InAnYan · 2024-10-18T06:05:47Z

#11926 (comment)

Sorry, I see. I thought we had to manually call set() on every property on every tab

koppor · 2024-10-18T10:56:34Z

@JxCl-L In 5ee850a (#11926), you removed your code and kept the comment (// setValues()). With my review comment, I just meant the opposite: Remove the line // setValues(), which is a comment in Java terms.

koppor · 2024-10-18T10:57:53Z

#11926 (comment)

Sorry, I see. I thought we had to manually call set() on every property on every tab

Thus, we are at the beginning and do not know why this does not work?

@JxCl-L Did you or someone of your team follow the steps at #11926 (review) to be able to reproduce and narrow down the issue?

JxCl-L · 2024-10-19T00:42:41Z

We just found out that by removing the setValue(), the imported preferences would not be overwritten by clicking "Save", which kind of "solve" the issue. We are working on narrowing it down and figuring out why.
The issue only happens in some preferences not all. For example, when the theme preference is changed by import, clicking "Save" wouldn't affect the import. But for preference like "Export", clicking Save can overwrite the import.
Also I have follow the steps, and I found that if you immediately export after import without restart which means UI hasn't been updated yet, in the exported xml file, the theme preference would be same as import, while the "Export" preference would be same as UI which is not updated by import.

koppor · 2024-10-19T10:39:03Z

I think, there should be a step-by-step guide how to reproduce the issue and what happens on which code changes.

Example:

Open preferences
Enable "Show previw as tab in entry editor"
Click "Save"
Open preferences
Export preferences
Disable "Show previw as tab in entry editor"
Click "Save"
Open preferences
Import preferences
See that "Show previw as tab in entry editor" is still disabled - but it should be enabled.
Close Preferences
Close JabRef
Start JabRef
Open preferences
See that "Show previw as tab in entry editor" is enabled

Thus, there is something wrong at the import. Please debug the import!

JxCl-L · 2024-10-20T03:45:35Z

Thank you for the advice. Here is how to reproduce the issue:

This is based on the code of JabRef main repo:

Open preferences
See the original settings and export the xml file, in which:
In General Tab, Visual theme = light
In Export Tab, Export sort order = original order
Change settings manually:
In General Tab, Visual theme = dark
In Export Tab, Export sort order = current table sort order
Click “Save” to close preferences
Open preferences
Import preferences with the xml file from step 2
See all settings not change in the preferences UI
Click “Save” to close preferences
Restart JabRef and open preferences
See settings:
In General Tab, Visual theme = light (Import successful)
In Export Tab, Export sort order = current table sort order (Import overwritten due to step 7 and 8)

Conclusion: Clicking “Save” overwriting imported preferences only happens in some settings e.g. Export sort order in Export Tab, not Visual theme in General Tab.

This is based on the code of my forked JabRef repo branch fix-10939:
The only code difference is commenting the “setValues;” in the importPeferences method in PreferencesDialogViewModel.java

Open preferences
See the original settings and export the xml file, in which:
In General Tab, Visual theme = light
In Export Tab, Export sort order = original order
Change settings manually:
In General Tab, Visual theme = dark
In Export Tab, Export sort order = current table sort order
Click “Save” to close preferences
Open preferences
Import preferences with the xml file from step 2
See all settings not change in the preferences UI
Click “Save” to close preferences
Restart JabRef and open preferences
See settings:
In General Tab, Visual theme = light (Import successful)
In Export Tab, Export sort order = original order (Import successfully, not overwritten)

Conclusion: Removing the setValues() could avoid preferences overwritten by clicking “Save”. However, we are unsure why this would work and it is kind of beyond our current capabilities. Would you help us to narrow it down?

koppor · 2024-10-23T12:23:28Z

Conclusion: Removing the setValues() could avoid preferences overwritten by clicking “Save”. However, we are unsure why this would work and it is kind of beyond our current capabilities. Would you help us to narrow it down?

I started reading the code.

preferences.importPreferences(file); "copyies" the preferences from the file to the internal storage and the JabRef in-memory store.
setValues() reads the in-memory-store and puts it into the forms (e.g., org.jabref.logic.preferences.JabRefCliPreferences#getCitationKeyPatternPreferences)

The culpit is that "caching" at the beginning of the getter method:

if (citationKeyPatternPreferences != null) {

They are not null after the import, thus the old version is returned.

If all the objects are set to null, it won't affect all used preference objects, but better than nothing.

Task:

Add null setting to all preference objects in org.jabref.logic.preferences.JabRefCliPreferences#importPreferences.
Keep setValues() call.

@calixtus Discussion

The other option is that all preference objects get a migrateTo method.

prefObj set to null
newPrefObj read using the getter
prefObj.migrateTo(newPrefObj) called # This migrates the object to the new data and all subscribers are notified on changes.
local caching variable set to prefObj - so that new get calls will use the existing object

This seems to be much too much effort for me - and thus I asked @JxCl-L to null objects.

However, since the lifecycle of our objects is long (e.g., entry editor tabs), it won't be affected by that null hack - and we need this migrateTo. -- Nevertheless, I think, this is way too much effort and users should just restart JabRef to get the new settings working. Thsi is also what the preference GUI says sometimes.

HaipengYY · 2024-10-24T02:38:54Z

I made changes to the org.jabref.logic.preferences#importPreferences method by adding null assignments to all preference objects as suggested. However, this may introduce a new issue—when preferences are set to null, it can cause problems when clicking “Save” and then returning to the main interface, as some necessary preferences might remain unset. Additionally, the change did not fully resolve the original issue of overwriting preferences when clicking “Save” after importing an XML file. Could you please give us some advices? Thank you for your time.

…esDialogViewModel.java

koppor

img.png not linked. Please remove.

koppor · 2024-10-27T00:34:20Z

src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java

@@ -960,6 +961,32 @@ public void exportPreferences(Path path) throws JabRefException {
    public void importPreferences(Path file) throws JabRefException {
        try (InputStream is = Files.newInputStream(file)) {
            Preferences.importPreferences(is);
+            citationKeyPatternPreferences = null;


~~Please add a comment above stating that nullin will trigger re-reading the preferences.~~

Extract method forcePreferenceCacheRefresh and put all setting null to there.

koppor · 2024-10-27T00:34:27Z

src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java

@@ -27,6 +27,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;

+import javafx.application.Platform;


Why this new import?

koppor · 2024-10-27T00:36:14Z

src/main/java/org/jabref/gui/preferences/PreferencesDialogView.java

@@ -141,6 +141,7 @@ void exportPreferences() {
    @FXML
    void importPreferences() {
        viewModel.importPreferences();
+        preferenceTabList.refresh();


Why is this necessary - add a Java comment ore remove.

koppor · 2024-12-09T20:39:17Z

Closing this issue due to inactivity 💤

HaipengYY and others added 4 commits October 10, 2024 15:20

Hi

e5c718b

Reset

03d81b5

Update importPreferences() to set the flag after successful import (J…

c8ebe9f

…abRef#10939)

Update changelog

a270bb8

JxCl-L changed the title ~~Fix 10939~~ Fix: Prevent overwriting of imported preferences on save Oct 12, 2024

InAnYan requested changes Oct 12, 2024

View reviewed changes

koppor requested changes Oct 12, 2024

View reviewed changes

JxCl-L and others added 3 commits October 16, 2024 19:28

Fix Imported settings cleared when clicking "Save" by replacing setVa…

0f8c56a

…lues() with updateAllTabs() in PreferencesDialogViewModel.importPreferences

Fix bug: add the missing ";" after updateAllTabs() in PreferencesDial…

47db2b9

…ogViewModel.importPreferences

Merge branch 'main' into fix-10939

ebe512f

koppor requested changes Oct 16, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

Merge branch 'JabRef:main' into fix-10939

b2028ea

github-actions bot reviewed Oct 16, 2024

View reviewed changes

JxCl-L changed the title ~~Fix: Prevent overwriting of imported preferences on save~~ [WIP] Fix: Prevent overwriting of imported preferences on save Oct 16, 2024

JxCl-L marked this pull request as draft October 16, 2024 20:52

Fix bug: Remove updateAllTabs() because it does not work. Comment set…

5ee850a

…Values() in importPreferences() to avoid imported preferences overwritten when clicking "Save" but still don't know why this works.

koppor added the status: changes required Pull requests that are not yet complete label Oct 23, 2024

Add null to preference obj after importing xml file

76a1615

HaipengYY mentioned this pull request Oct 24, 2024

Fix imported settings cleared when clicking "Save" JxCl-L/jabref#4

Open

6 tasks

JxCl-L marked this pull request as ready for review October 24, 2024 03:36

JxCl-L changed the title ~~[WIP] Fix: Prevent overwriting of imported preferences on save~~ Fix: Prevent overwriting of imported preferences on save Oct 24, 2024

Discard changes to src/main/java/org/jabref/gui/preferences/Preferenc…

87b8935

…esDialogViewModel.java

koppor requested changes Oct 27, 2024

View reviewed changes

koppor closed this Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Prevent overwriting of imported preferences on save #11926

Fix: Prevent overwriting of imported preferences on save #11926

JxCl-L commented Oct 12, 2024

InAnYan left a comment

InAnYan Oct 12, 2024

InAnYan Oct 12, 2024

InAnYan Oct 12, 2024

koppor left a comment

InAnYan commented Oct 12, 2024

InAnYan commented Oct 12, 2024

koppor commented Oct 12, 2024

koppor left a comment

koppor Oct 16, 2024

koppor Oct 16, 2024

This comment was marked as outdated.

github-actions bot left a comment

JxCl-L commented Oct 16, 2024

koppor commented Oct 16, 2024

InAnYan commented Oct 18, 2024

koppor commented Oct 18, 2024

koppor commented Oct 18, 2024

JxCl-L commented Oct 19, 2024

koppor commented Oct 19, 2024

JxCl-L commented Oct 20, 2024

koppor commented Oct 23, 2024

HaipengYY commented Oct 24, 2024

koppor left a comment

koppor Oct 27, 2024

koppor Oct 27, 2024

koppor Oct 27, 2024

koppor commented Dec 9, 2024

Fix: Prevent overwriting of imported preferences on save #11926

Fix: Prevent overwriting of imported preferences on save #11926

Conversation

JxCl-L commented Oct 12, 2024

Mandatory checks

InAnYan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

InAnYan commented Oct 12, 2024

InAnYan commented Oct 12, 2024

koppor commented Oct 12, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

github-actions bot left a comment

Choose a reason for hiding this comment

JxCl-L commented Oct 16, 2024

koppor commented Oct 16, 2024

InAnYan commented Oct 18, 2024

koppor commented Oct 18, 2024

koppor commented Oct 18, 2024

JxCl-L commented Oct 19, 2024

koppor commented Oct 19, 2024

JxCl-L commented Oct 20, 2024

koppor commented Oct 23, 2024

HaipengYY commented Oct 24, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Dec 9, 2024