Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public class GeneralTab extends AbstractPreferenceTabView<GeneralTabViewModel> i
@FXML private CheckBox remoteServer;
@FXML private TextField remotePort;
@FXML private CheckBox enableHttpServer;
@FXML private TextField httpServerPort;
@FXML private Button remoteHelp;
@Inject private FileUpdateMonitor fileUpdateMonitor;
@Inject private BibEntryTypesManager entryTypesManager;
Expand Down Expand Up @@ -143,6 +144,7 @@ public void initialize() {

Platform.runLater(() -> {
validationVisualizer.initVisualization(viewModel.remotePortValidationStatus(), remotePort);
validationVisualizer.initVisualization(viewModel.httpPortValidationStatus(), httpServerPort);
validationVisualizer.initVisualization(viewModel.fontSizeValidationStatus(), fontSize);
validationVisualizer.initVisualization(viewModel.customPathToThemeValidationStatus(), customThemePath);
});
Expand All @@ -152,6 +154,8 @@ public void initialize() {
remotePort.disableProperty().bind(remoteServer.selectedProperty().not());

enableHttpServer.selectedProperty().bindBidirectional(viewModel.enableHttpServerProperty());
httpServerPort.textProperty().bindBidirectional(viewModel.httpPortProperty());
httpServerPort.disableProperty().bind(enableHttpServer.selectedProperty().not());
}

@FXML
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ public class GeneralTabViewModel implements PreferenceTabViewModel {
private final StringProperty remotePortProperty = new SimpleStringProperty("");
private final Validator remotePortValidator;
private final BooleanProperty enableHttpServerProperty = new SimpleBooleanProperty();
private final StringProperty httpPortProperty = new SimpleStringProperty("");
private final Validator httpPortValidator;
private final TrustStoreManager trustStoreManager;

private final FileUpdateMonitor fileUpdateMonitor;
Expand Down Expand Up @@ -160,14 +162,28 @@ public GeneralTabViewModel(DialogService dialogService, GuiPreferences preferenc
Localization.lang("Network"),
Localization.lang("Remote operation"),
Localization.lang("You must enter an integer value in the interval 1025-65535"))));

httpPortValidator = new FunctionBasedValidator<>(
httpPortProperty,
input -> {
try {
int portNumber = Integer.parseInt(httpPortProperty().getValue());
return RemoteUtil.isUserPort(portNumber);
} catch (NumberFormatException ex) {
return false;
}
},
ValidationMessage.error("%s".formatted(Localization.lang("You must enter an integer value in the interval 1025-65535"))));
this.trustStoreManager = new TrustStoreManager(Path.of(preferences.getSSLPreferences().getTruststorePath()));
}

public ValidationStatus remotePortValidationStatus() {
return remotePortValidator.getValidationStatus();
}

public ValidationStatus httpPortValidationStatus() {
return httpPortValidator.getValidationStatus();
}

