Use environment variables for hostname detection by koppor · Pull Request #9910 · JabRef/jabref

koppor · 2023-05-16T20:13:58Z

### Compulsory checks
- [x] Change in `CHANGELOG.md` described in a way that is understandable for the average user (if applicable)
- [x] 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](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
- [ ] [Checked documentation](https://docs.jabref.org/): 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.

calixtus

More than nitpicks. Sorry.

calixtus · 2023-05-17T09:59:28Z

+    /**
+     * The constructor is made package private to enforce this as a singleton class while allowing testing of the class
+     */
+    JabRefPreferences() {


Yet it could now be instantiated by any other class in the same package but the internal singleton 'factory'. Please do not remove private, use the singleton factory getInstance also for the tests instead.

I would need to rewrite "get instance". -- A new method "reset references"
The intention is that the preferences are created from scratch at each test. No reuse.

The main paint is about re initialization each time in the tests

Reading your next comment, the whole change is unnecessary. However, I wonder why there are no tests for preferences at all.

Right now the preferences work live on the real preferences data. there are no tests at all, because the whole class is still untestable. This is one driver for me to refactor the whole preferences mechanic. So we have a testable part that can be instantiated with a mock dao (?) and the real java preferences.

calixtus · 2023-05-17T10:08:05Z

+        String hostName;
+        // Following code inspired by https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/SystemUtils.html#getHostName--
+        // See also https://stackoverflow.com/a/20793241/873282
+        if (OS.WINDOWS) {
+            hostName = System.getenv("COMPUTERNAME");
+        } else {
+            hostName = System.getenv("HOSTNAME");
        }
+        if (StringUtil.isBlank(hostName)) {
+            try {
+                hostName = InetAddress.getLocalHost().getHostName();
+            } catch (UnknownHostException e) {
+                LOGGER.info("Hostname not found. Using \"localhost\" as fallback.", e);
+                hostName = "localhost";
+            }
+        }
+
+        userAndHost = get(DEFAULT_OWNER) + '-' + hostName;
+        return userAndHost;
    }


Based on the open-closed design principle, i believe this should all go to the OS classes instead, so the JabRefPreferences does not have to distinguish between the different os, but just call to the proper overridden method. Also the call directly to System.getenv would imho break the level of abstraction.

The change was easy: 151ea22 (#9910). Huge preparation work required though: 84899aa (#9910).

calixtus · 2023-05-17T10:16:39Z

    private final ObjectProperty<Path> backupDiretory = new SimpleObjectProperty<>();

-    public FilePreferences(String user,
+    public FilePreferences(String userAndHost,


Single responsibility? Please use separate variables instead and differentiate between the vars in FilePreferences and InternalPreferences if you want to use it for different purposes.

I renamed the variables to match the existing (!!!) content. Separatiin can be done in a follow up PR.

In other words, calixtus can do the separation in a follow up pr? 😅

The renaming is now in 7025d45 (#9910). I think, the combination is OK, because it is "only" needed for the directory and the stitching together should be done as early as possible.

Nevertheless, some changes will be necessary to prevent restart of JabRef when the owner name is changed. I made a small beginning at cc40eaf (#9910).

I think, you are right. Was nevertheless too much work for me now ^^

…anged

koppor · 2023-06-08T13:31:47Z

After this is merged, work should continue at #9990.

* upstream/main: (68 commits) Fix issue 9863 - Change Tab selection order (#9907) New Crowdin updates (#9994) Enable editing typo with double clicking on field name in custom entry type (#9977) Remove "Field name:" from localization - we already have "Field name" (#9991) Changed database to catalog in org.jabref.gui.slr and org.jabref.logic.crawler (#9989) Use environment variables for hostname detection (#9910) Add cleanup activity for URL field (#9970) Fix freezing when fetching IBSN and no results are found (#9987) Make Group(Node)TreeViewModel more OO (#9978) Fix container for group item count still visible if display count is off (#9980) Fix paste of BibTeX data (#9985) Bring back SimplifyBoolean* and UnnecessaryParantheses (and refine guide) (#9981) Add new menu entry to remove braces from selection, aka unprotect it. (#9968) User specific comment (#9727) Bump org.openrewrite.recipe:rewrite-recipe-bom from 1.19.3 to 2.0.0 (#9975) Fix typos Remove non-needed link Fix typos Enable RemoveJavaDocAuthorTag (#9973) Bump com.fasterxml.jackson.datatype:jackson-datatype-jsr310 (#9974) ...

Siedlerchr reviewed May 16, 2023

View reviewed changes

Comment thread src/main/java/org/jabref/preferences/JabRefPreferences.java Outdated

calixtus requested changes May 17, 2023

View reviewed changes

calixtus added the status: changes-required Pull requests that are not yet complete label May 17, 2023

ThiloteE added the component: logging label Jun 6, 2023

koppor added 2 commits June 7, 2023 11:47

Rename "user" to "userAndHost" (adapt test mocks and fix two typos)

7025d45

Refactor NativeDesktop to be an abstract class (more natural)

84899aa

koppor force-pushed the fix-hostname-detection branch from 79ccba1 to 151ea22 Compare June 7, 2023 10:03

koppor added 2 commits June 7, 2023 12:03

Add NativeDesktop#getHostName (and use it)

151ea22

If the owner is changed, the default file directory should also be ch…

cc40eaf

…anged

JabRef deleted a comment from github-actions Bot Jun 7, 2023

koppor added 3 commits June 7, 2023 12:10

Fix imports

d07b69c

Even more "dependency" updates

07f4bd8

Fix JavaDoc

eafe42c

Siedlerchr approved these changes Jun 7, 2023

View reviewed changes

Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes-required Pull requests that are not yet complete labels Jun 7, 2023

Siedlerchr merged commit 85386e6 into main Jun 8, 2023

Siedlerchr deleted the fix-hostname-detection branch June 8, 2023 14:40

Siedlerchr mentioned this pull request Feb 26, 2024

Changing owner name crashes the application #10924

Closed

2 tasks

koppor mentioned this pull request Feb 27, 2024

Fix "Changing owner name crashes the application" #10932

Merged

6 tasks

Uh oh!

Conversation

koppor commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calixtus May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Jun 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

koppor commented May 16, 2023 •

edited

Loading

calixtus May 17, 2023 •

edited

Loading