search

se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 426 forks source link

Upgrade JavaFX to ver 17 #199

Closed Eclipse-Dominator closed 1 year ago

Eclipse-Dominator commented 1 year ago

Fixes #193 Fixes #108

This is an alternative PR to #196 which explores adding an extra log message to inform the user to ignore the warning message while upgrading to an LTS version.

The application uses JavaFX 11 which is reaching its EOL in September

  1. Thus it is better to upgrade JavaFX to the next LTS version which is version 17.

However, after version 16, JavaFX now throws a warning when JavaFX modules are loaded from the classpath. As there is currently no plan to migrate to using Modules in AB3, let's include an additional log message to inform the users to ignore the warning message.

image image
canihasreview[bot] commented 1 year ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

damithc commented 1 year ago

@Eclipse-Dominator also post a screenshot of how the log output looks.

canihasreview[bot] commented 1 year ago

v1

@Eclipse-Dominator submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/199/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v2

@Eclipse-Dominator submitted v2 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/199/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
codecov[bot] commented 1 year ago

Codecov Report

Merging #199 (7e3fb3b) into master (a976ec9) will decrease coverage by 0.12%. The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #199      +/-   ##
============================================
- Coverage     74.03%   73.91%   -0.12%     
  Complexity      423      423              
============================================
  Files            71       71              
  Lines          1290     1292       +2     
  Branches        127      127              
============================================
  Hits            955      955              
- Misses          301      303       +2     
  Partials         34       34
Files Changed Coverage Δ
src/main/java/seedu/address/Main.java 0.00% <0.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

canihasreview[bot] commented 1 year ago

v3

@Eclipse-Dominator submitted v3 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/199/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v3

@Eclipse-Dominator submitted v3 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/199/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

@se-edu/tech-team-level1 for your review ...

markusyeo commented 1 year ago

LGTM!

In addition to @chia-yh 's catch

I did a quick search of other instances of http://javafx.com/javafx/ appearing in the fxml files and noticed that there are other namespaces still pointing at http://javafx.com/javafx/8.

I think we might need to look at updating all of them to be prudent, or consider removing the version too

Eclipse-Dominator commented 1 year ago

There's a warning that need to resolve. image

Additionally, all .fxml files in resources\view should be updated to the latest version.

I included a separate info log to tell the users to discard this warning. It seems intellij filters away info logs so I updated the logger to use warning instead and it will appear in intellij as such:

image

canihasreview[bot] commented 1 year ago

v4

@Eclipse-Dominator submitted v4 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v3 and v4) (:chart_with_upwards_trend: Range-Diff between v3 and v4)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/199/4/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v5

@Eclipse-Dominator submitted v5 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v4 and v5) (:chart_with_upwards_trend: Range-Diff between v4 and v5)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/199/5/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
SPWwj commented 1 year ago

I included a separate info log to tell the users to discard this warning. It seems intellij filters away info logs so I updated the logger to use warning instead and it will appear in intellij as such:

I'm unsure if we can resolve this warning. This solution look like a last resort.

hansstanley commented 1 year ago

Small issue since it's a test file, but for consistency should the xmlns and xmlns:fx of validFileWithFxRoot.fxml be updated too? Currently it does not specify version:

<fx:root type="seedu.address.ui.TestFxmlObject" xmlns="http://javafx.com/javafx"
            xmlns:fx="http://javafx.com/fxml">
    <text>Hello World!</text>
</fx:root>
Eclipse-Dominator commented 1 year ago

I included a separate info log to tell the users to discard this warning. It seems intellij filters away info logs so I updated the logger to use warning instead and it will appear in intellij as such:

I'm unsure if we can resolve this warning. This solution look like a last resort.

The proper solution to resolve the issue is not very clean. It mainly involves migrating to use java modules which incur the following problems:

Alternatively, we can also manually suppress the warning by changing the JavaFX logger but it might not be ideal since using modules is the supposedly "correct" way of using JavaFX.

damithc commented 1 year ago

Small issue since it's a test file, but for consistency should the xmlns and xmlns:fx of validFileWithFxRoot.fxml be updated too? Currently it does not specify version:

<fx:root type="seedu.address.ui.TestFxmlObject" xmlns="http://javafx.com/javafx"
            xmlns:fx="http://javafx.com/fxml">
    <text>Hello World!</text>
</fx:root>

@Eclipse-Dominator any response to this comment?

SPWwj commented 1 year ago

I included a separate info log to tell the users to discard this warning. It seems intellij filters away info logs so I updated the logger to use warning instead and it will appear in intellij as such:

The warnings are issued by the JVM and are emitted before the Java code even starts to run. I think your current solution is effectively handle the situation, as the warning doesn't affect the application.

damithc commented 1 year ago

@Eclipse-Dominator any response to this comment?

@Eclipse-Dominator reminder ...

Eclipse-Dominator commented 1 year ago

Small issue since it's a test file, but for consistency should the xmlns and xmlns:fx of validFileWithFxRoot.fxml be updated too? Currently it does not specify version:

<fx:root type="seedu.address.ui.TestFxmlObject" xmlns="http://javafx.com/javafx"
            xmlns:fx="http://javafx.com/fxml">
    <text>Hello World!</text>
</fx:root>

@Eclipse-Dominator any response to this comment?

I have updated the format xmlns and xmlns:fx to follow the current project versions.

damithc commented 1 year ago

I have updated the format xmlns and xmlns:fx to follow the current project versions.

Did you post a new version (via CIHR) after this change?

Eclipse-Dominator commented 1 year ago

I have updated the format xmlns and xmlns:fx to follow the current project versions.

Did you post a new version (via CIHR) after this change?

I encountered some issues with CIHR. It appears that

PR not up to date with base (master) head. Rebase required.

even though the current upgrade-to-jfx-17 branch is already update to date with master

damithc commented 1 year ago

even though the current upgrade-to-jfx-17 branch is already update to date with master

@Eclipse-Dominator How did you update it? I suspect CIHR requires the branch to be synced with master via a rebase, not a merge.

damithc commented 1 year ago

@Eclipse-Dominator Also, update the master branch of your fork (in case that matters). It seems to be behind the upstream master.

Eclipse-Dominator commented 1 year ago

even though the current upgrade-to-jfx-17 branch is already update to date with master

@Eclipse-Dominator How did you update it? I suspect CIHR requires the branch to be synced with master via a rebase, not a merge.

@damithc It's done via a rebase and I can confirm that the current branch is up to date with upstream's master branch. I will try to do some force pushes to see if this issue can be resolved.

image

canihasreview[bot] commented 1 year ago

v6

@Eclipse-Dominator submitted v6 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v5 and v6) (:chart_with_upwards_trend: Range-Diff between v5 and v6)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/199/6/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.