-
Notifications
You must be signed in to change notification settings - Fork 66
Feature/mvc setup #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3483b35
1002eac
e711db1
a2826b6
22213b0
f9f5a01
cd7a5ad
77c2714
45b99d9
c48ed72
8f02f1c
dff3fe2
a41b14b
c78f3bc
4862ead
812714b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,7 @@ | ||
| target/ | ||
| /.idea/ | ||
| .idea/ | ||
| .vscode/ | ||
| *.class | ||
| *.log | ||
| .env | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package com.example.chat; | ||
|
|
||
| import javafx.application.Application; | ||
| import javafx.fxml.FXMLLoader; | ||
| import javafx.scene.Scene; | ||
| import javafx.stage.Stage; | ||
|
|
||
| public class App extends Application { | ||
|
|
||
| @Override | ||
| public void start(Stage stage) throws Exception { | ||
| FXMLLoader loader = new FXMLLoader(getClass().getResource("/com/example/chat/ChatView.fxml")); | ||
| Scene scene = new Scene(loader.load()); | ||
| stage.setTitle("JavaFX Chat Client"); | ||
| stage.setScene(scene); | ||
| stage.show(); | ||
| } | ||
|
|
||
| public static void main(String[] args) { | ||
| launch(); | ||
| } | ||
| } | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package com.example.chat; | ||
|
|
||
| import javafx.fxml.FXML; | ||
| import javafx.scene.control.Button; | ||
| import javafx.scene.control.ListView; | ||
| import javafx.scene.control.TextField; | ||
|
|
||
| public class ChatController { | ||
|
|
||
| @FXML | ||
| private TextField inputField; | ||
|
|
||
| @FXML | ||
| private Button sendButton; | ||
|
|
||
| @FXML | ||
| private ListView<String> messageList; | ||
|
|
||
| private ChatModel model; | ||
|
|
||
| public void initialize() { | ||
|
|
||
| model = new ChatModel(); | ||
|
|
||
|
|
||
| messageList.setItems(model.getMessages()); | ||
|
|
||
| //Sätt upp knapp och enter-tangent | ||
| sendButton.setOnAction(e -> sendMessage()); | ||
| inputField.setOnAction(e -> sendMessage()); | ||
| } | ||
|
|
||
| private void sendMessage() { | ||
| String text = inputField.getText(); | ||
| if (text != null && !text.isBlank()) { | ||
| model.addMessage(text); | ||
| inputField.clear(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| package com.example.chat; | ||
|
|
||
| import javafx.application.Platform; | ||
| import javafx.collections.FXCollections; | ||
| import javafx.collections.ObservableList; | ||
|
|
||
| import java.net.URI; | ||
| import java.net.http.HttpClient; | ||
| import java.net.http.HttpRequest; | ||
| import java.net.http.HttpResponse; | ||
| import java.nio.charset.StandardCharsets; | ||
|
|
||
| public class ChatModel { | ||
|
|
||
| private final ObservableList<String> messages = FXCollections.observableArrayList(); | ||
| private final HttpClient client = HttpClient.newHttpClient(); | ||
| private final String ntfyUrl; | ||
|
|
||
| public ChatModel() { | ||
| String url = System.getenv("NTFY_URL"); | ||
| if (url == null || url.isBlank()) { | ||
| url = "http://localhost:8080/topic/test"; // fallback | ||
| } | ||
| ntfyUrl = url; | ||
| startListening(); | ||
| } | ||
|
|
||
| public ObservableList<String> getMessages() { | ||
| return messages; | ||
| } | ||
|
|
||
| public void addMessage(String message) { | ||
| if (message != null && !message.isBlank()) { | ||
| messages.add("Me: " + message); | ||
| sendToBackend(message); | ||
| } | ||
| } | ||
|
|
||
| private void sendToBackend(String message) { | ||
| new Thread(() -> { | ||
| try { | ||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(ntfyUrl)) | ||
| .POST(HttpRequest.BodyPublishers.ofString(message, StandardCharsets.UTF_8)) | ||
| .header("Content-Type", "text/plain") | ||
| .build(); | ||
|
|
||
| HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
| if (response.statusCode() >= 400) { | ||
| Platform.runLater(() -> messages.add("Error sending message: " + response.statusCode())); | ||
| } | ||
| } catch (Exception e) { | ||
| Platform.runLater(() -> messages.add("Error sending message: " + e.getMessage())); | ||
| } | ||
| }).start(); | ||
| } | ||
|
|
||
| private void startListening() { | ||
| new Thread(() -> { | ||
| try { | ||
| while (true) { | ||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(ntfyUrl + "/events")) | ||
| .build(); | ||
|
|
||
| client.send(request, HttpResponse.BodyHandlers.ofLines()) | ||
| .body() | ||
| .forEach(line -> { | ||
| if (line != null && !line.isBlank()) { | ||
| // Ta bort "data:" om det finns | ||
| String content = line.contains("data:") ? line.replaceFirst("data:", "").trim() : line; | ||
| if (!content.isEmpty()) { | ||
| Platform.runLater(() -> messages.add("Other: " + content)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Thread.sleep(1000); // poll varje sekund | ||
| } | ||
| } catch (Exception e) { | ||
| Platform.runLater(() -> messages.add("Error receiving messages: " + e.getMessage())); | ||
| } | ||
| }).start(); | ||
| } | ||
|
Comment on lines
+19
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainThread lifecycle and constructor side effects in Right now:
Implications:
At minimum, I’d recommend:
For example, to mark threads as daemon: private void sendToBackend(String message) {
- new Thread(() -> {
+ Thread thread = new Thread(() -> {
try {
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(ntfyUrl))
.POST(HttpRequest.BodyPublishers.ofString(message, StandardCharsets.UTF_8))
.header("Content-Type", "text/plain")
.build();
HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
if (response.statusCode() >= 400) {
Platform.runLater(() -> messages.add("Error sending message: " + response.statusCode()));
}
} catch (Exception e) {
Platform.runLater(() -> messages.add("Error sending message: " + e.getMessage()));
}
- }).start();
+ });
+ thread.setDaemon(true);
+ thread.start();
}
private void startListening() {
- new Thread(() -> {
+ Thread thread = new Thread(() -> {
try {
while (true) {
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(ntfyUrl + "/events"))
.build();
client.send(request, HttpResponse.BodyHandlers.ofLines())
.body()
.forEach(line -> {
if (line != null && !line.isBlank()) {
// Ta bort "data:" om det finns
String content = line.contains("data:") ? line.replaceFirst("data:", "").trim() : line;
if (!content.isEmpty()) {
Platform.runLater(() -> messages.add("Other: " + content));
}
}
});
Thread.sleep(1000); // poll varje sekund
}
} catch (Exception e) {
Platform.runLater(() -> messages.add("Error receiving messages: " + e.getMessage()));
}
- }).start();
+ });
+ thread.setDaemon(true);
+ thread.start();
}You can then later extend this with a proper shutdown/interrupt strategy if needed. Please verify the app actually terminates cleanly after closing the window, and confirm tests don’t hang or produce background thread errors when Length of output: 3157 🏁 Script executed: #!/bin/bash
# Search for test files referencing ChatModel
fd -e java -path '*/test/*' -exec grep -l 'ChatModel' {} \;Length of output: 168 🏁 Script executed: #!/bin/bash
# Search for ChatModel instantiation throughout codebase
rg -n 'new ChatModel\|ChatModel\s*\(\s*\)' --type=java -C3Length of output: 0 🏁 Script executed: #!/bin/bash
# Check for shutdown hooks or Application lifecycle methods
rg -n 'setOnCloseRequest|shutdown|stop\(\)' --type=java -C2Length of output: 0 Length of output: 170 🏁 Script executed: #!/bin/bash
# Search for Application.java, main, or JavaFX app entry points
rg -n 'class.*extends Application|public static void main' --type=java -C3Length of output: 1718 🏁 Script executed: #!/bin/bash
# Broader search: any file that references ChatModel
rg -n 'ChatModel' --type=java -B2 -A2Length of output: 2880 🏁 Script executed: #!/bin/bash
# Check if any shutdown/interrupt/close logic exists
rg -n 'Thread\.|interrupt|daemon' --type=java -C2Length of output: 392 Non-daemon threads and constructor side effects prevent clean shutdown and break tests The concerns are confirmed.
Required fixes:
Apply the suggested diff to both 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| module hellofx { | ||
| requires javafx.controls; | ||
| requires javafx.fxml; | ||
| requires java.net.http; // tillagd | ||
|
|
||
| opens com.example.chat to javafx.fxml; | ||
| exports com.example.chat; | ||
| } | ||
|
|
||
| opens com.example to javafx.fxml; | ||
| exports com.example; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
|
|
||
| <?import javafx.scene.control.*?> | ||
| <?import javafx.scene.layout.*?> | ||
| <?import javafx.geometry.Insets?> | ||
|
|
||
| <VBox xmlns="http://javafx.com/javafx/25" xmlns:fx="http://javafx.com/fxml/1" | ||
| fx:controller="com.example.chat.ChatController" | ||
| spacing="10"> | ||
|
|
||
| <padding> | ||
| <Insets top="10" right="10" bottom="10" left="10"/> | ||
| </padding> | ||
|
|
||
| <ListView fx:id="messageList" prefHeight="400"/> | ||
|
|
||
| <HBox spacing="10"> | ||
| <TextField fx:id="inputField" HBox.hgrow="ALWAYS" promptText="Type your message..."/> | ||
| <Button fx:id="sendButton" text="Send"/> | ||
| </HBox> | ||
| </VBox> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package com.example.chat.model; | ||
|
|
||
| import com.example.chat.ChatModel; | ||
| import javafx.collections.ObservableList; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class ChatModelTest { | ||
|
|
||
| private ChatModel model; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| model = new ChatModel(); | ||
| } | ||
|
|
||
| @Test | ||
| void testAddMessageAddsToList() { | ||
| ObservableList<String> messages = model.getMessages(); | ||
| assertEquals(0, messages.size()); | ||
|
|
||
| model.addMessage("Hello"); | ||
| assertEquals(1, messages.size()); | ||
| assertEquals("Me: Hello", messages.get(0)); | ||
| } | ||
|
|
||
| @Test | ||
| void testAddMessageIgnoresEmpty() { | ||
| ObservableList<String> messages = model.getMessages(); | ||
| model.addMessage(""); | ||
| model.addMessage(" "); | ||
| assertEquals(0, messages.size()); | ||
| } | ||
| } | ||
|
Comment on lines
+1
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainTest class is tightly coupled to ChatModel’s networking/JavaFX behavior and may be mislocated
Consider:
Please verify the actual on‑disk path of this file and run the tests once 🏁 Script executed: #!/bin/bash
# Locate ChatModelTest.java and ChatModel.java in the repository
echo "=== Finding ChatModelTest.java ==="
fd -n "ChatModelTest\.java"
echo ""
echo "=== Finding ChatModel.java ==="
fd -n "ChatModel\.java"
echo ""
echo "=== Checking for build files ==="
fd -n "(pom\.xml|build\.gradle)"Length of output: 793 🏁 Script executed: #!/bin/bash
# Check directory structure for test locations
echo "=== Directory structure for test sources ==="
find . -type d -name "test" 2>/dev/null | head -20
echo ""
echo "=== Directory structure for resources ==="
find . -type d -name "resources" 2>/dev/null | head -20Length of output: 140 🏁 Script executed: #!/bin/bash
# If ChatModel.java exists, inspect its content for networking and JavaFX behavior
CHATMODEL=$(fd -n "ChatModel\.java" | grep -v Test | head -1)
if [ -n "$CHATMODEL" ]; then
echo "=== ChatModel.java implementation ==="
echo "File: $CHATMODEL"
echo ""
cat "$CHATMODEL"
else
echo "ChatModel.java not found"
fiLength of output: 255 🏁 Script executed: #!/bin/bash
# Locate ChatModelTest.java and ChatModel.java with correct fd syntax
echo "=== Finding ChatModelTest.java ==="
fd "ChatModelTest\.java"
echo ""
echo "=== Finding ChatModel.java ==="
fd "ChatModel\.java"
echo ""
echo "=== Checking for build files ==="
fd "(pom\.xml|build\.gradle)"Length of output: 220 🏁 Script executed: #!/bin/bash
# Inspect ChatModel.java implementation
echo "=== ChatModel.java implementation ==="
CHATMODEL=$(find . -name "ChatModel.java" -not -name "*Test*" | head -1)
if [ -n "$CHATMODEL" ]; then
echo "File: $CHATMODEL"
echo ""
cat "$CHATMODEL"
else
echo "ChatModel.java not found"
fiLength of output: 3250 🏁 Script executed: #!/bin/bash
# Check the exact path of ChatModelTest.java in the test directory
echo "=== Listing files in src/test ==="
find src/test -type f -name "*.java" 2>/dev/null
echo ""
echo "=== Full path of ChatModelTest.java ==="
find . -path "*/src/test*" -name "ChatModelTest.java" 2>/dev/nullLength of output: 208 Move test to src/test/java and refactor ChatModel to support unit testing without network dependencies The review comment is accurate. Two critical issues require fixing:
Fixes required:
🤖 Prompt for AI Agents |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded Windows platform breaks cross-platform builds.
The
javafx.platformproperty is hardcoded towin, which means developers using Linux or macOS systems will be unable to build or run the project. This is a critical blocker for a classroom project where students likely use different operating systems.Consider using platform detection via Maven profiles or a build plugin to automatically determine the platform. Here's a common approach using profiles:
Alternatively, use the
os-maven-pluginto automatically detect the platform at build time.🤖 Prompt for AI Agents