@Override
public void setValues() {
selectedLanguageProperty.setValue(workspacePreferences.getLanguage());
Expand Down Expand Up @@ -210,6 +226,7 @@ public void setValues() {
remotePortProperty.setValue(String.valueOf(remotePreferences.getPort()));

enableHttpServerProperty.setValue(remotePreferences.enableHttpServer());
httpPortProperty.setValue(String.valueOf(remotePreferences.getHttpPort()));
}

@Override
Expand Down Expand Up @@ -276,6 +293,12 @@ public void storeSettings() {
remoteListenerServerManager.stop();
}

getPortAsInt(httpPortProperty.getValue()).ifPresent(newPort -> {
if (remotePreferences.isDifferentHttpPort(newPort)) {
remotePreferences.setHttpPort(newPort);
}
});

HttpServerManager httpServerManager = Injector.instantiateModelOrService(HttpServerManager.class);
// stop in all cases, because the port might have changed
httpServerManager.stop();
Expand Down Expand Up @@ -308,6 +331,10 @@ public boolean validateSettings() {
validator.addValidators(remotePortValidator);
}

if (enableHttpServerProperty.getValue()) {
validator.addValidators(httpPortValidator);
}

if (fontOverrideProperty.getValue()) {
validator.addValidators(fontSizeValidator);
}
Expand Down Expand Up @@ -442,6 +469,10 @@ public BooleanProperty enableHttpServerProperty() {
return enableHttpServerProperty;
}

public StringProperty httpPortProperty() {
return httpPortProperty;
}

public void openBrowser() {
String url = "https://themes.jabref.org";
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,10 @@
</HBox>

<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) on port" />
<TextField fx:id="httpServerPort" maxWidth="100.0" HBox.hgrow="ALWAYS" />
</HBox>

<Label styleClass="sectionHeader" text="%Libraries"/>
<GridPane hgap="10.0" vgap="4.0">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ public class JabRefCliPreferences implements CliPreferences {
// Remote
private static final String USE_REMOTE_SERVER = "useRemoteServer";
private static final String REMOTE_SERVER_PORT = "remoteServerPort";
private static final String HTTP_SERVER_PORT = "httpServerPort";
private static final String ENABLE_HTTP_SERVER = "enableHttpServer";

private static final String AI_ENABLED = "aiEnabled";
Expand Down Expand Up @@ -639,6 +640,7 @@ public JabRefCliPreferences() {
defaults.put(USE_REMOTE_SERVER, Boolean.TRUE);
defaults.put(REMOTE_SERVER_PORT, 6050);
defaults.put(ENABLE_HTTP_SERVER, Boolean.FALSE);
defaults.put(HTTP_SERVER_PORT, 23119);

defaults.put(EXTERNAL_JOURNAL_LISTS, "");
defaults.put(USE_AMS_FJOURNAL, true);
Expand Down Expand Up @@ -1250,10 +1252,12 @@ public RemotePreferences getRemotePreferences() {
remotePreferences = new RemotePreferences(
getInt(REMOTE_SERVER_PORT),
getBoolean(USE_REMOTE_SERVER),
getInt(HTTP_SERVER_PORT),
getBoolean(ENABLE_HTTP_SERVER));

EasyBind.listen(remotePreferences.portProperty(), (_, _, newValue) -> putInt(REMOTE_SERVER_PORT, newValue));
EasyBind.listen(remotePreferences.useRemoteServerProperty(), (_, _, newValue) -> putBoolean(USE_REMOTE_SERVER, newValue));
EasyBind.listen(remotePreferences.httpPortProperty(), (_, _, newValue) -> putInt(HTTP_SERVER_PORT, newValue));
EasyBind.listen(remotePreferences.enableHttpServerProperty(), (_, _, newValue) -> putBoolean(ENABLE_HTTP_SERVER, newValue));

return remotePreferences;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ public class RemotePreferences {
private final IntegerProperty port;
private final BooleanProperty useRemoteServer;

private final IntegerProperty httpPort;
private final BooleanProperty enableHttpServer;

public RemotePreferences(int port, boolean useRemoteServer, boolean enableHttpServer) {
public RemotePreferences(int port, boolean useRemoteServer, int httpPort, boolean enableHttpServer) {
this.port = new SimpleIntegerProperty(port);
this.useRemoteServer = new SimpleBooleanProperty(useRemoteServer);
this.httpPort = new SimpleIntegerProperty(httpPort);
this.enableHttpServer = new SimpleBooleanProperty(enableHttpServer);
}

Expand Down Expand Up @@ -60,6 +62,22 @@ public void setUseRemoteServer(boolean useRemoteServer) {
this.useRemoteServer.setValue(useRemoteServer);
}

public int getHttpPort() {
return httpPort.getValue();
}

public IntegerProperty httpPortProperty() {
return httpPort;
}

public void setHttpPort(int httpPort) {
this.httpPort.setValue(httpPort);
}

public boolean isDifferentHttpPort(int otherHttpPort) {
return getHttpPort() != otherHttpPort;
}

public boolean enableHttpServer() {
return enableHttpServer.getValue();
}
Expand All @@ -77,9 +95,19 @@ public static InetAddress getIpAddress() throws UnknownHostException {
return InetAddress.getByName("localhost");
}

private boolean isUriValid(String hostAddress, int port) {
try {
URI uri = new URI("http", null, hostAddress, port, null, null, null);
return true;
} catch (URISyntaxException e) {
return false;
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return new URI("http://" + RemotePreferences.getIpAddress().getHostAddress() + ":" + getHttpPort());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll attempt to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (UnknownHostException | URISyntaxException e) {
LOGGER.error("Could not create HTTP server URI. Falling back to default.", e);
try {
Expand Down
2 changes: 1 addition & 1 deletion jablib/src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -1817,7 +1817,7 @@ Remote\ operation=Remote operation
Remote\ services=Remote services
Cannot\ use\ port\ %0\ for\ remote\ operation;\ another\ application\ may\ be\ using\ it.\ Try\ specifying\ another\ port.=Cannot use port %0 for remote operation; another application may be using it. Try specifying another port.

Enable\ HTTP\ Server\ (e.g.,\ for\ JabMap)=Enable HTTP Server (e.g., for JabMap)
Enable\ HTTP\ Server\ (e.g.,\ for\ JabMap)\ on\ port=Enable HTTP Server (e.g., for JabMap) on port
HTTP\ Server=HTTP Server

Grobid\ URL=Grobid URL
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,31 @@ class RemotePreferencesTest {

@BeforeEach
void setUp() {
preferences = new RemotePreferences(1000, true, false);
preferences = new RemotePreferences(1000, true, 3000, false);
}

@Test
void getPort() {
assertEquals(1000, preferences.getPort());
}

@Test
void getHttpPort() {
assertEquals(3000, preferences.getHttpPort());
}

@Test
void setPort() {
preferences.setPort(2000);
assertEquals(2000, preferences.getPort());
}

@Test
void setHttpPort() {
preferences.setHttpPort(4000);
assertEquals(4000, preferences.getHttpPort());
}

@Test
void useRemoteServer() {
assertTrue(preferences.useRemoteServer());
Expand All @@ -43,8 +54,18 @@ void isDifferentPortTrue() {
assertTrue(preferences.isDifferentPort(2000));
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@Test
void isDifferentPortFalse() {
assertFalse(preferences.isDifferentPort(1000));
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}