[WIP] Fix: Prevent overwriting of imported preferences on save

[JxCl-L]

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

• [x] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
• [ ] Tests created for changes (if applicable)
• [x] Manually tested changed features in running JabRef (always required)
• [ ] Screenshots added in PR description (for UI changes)
• [x] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
• [x] 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.

[InAnYan]

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]

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 :smile: . Maybe it's not exactly the user would expect, but it will solve the issue correctly

[koppor]

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.

[JxCl-L]

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]

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 https://github.com/JabRef/jabref/pull/11926#pullrequestreview-2363828060

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

[InAnYan]

https://github.com/JabRef/jabref/pull/11926#issuecomment-2408530738

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

[koppor]

@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]

#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 https://github.com/JabRef/jabref/pull/11926#pullrequestreview-2363828060 to be able to reproduce and narrow down the issue?

[JxCl-L]

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]

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

Example:

1. Open preferences
2. Enable "Show previw as tab in entry editor"
3. Click "Save"
4. Open preferences
5. Export preferences
6. Disable "Show previw as tab in entry editor"
7. Click "Save"
8. Open preferences
9. Import preferences
10. See that "Show previw as tab in entry editor" is still disabled - but it should be enabled.
11. Close Preferences
12. Close JabRef
13. Start JabRef
14. Open preferences
15. 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]

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

This is based on the code of JabRef main repo:

1. Open preferences
2. 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
3. Change settings manually: In General Tab, Visual theme = dark In Export Tab, Export sort order = current table sort order
4. Click “Save” to close preferences
5. Open preferences
6. Import preferences with the xml file from step 2
7. See all settings not change in the preferences UI
8. Click “Save” to close preferences
9. Restart JabRef and open preferences
10. 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

1. Open preferences
2. 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
3. Change settings manually: In General Tab, Visual theme = dark In Export Tab, Export sort order = current table sort order
4. Click “Save” to close preferences
5. Open preferences
6. Import preferences with the xml file from step 2
7. See all settings not change in the preferences UI
8. Click “Save” to close preferences
9. Restart JabRef and open preferences
10. 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]

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.

1. preferences.importPreferences(file); "copyies" the preferences from the file to the internal storage and the JabRef in-memory store.
2. 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.

1. prefObj set to null
2. newPrefObj read using the getter
3. prefObj.migrateTo(newPrefObj) called # This migrates the object to the new data and all subscribers are notified on changes.
4. 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]

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.

[koppor]

Closing this issue due to inactivity :zzz: