Add field to change HTTP port by mugiwara-tech · Pull Request #13479 · JabRef/jabref

mugiwara-tech · 2025-07-05T23:55:22Z

My intended steps is to update all classes and configuration files to be able to change the HTTP port. I would like feedback to accomplish this which includes removing or adding lines of code.

Steps to test

You can test this task by building the application and checking JabRef preferences -> general -> HTTP Server and check to see if the box in the section has the port number 23119. Next, run the test on line 10 in the RemotePreferencesTest class.

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
[/] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (if change is visible to the user)
[/] 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.

koppor

Did you use a web browser connecting? (http://localhost:23119 is default)

Try to change the port to 8892 and then connect to http://localhost:8892.

If if fails, you probably did not change getHttpServerUri() {

koppor · 2025-07-06T09:32:14Z

    public @NonNull URI getHttpServerUri() {
        try {
-            return new URI("http://" + RemotePreferences.getIpAddress().getHostAddress() + ":23119");
+            return new URI("http://" + RemotePreferences.getIpAddress().getHostAddress() + ":" + getHttpPort());


Do you think, you are able to add another validator for checking that an URI can be constructed?

Extract this line into a new method. The validator calls it and if there is an exception, the host is invalid.

I'll attempt to do that.

Do you mean attempt to test an HTTP connection or just making sure the IP Address is in the correct format along with everything else?

just ensuring that new URI( does not throw any exception - no self-implemented checks, just re-use waht's offered by JAva

I'm confused since new URI throws the URISyntaxException - before my changes. I changed the code a few days ago to have a method isUriValid, but I'm not sure if it is necessary. Unfortunately, I believe I need some guidance on this.

Maybe, its too complicated. Just add a validator. Don't use isValid here - just construct the URL

However, construct the URL in isValid and here THE SAME WAY.

THERE: With multiple parameters to URI constructor (which is better than the existing code)
HERE: Stirng concatination (existing code)

So do not use isValid here - which I'm assuming is above the return statement on line 100 (or line 110 with recent push with my changes). But construct the URL in isValid and here the same way - confused by this.

However, what I believe is meant by there is the declaration of the isValid method - I changed this to only accept two params.

Based on my recent push, I put the validator above the returned URI - which includes the construction of the concatenated string in the param.

Just making sure I'm understanding this correctly.

palukku · 2025-07-06T15:06:26Z

    <Label styleClass="sectionHeader" text="%HTTP Server" />
-    <CheckBox fx:id="enableHttpServer" text="%Enable HTTP Server (e.g., for JabMap)" />
+    <HBox alignment="CENTER_LEFT" spacing="10.0">
+        <CheckBox fx:id="enableHttpServer" text="%Enable HTTP Server (e.g., for JabMap)" />


Maybe also add on port to the end of the text to clarify that the integer value refers to a port

What about line 88? Would the id name httpServerPort be enough to clarify that's it's a port?

But that information is not visible to the user, i mean the text that is being displayed to the user. So it would be better if it looked more like the one above for the single instance

Oh! Okay, I understand now.

into task-for-issue-13463

…ableHttpServer in GeneralTab.fxml

into task-for-issue-13463

koppor · 2025-07-10T14:07:58Z

        return InetAddress.getByName("localhost");
    }

+    private boolean isUriValid(String protocol, String hostAddress, int port) {


Try to use this in a validator...

Create a method accepting host AND port. - Nothing else

So I changed the validator to only accept host and port. This has been pushed. Is it fine as it is to only return a boolean?

trag-bot · 2025-07-10T18:18:33Z

    public @NonNull URI getHttpServerUri() {
        try {
-            return new URI("http://" + RemotePreferences.getIpAddress().getHostAddress() + ":23119");
+            isUriValid(RemotePreferences.getIpAddress().getHostAddress(), getHttpPort());


The isUriValid method's return value is ignored, making the call redundant. The method should either be removed or its result should be used to handle invalid URIs.

I can change the return statement here to use isUriValid to return a URI object instead of a boolean.

koppor

I think, I was wrong with the validation idea.

I streamlined the isValid method - and now its good to go. Thank you for your work.

trag-bot · 2025-07-11T02:39:35Z

@trag-bot didn't find any issues in the code! ✅✨

trag-bot · 2025-07-11T02:44:39Z


+    @Test
+    void isDifferentHttpPortTrue() {
+        assertTrue(preferences.isDifferentHttpPort(4000));


Using assertTrue with a boolean condition instead of asserting the actual values. Should use assertEquals to compare the actual HTTP port values for better test clarity and error messages.

trag-bot · 2025-07-11T02:44:40Z

+
+    @Test
+    void isDifferentHttpPortFalse() {
+        assertFalse(preferences.isDifferentHttpPort(3000));


Using assertFalse with a boolean condition instead of asserting the actual values. Should use assertEquals to compare the actual HTTP port values for better test clarity and error messages.

mugiwara-tech · 2025-07-11T03:12:29Z

I think, I was wrong with the validation idea.

I streamlined the isValid method - and now its good to go. Thank you for your work.

Thank you for the feedback. Yeah, I looked into how URI construction can use parameters instead of a string. Aside from that, I definitely believed I missed the big picture at some point

* upstream/main: Also label PR if good first issue is made (#13526) New Crowdin updates (#13529) chore(deps): update dependency org.apache.logging.log4j:log4j-to-slf4j to v2.25.1 (#13528) chore: bump-okhttp-4.12.0-to-5.0.0 (#13521) Have the picker always on top (#13525) Refactor PushToApplications and split into logic and GUI (#13514) Add field to change HTTP port (#13479) Fix trigger of comment Use Java 11 isBlank (#13523) Add run openrewrite (#13524) Comment ion PR just opened (#13522) Improve merge logic to prefer valid year and entry type (#13506) Update dependency com.konghq:unirest-modules-gson to v4.4.12 (#13517) Add rpm target (#13516) Revert module name changes for remaining 'unnamed' Jars (#13515) fix: revert Java module names to restore Status Log compatibility in JabRef 5.15 (#13511) update java vendor in devcontainer and sdkmanrc (#13513)

Add field to change HTTP port

c7798cd

koppor requested changes Jul 6, 2025

View reviewed changes

mugiwara-tech and others added 2 commits July 5, 2025 19:50

Change URI to include the user's HTTP port

1ce282e

Merge branch 'main' into task-for-issue-13463

157c573

mugiwara-tech requested a review from koppor July 6, 2025 02:05

koppor reviewed Jul 6, 2025

View reviewed changes

palukku reviewed Jul 6, 2025

View reviewed changes

koppor mentioned this pull request Jul 6, 2025

Hotfix for non-shutdown of http server #13485

Merged

2 tasks

mugiwara-tech added 2 commits July 7, 2025 08:08

Add URI validator and validate before returning URI for HTTP server

df53f2f

Merge branch 'task-for-issue-13463' of https://github.com/dbolate/jabref

59c2e07

into task-for-issue-13463

mugiwara-tech requested a review from koppor July 7, 2025 13:11

mugiwara-tech and others added 4 commits July 7, 2025 09:04

Merge branch 'JabRef:main' into task-for-issue-13463

9f04c77

Update JabRef_en.properties to resolve issues for updated text for en…

47aeffd

…ableHttpServer in GeneralTab.fxml

Merge branch 'task-for-issue-13463' of https://github.com/dbolate/jabref

92a0887

into task-for-issue-13463

Merge branch 'main' into task-for-issue-13463

987ed00

koppor reviewed Jul 10, 2025

View reviewed changes

Change isUriValid to only accept host address and port number

bb33695

trag-bot Bot reviewed Jul 10, 2025

View reviewed changes

koppor added 2 commits July 11, 2025 04:25

Construct URI the same way

a569a88

Remove obsolete method

6ca44e7

koppor marked this pull request as ready for review July 11, 2025 02:38

koppor enabled auto-merge July 11, 2025 02:38

koppor approved these changes Jul 11, 2025

View reviewed changes

trag-bot Bot reviewed Jul 11, 2025

View reviewed changes

koppor added this pull request to the merge queue Jul 11, 2025

Merged via the queue into JabRef:main with commit 4506db9 Jul 11, 2025
1 check passed

github-actions Bot mentioned this pull request Jul 11, 2025

Add http port configuration #13463

Closed

Uh oh!

Conversation

mugiwara-tech commented Jul 5, 2025

Steps to test

Mandatory checks

Uh oh!

koppor 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!

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!

palukku Jul 6, 2025 • 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!

mugiwara-tech Jul 10, 2025 • 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!

trag-bot Bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot commented Jul 11, 2025

Uh oh!

trag-bot Bot Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mugiwara-tech commented Jul 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

palukku Jul 6, 2025 •

edited

Loading

mugiwara-tech Jul 10, 2025 •

edited

Loading