Handling resetting of PushToApplicationPreferences #14691
Conversation
|
Hey @Hamza-Azeem! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. Please re-check our AI Usage Policy to ensure that your pull request is in line with it. It also contains links to our contribution guide in case of any other doubts related to our contribution workflow. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
github-actions
bot
added
status: changes-required
| if (defaults.get(key) == null) { | ||
| // Couldn't run the application without this condition. | ||
| LOGGER.info("************************* THIS KEY IS NULL {}", key); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Hello, Calixtus
Sorry if I made a mistake here, please can you guide me into how can I fix this?
I followed the steps written in https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/
and when I got to the step where I should run the application from gradle menu the compilation fails and I get a null pointer exception.
I will also work on the instructions given by ai to improve the code, but I would like your opinion on how to handle: getBooleanDefault method
I could have made a map linking keys in JabRefCliPreferences to the displayed names without modifying of PushApplications class or is there a better approach ?
There was a problem hiding this comment.
Dont touch getBooleanDefault method. Goal is to get rid of the defaults map and to put all the default values into the high level prefs objects.
| private String getEmptyIsDefault(String key, String defaultValue) { | ||
| final Preferences PREFS_NODE = Preferences.userRoot().node("/org/jabref"); | ||
| String result = PREFS_NODE.get(key, defaultValue); | ||
| if ("".equals(result)) { | ||
| return defaultValue; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Nope! Low level implementation of BackingStore access in highlevel preferences object.
| TEXWORKS("texworks", "TeXworks", "TeXworksPath"), | ||
| VIM("vim", "Vim", "vim"), | ||
| WIN_EDT("winedt", "WinEdt", "winEdtPath"), | ||
| SUBLIME_TEXT("sublime", "Sublime Text", "sublimeTextPath"), |
There was a problem hiding this comment.
This mixes lowest level (name in BackingStore) with high level implementation.
This is totally wrong.
| private PushToApplicationPreferences getPushToApplicationPreferencesFromBackingStore(PushToApplicationPreferences defaults) { | ||
| return new PushToApplicationPreferences( | ||
| get(PUSH_TO_APPLICATION, defaults.getActiveApplicationName()), | ||
| defaults.getCommandPaths(), | ||
| get(PUSH_EMACS_ADDITIONAL_PARAMETERS, defaults.getEmacsArguments()), | ||
| get(PUSH_VIM_SERVER, defaults.getVimServer()), | ||
| defaults.getCiteCommand(), | ||
| defaults.getDefaultCiteCommand() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why are the others not read from the prefs node here?
| commands.put(PushApplications.TEXMAKER.getDisplayName(), getEmptyIsDefault(PushApplications.TEXMAKER.getKey(), OS.detectProgramPath("texmaker", "Texmaker"))); | ||
| commands.put(PushApplications.WIN_EDT.getDisplayName(), getEmptyIsDefault(PushApplications.WIN_EDT.getKey(), OS.detectProgramPath("WinEdt", "WinEdt Team\\WinEdt"))); | ||
| commands.put(PushApplications.TEXSTUDIO.getDisplayName(), getEmptyIsDefault(PushApplications.TEXSTUDIO.getKey(), OS.detectProgramPath("texstudio", "TeXstudio"))); | ||
| commands.put(PushApplications.TEXWORKS.getDisplayName(), getEmptyIsDefault(PushApplications.TEXWORKS.getKey(), OS.detectProgramPath("texworks", "TeXworks"))); | ||
| commands.put(PushApplications.SUBLIME_TEXT.getDisplayName(), getEmptyIsDefault(PushApplications.SUBLIME_TEXT.getKey(), OS.detectProgramPath("subl", "Sublime"))); | ||
| commands.put(PushApplications.LYX.getDisplayName(), getEmptyIsDefault(PushApplications.LYX.getKey(), System.getProperty("user.home") + File.separator + ".lyx/lyxpipe")); | ||
| commands.put(PushApplications.VSCODE.getDisplayName(), getEmptyIsDefault(PushApplications.VSCODE.getKey(), OS.detectProgramPath("Code", "Microsoft VS Code"))); | ||
| commands.put(PushApplications.VIM.getDisplayName(), getEmptyIsDefault(PushApplications.VIM.getKey(), "vim")); |
There was a problem hiding this comment.
This should be default values, not read from BackingStore. They are always static.
There was a problem hiding this comment.
Noted. I will work on my mistakes
Hamza-Azeem
force-pushed
the
enable-resetting-of-push-to-application-notification
branch
from
252b5fd to
2d0f027
Compare
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
|
Hello @calixtus, I have removed getEmptyIsDefault from PushToApplicationPreference class, added the static keys instead of getting them from PushApplications Enum and got PushApplications into it's initial state without the modifications I did in the last commit. |
Maybe check the CI output? This is what you can address without someone telling you the exact steps 
This is not respecting our time. |
|
I think, this project could be too hard for you @Hamza-Azeem |
And maybe your first time using GitHub, seeing a CI pipeline running, seeing automated tests, interacting with a code hosting platform, using Google, using AI to assist you, etc. I can understand, that the whole development tooling used in practice might be overwhelming. I can also understand that English texts are hard to understand. I also can understand, that one cannot see the text "3 failing checks" and see not understanding it. Therefore, we created a bot posting comments. Not sure why you did not see #14691 (comment). |
| Map<String, String> commandPaths = new HashMap<>(defaults.getCommandPaths()); | ||
| Map<String, String> lookup = new HashMap<>(); | ||
| lookup.put(PushApplications.EMACS.getDisplayName(), PUSH_EMACS_PATH); | ||
| lookup.put(PushApplications.LYX.getDisplayName(), PUSH_LYXPIPE); | ||
| lookup.put(PushApplications.TEXMAKER.getDisplayName(), PUSH_TEXMAKER_PATH); | ||
| lookup.put(PushApplications.TEXSTUDIO.getDisplayName(), PUSH_TEXSTUDIO_PATH); | ||
| lookup.put(PushApplications.TEXWORKS.getDisplayName(), PUSH_TEXWORKS_PATH); | ||
| lookup.put(PushApplications.VIM.getDisplayName(), PUSH_VIM); | ||
| lookup.put(PushApplications.WIN_EDT.getDisplayName(), PUSH_WINEDT_PATH); | ||
| lookup.put(PushApplications.SUBLIME_TEXT.getDisplayName(), PUSH_SUBLIME_TEXT_PATH); | ||
| lookup.put(PushApplications.VSCODE.getDisplayName(), PUSH_VSCODE_PATH); | ||
| for(Map.Entry<String, String> entry : commandPaths.entrySet()) { | ||
| String oldKey = lookup.get(entry.getKey()); | ||
| String defaultValue = entry.getValue(); | ||
| entry.setValue(getEmptyIsDefault(oldKey, defaultValue)); | ||
| } |
There was a problem hiding this comment.
This looks incredibly complicated for a very simple task.
|
|
||
| public String getEmptyIsDefault(String key) { | ||
| String defaultValue = (String) defaults.get(key); | ||
| public String getEmptyIsDefault(String key, String defaultValue) { |
| private final String id; | ||
| private final String displayName; | ||
|
|
||
|
|
| this.displayName = displayName; | ||
| } | ||
|
|
||
|
|
|
|
||
|
|
|
|
||
|
|
||
| private PushToApplicationPreferences(){ | ||
| this.activeApplicationName = new SimpleStringProperty("Texmaker"); |
| commands.put("TeXstudio", OS.detectProgramPath("texstudio", "TeXstudio")); | ||
| commands.put("TeXworks", OS.detectProgramPath("texworks", "TeXworks")); | ||
| commands.put("Sublime Text", OS.detectProgramPath("subl", "Sublime")); | ||
| commands.put("LyX/Kile", System.getProperty("user.home") + File.separator + ".lyx/lyxpipe"); |
There was a problem hiding this comment.
| commands.put("LyX/Kile", System.getProperty("user.home") + File.separator + ".lyx/lyxpipe"); | |
| commands.put("LyX/Kile", USER_HOME + File.separator + ".lyx/lyxpipe"); |
| if(OS.WINDOWS){ | ||
| commands.put("Emacs", "emacsclient.exe"); | ||
| }else if(OS.OS_X || OS.LINUX){ | ||
| commands.put("Emacs", "emacsclient"); | ||
| } |
There was a problem hiding this comment.
| if(OS.WINDOWS){ | |
| commands.put("Emacs", "emacsclient.exe"); | |
| }else if(OS.OS_X || OS.LINUX){ | |
| commands.put("Emacs", "emacsclient"); | |
| } | |
| commands.put("Emacs", OS.WINDOWS ? "emacsclient.exe" : "emacsclient"); |
|
|
||
| private PushToApplicationPreferences(){ | ||
| this.activeApplicationName = new SimpleStringProperty("Texmaker"); | ||
| Map<String, String> commands = new HashMap<>(); |
There was a problem hiding this comment.
Why not FXCollections.observableMap here already?
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
…https://github.com/Hamza-Azeem/jabref into enable-resetting-of-push-to-application-notification
|
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Source Code Tests / Checkstyle (pull_request)" and click on it. In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push. |
|
The requested changes were not addressed for 14 days. Please follow-up in the next 7 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
github-actions
bot
added
the
status: stale
|
Closing after 3 weeks unfinished draft |
|
This pull requests was closed without merging. You have been unassigned from the respective issue #14410. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
4 participants
User description
Closes #14410
Handled PushToApplicationPreferences removed default config from JabRefCliPreferences, added private constructor for
PushToApplicationPreferences to handle default config with getDefault method, setAll method and followed the rest of instructions. Moved logic of getEmptyIsDefault method from JabRefCliPreferences to PushToApplicationPreferences but didnt delete it from JabRefCliPreferences. Modified PushApplications class by adding a new instance variable(key).
Steps to test
After running the application click on File then preferences, navigate to External programs.
Try changing Application to push entries to option, restart the application try resetting preferences.
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)PR Type
Enhancement, Bug fix
Description
Moved PushToApplicationPreferences default configuration from JabRefCliPreferences to PushToApplicationPreferences class
Added private constructor and getDefault() method for proper default initialization
Implemented setAll() method to enable resetting preferences to defaults
Updated getEmptyIsDefault() method signature to accept default value parameter
Added preference reset logic in clear() and importPreferences() methods
Diagram Walkthrough
File Walkthrough
JabRefCliPreferences.java
Refactored PushToApplicationPreferences initialization and resetjablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java
constructor
preferences from backing store with defaults
PushToApplicationPreferences.getDefault()
preference reset
PushToApplicationPreferences.java
Added default initialization and reset functionalityjablib/src/main/java/org/jabref/logic/push/PushToApplicationPreferences.java
instance
instance's values
PushApplications.java
Minor formatting adjustmentsjablib/src/main/java/org/jabref/logic/push/PushApplications.java