Open damithc opened 1 month ago
LGTM! Just a few nits
Grammar:
You then decide where to layout the props for every scene.
You then decide where to place the props for every scene.
There on, it’s just a matter of pulling the curtains on your masterpiece.
Thereafter, it’s just a matter of pulling the curtains on your masterpiece.
Punctuation:
This tutorial takes you through the steps of building a typical Java FX application, using a chatbot application called Duke, as a running example. Given below is what the end result can look like, if you follow this tutorial until the end.
This tutorial takes you through the steps of building a typical Java FX application, using a chatbot application called Duke as a running example. Given below is what the end result can look like if you follow this tutorial until the end:
Inconsistent whether the plural of Node
is Nodes
or Node
s.
Note: the rest of this page uses Node
s, but page 2 uses Node
. This inconsistency is seen in other elements as well (e.g. ImageView
). Also, part 5 just uses node and nodes, not in code form.
Grammar:
Can you have more than one
Stage
an application?
Can you have more than one Stage
in an application?
As customary, ...
As is customary, ...
Then, create an example DialogBox with a simple message.
Then, create an example DialogBox
with a simple message.
Grammar:
Follow the same for similar cases of using
getResourceAsStream
method in later parts of this tutorial.
Do the same for similar cases of using getResourceAsStream
method in later parts of this tutorial.
First, let's delete the following two lines that shows a dialog box by default.
First, let's delete the following two lines that show a dialog box by default:
Next, let's create an instance of this Duke object in the
Main
(to be used for generating responses to user input).
Next, let's create an instance of this Duke object in the Main
method (to be used for generating responses to user input).
Add padding between each DialogBox Add padding between each ImageView and its Label Clip the ImageView into a circle
Add padding between each DialogBox
Add padding between each ImageView
and its Label
Clip the ImageView
into a circle
For example consider the following FXML snippet that defines a TextField similar to the one that we programmatically defined previous in part 2.:
For example consider the following FXML snippet that defines a TextField
similar to the one that we programmatically defined previous in part 2:
Once we switch over to using FXML, we can use a tool such as Scene Builder (a tool developed by Oracle and currently maintained by Gluon) to inspect and tweak the GUI design in a WYSIWYG) fashion.
Once we switch over to using FXML, we can use a tool such as Scene Builder (a tool developed by Oracle and currently maintained by Gluon) to inspect and tweak the GUI design in a WYSIWYG fashion.
Grammar:
Without setting
minHeight
to -Infinity, text overrun may occur when the text to be displayed exceeds the size of the label, causing the text to not be fully displayed and ending with...
instead.
Without setting minHeight
to -Infinity, text overrun may occur when the text to be displayed exceeds the size of the label, causing the text to not be fully displayed and end with ...
instead.
When we create a new instance of
DialogBox
(i.e., in theDialogBox
constructor), we set both the controller and root Node toDialogBox
.
When we create a new instance of DialogBox
(i.e., in the DialogBox
constructor), we set both the controller and root Node
to DialogBox
.
Only part 5 shows last updated
Grammar:
As you may have noticed, some of the elements does not automatically follow the dimension of the app when resizing:
As you may have noticed, some of the elements do not automatically follow the dimension of the app when resizing:
Send Button
does not follow to bottom right of the appScrollPane
does not automatically resize both horizontally and vertically
Send Button
does not follow the bottom right of the app
ScrollPane
does not automatically resize either horizontally or vertically
To rectify this, we can use a mixture of the following two techniques:
To rectify this, we can use a combination of the following two techniques:
Grammar:
Anchor
TextField
to the bottom while also automatically resize horizontally
Anchor TextField
to the bottom while also automatically resizing horizontally
Grammar:
At the same time we enable Fit To Width so that the content within the
ScrollPane
resize as well.
At the same time we enable Fit To Width so that the contents within the ScrollPane
resize as well.
Labels, TextBoxes, Buttons, more or less all the nodes used so far are subclasses of Region, and hence share the following CSS properties for borders:
Labels
, TextBoxes
, Buttons
, more or less all the nodes used so far are subclasses of Region
, and hence share the following CSS properties for borders:
Using a ladder to change color based on background Using derive to get a brighter or darker version of selected color
Using a ladder to change color based on the background Using derive to get a brighter or darker version of a selected color
This works for TextFields/Labels/Images as well.
This works for TextFields
/Labels
/Images
as well.
:focused
, when the node is selected (e.g., selecting the TextField)
:focused
, when the node is selected (e.g., selecting the TextField
)
Since an ImageView is a node, we can play around with the size, opacity, rotation, and more. This also means that the following properties can also be used for other kind of nodes.
Since an ImageView
is a node, we can play around with the size, opacity, rotation, and more. This also means that the following properties can also be used for other kind of nodes.
Create your own style class manually
Grammar:
Create style classes for each type of commands in
dialog-box.css
.
Create style classes for each type of command in dialog-box.css
.
We can set an image using CSS.
We can set a background image using CSS.
Grammar:
Pictures repeating on both axis.
Pictures repeating on both axes.
Set GIF as profile pictures
Set profile pictures as GIFs
Other general comments:
:
or .
after a sentence with "as shown below" or "as follows".Otherwise, LGTM!
@jedkohjk thanks very much for the detailed feedback. If you have time, please review https://github.com/se-edu/guides/pull/76 as well.
Question: Part 1
The given guide link just mentioned some examples, is it necessary that we ask them to select a particular version of JDK version?
Part 1
./gradlew clean build
does not work until I commented out this line of code. Commenting this line out had made ./gradlew clean build
successful.
Part 1
Launcher
would be clearer to students. For example, run Launcher
by executing ./gradlew run
.
Part 2
Part 3
user
is changed to userImage
in one of the PR's if I remembered correctly.
This are all the issues I have found, no major issues currently.
- I am not sure whether we should just leave it as jdk-11 instead of other jdk versions
./gradlew clean build
does not work until I commented out this line of code. Commenting this line out had made./gradlew clean build
successful.
@jyue487 good catch. This appears to be a stray file. Removed.
@jyue487 Thanks for the review. I've fixed problems 1,3,4,5 as well.
Ran through the tutorial up to Part 4 and the code works well. Here are some minor issues:
Part 2.
`Scrollpane` contains one node, whereas a `Pane` can contain multiple nodes.
Scrollpane
to ScrollPane
Part 3.
In part 3, we achieved the desired layout.
In part 2, we achieved the desired layout.
dialogContainer.getChildren().addAll(new DialogBox(userInput.getText(), user));
dialogContainer.getChildren().addAll(new DialogBox(userInput.getText(), userImage));
On top of showing what the user sent, we need to take the response generated my the program and pass it to the UI components.
On top of showing what the user sent, we need to take the response generated by the program and pass it to the UI components.
Part 4.
text overrun may occur when the text to be displayed exceeds the size of the label, causing the text to not be fully displayed and ending with `...` instead:
text overrun may occur when the text to be displayed exceeds the size of the label, causing the text to not be fully displayed and ends with ...
instead:
The issues mentioned above are fixed in PR #78. The other two below may require further review.
Part 2.
Then, create an example DialogBox with a simple message.
Would it be clearer to say Then, create a sample DialogBox with a simple message
instead.
Part 4.
Let's return to Duke and convert it to use FXML instead, with give Scene Builder a try as well.
The second part of the sentence seems unclear. Can it be we will give Scene Builder a try as well
?
@JiaXinEu
Would it be clearer to say
Then, create a sample DialogBox with a simple message
instead.
Sure. We can change it in that way.
The second part of the sentence seems unclear. Can it be
we will give Scene Builder a try as well
?
Let's return to Duke and convert it to use FXML instead, and give Scene Builder a try as well.
?
Your suggestion is fine too.
Thank you for the review @damithc.
Let's return to Duke and convert it to use FXML instead, and give Scene Builder a try as well.
This sounds great. Committed the changes into PR #78
Ran through the full tutorial other than some sections in Part 5. Here are just a few minor issues/nits:
Last code block on the page (Main.java
)
String dukeText = getResponse(userInput.getText());
String dukeText = duke.getResponse(userInput.getText());
Personally, the word "stuff" feels somewhat informal or casual in a tutorial. Could possibly consider using "aspects", "elements" or "components" instead.
Grammar:
Therefore, feel free to only look at the sections that interests you.
Therefore, feel free to only look at the sections that interest you.
The spacing between the points look off. Additionally, inconsistent use of bullet points or numbering across the tutorial parts.
Formatting of terms like "send button" and " enter key" are inconsistent.
Grammar:
does not follow to bottom right of the app
does not follow to the bottom right of the app
Otherwise, LGTM!
Thanks for the review @macareonie
Enter is a key in the keyboard while Send
is a button in the GUI (hence, different formatting). Fixed the rest. Pls check and see if I missed anything.
Thanks for the clarification @damithc
Just looked through the changes and it LGTM!
OS: Ubuntu 22.04.4 LTS JDK: OpenJDK 17 IDE: IntelliJ IDEA Ultimate 2024.1.4
Part 1:
I noticed that when first creating Main.java
after editing the build.gradle
file, the IDE will highlight many unresolved symbols
these do not go away on using ./gradlew clean build
, but do when clicking on the refresh icon at the top right of the screenshot (I assume this loads in the new dependencies). I don't really think it's a problem, but would be helpful to keep in mind in case students ask about this error.
Initially I could not use ./gradlew
and had to run chmod +x gradlew
. Is this intended? Could be an OS specific issue?
For reference, on my OS the hello world app looks like this
Part 4:
2. Initially I could not use
./gradlew
and had to runchmod +x gradlew
. Is this intended? Could be an OS specific issue?
Good catch, @drustanyjt I changed permissions of that file. Can you clone again and try again?
@damithc seems to be fixed, I can use ./gradlew
without issues now.
I followed the tutorial through Parts 1 to 4, and here are some of my findings. The findings which I feel are more important have been marked with exclamation marks :exclamation:. Overall, the tutorial has been improved greatly from how I remembered it being like, it is much easier to follow now, not to mention some of the cool animations (like resizing of the GUI window in part 5). So good job to everyone involved! For part 5, I might look through it another day.
Should we consider introducing the JavaFX docs here, or providing a link to the label class? The docs are only introduced in the next part (Part 2), and students trying out these exercises might be a little lost as to how to interact with the label class.
There is a mismatch between the layout diagram and the mockup UI. In the diagram, The "ImageView" and "Label" should be pointing into a "Hbox" which points into the "Vbox". Not sure if this was omitted for conciseness.
At first glance everything appears to work perfectly... You'll notice that when the chat entries fill up beyond scroll pane's display area, ... the ScrollPane down.
change "scroll pane's" to ScrollPane
's
Verify that the scroll pane now scrolls as intended.
change "scroll pane" to ScrollPane
Now, we can update the
handleUserInput()
as follows, ...
Changing to either of these should be more grammatically correct
"Now, we can update handleUserInput()
as follows"
"Now, we can update the handleUserInput()
function as follows"
Problem 1: The
MainWindow
class attempts to do it all.
I think it should be Main
instead of MainWindow
to be consistent with previous parts. This can be quite confusing for students following the guide as this is the first ever mention of MainWindow
For a better flow, I think it might be better if the image of the expanded controller panel should be moved to immediately after
Let’s now open DialogBox.fxml in Scene Builder (open it the same way you opened the previous file), and expand the Controller panel on the left-side accordion.
Thanks for the inputs @billyhoce I've fixed some issues as suggested, due to urgency. We can revert if others disagree.
Part 1 -> Exercises
Should we consider introducing the JavaFX docs here, or providing a link to the label class? The docs are only introduced in the next part (Part 2), and students trying out these exercises might be a little lost as to how to interact with the label class.
Waiting for inputs from others.
Part 2 -> Designing the Layout ❗❗❗
There is a mismatch between the layout diagram and the mockup UI. In the diagram, The "ImageView" and "Label" should be pointing into a "Hbox" which points into the "Vbox". Not sure if this was omitted for conciseness.
Added HBox
. Please check.
Part 3 -> Iteration 1 – Echoing the user
At first glance everything appears to work perfectly... You'll notice that when the chat entries fill up beyond scroll pane's display area, ... the ScrollPane down.
change "scroll pane's" to
ScrollPane
'sVerify that the scroll pane now scrolls as intended.
change "scroll pane" to
ScrollPane
I think 'scroll pane' is used when referring to user POV while ScrollPane
refers to developer/Java POV. Nevertheless, I've changed as suggested, for consistency.
Part 3 -> Iteration 2 – Adding dialog boxes for Duke's response
Now, we can update the
handleUserInput()
as follows, ...Changing to either of these should be more grammatically correct "Now, we can update
handleUserInput()
as follows" "Now, we can update thehandleUserInput()
function as follows"
the
was there already. Added method
to read as Now, we can update the handleUserInput() method as follows
Part 4 (Introduction paragraph) ❗❗❗
Problem 1: The
MainWindow
class attempts to do it all.I think it should be
Main
instead ofMainWindow
to be consistent with previous parts. This can be quite confusing for students following the guide as this is the first ever mention ofMainWindow
Fixed.
Part 4 -> Using Scene Builder to tweak the GUI
For a better flow, I think it might be better if the image of the expanded controller panel should be moved to immediately after
Let’s now open DialogBox.fxml in Scene Builder (open it the same way you opened the previous file), and expand the Controller panel on the left-side accordion.
Done
A question: In https://se-education.org/guides/tutorials/javaFxPart4.html we say that minHeight="-Infinity"
prevents the dialog box from being clipped at a certain height. But this property is about the minimum height, so it is a bit puzzling (why minHeight? why negative infinity?). Intuitively, it feels like the maxHeight="Infinity"
should be the way to indicate "I do not want the dialog box to be limited to any height limit".
Any thoughts?
A question: In https://se-education.org/guides/tutorials/javaFxPart4.html we say that
minHeight="-Infinity"
prevents the dialog box from being clipped at a certain height. But this property is about the minimum height, so it is a bit puzzling (why minHeight? why negative infinity?). Intuitively, it feels like themaxHeight="Infinity"
should be the way to indicate "I do not want the dialog box to be limited to any height limit". Any thoughts?
The last comment on this forum might give some insight. I'm not sure if there is a workaround though.
TLDR: -Infinity
is used as a constant to tell the label to set minHeight
as the preferred height (Which is likely the length of the text in the label). Without specifying this, the minHeight property is set by default to "USE_COMPUTED_SIZE" which might not necessarily give us the text in label.
The last comment on this forum might give some insight. I'm not sure if there is a workaround though.
TLDR:
-Infinity
is used as a constant to tell the label to setminHeight
as the preferred height (Which is likely the length of the text in the label). Without specifying this, the minHeight property is set by default to "USE_COMPUTED_SIZE" which might not necessarily give us the text in label.
I see. Should be fine, then. Thanks for looking into this, @billyhoce
Please have a look at the significantly-revamped Java FX tutorial here and report any problems or suggestions as separate issues in this issue tracker. If you found no issues, you can reply in this thread so that your effort doesn't go unnoticed.
Strongly suggested to follow the tutorial as if you are a student (i.e., actually do the steps given) so that we can tease out hidden issues.