Skip to content
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

JavaFX Incubator Modules #1375

Closed
wants to merge 38 commits into from

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 21, 2024

This PR contains a proposal for JavaFX Incubator Modules, along with an example of how such a module might be implemented.

NOTE: This PR will remain in Draft. It is intended only as supporting material for the proposal.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1375/head:pull/1375
$ git checkout pull/1375

Update a local copy of the PR:
$ git checkout pull/1375
$ git pull https://git.openjdk.org/jfx.git pull/1375/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1375

View PR using the GUI difftool:
$ git pr show -t 1375

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1375.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 21, 2024

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@kevinrushforth
Copy link
Member Author

I pushed the following updates:

  • Added a utility to print an incubator warning
  • Updated the Incubator Modules JEP:
    • Change javafx.incubator to jfx.incubator
    • Say that a JEP is needed to finalize or drop an incubating feature
    • Clarify that a feature should not incubate indefinitely
  • Updated the instructions to create a new incubator module. As part of this, I marked the changes in the JavaFX core (e.g., in build.gradle) with one of the following two comments:
    • //TODO: incubator dependency : Changes that will be needed in the core in support of incubator modules in general, which include the new utlity class and many of the changes in build.gradle; these changes will need go into mainline (via a separate enhancement) ahead of any specific incubator module
    • //TODO: incubator template : Identifies modifications needed in the core for a specific incubator module; these changes would be part of a PR for that specific incubator

private static final Module MODULE_JAVA_BASE = Module.class.getModule();

@SuppressWarnings("removal")
public static void incubatorWarning() {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we can add a javadoc to this method explaining why it is here and giving a usage example?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

@andy-goryachev-oracle
Copy link
Contributor

Incubator warning works as expected:

WARNING: Using incubator modules: jfx.incubator.richtext

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@andy-goryachev-oracle
Copy link
Contributor

BTW, should we have a separate, real PR with changes only for enabling the functionality (.md file and build.gradle, the latter with sample module changes commented out - maybe with an easy to grep marker for anyone who wants to add a new incubator module)?

@kevinrushforth
Copy link
Member Author

BTW, should we have a separate, real PR with changes only for enabling the functionality (.md file and build.gradle, the latter with sample module changes commented out - maybe with an easy to grep marker for anyone who wants to add a new incubator module)?

Yes, something along those lines is in the works.

I've already identified many of the changes that would be part of this separate Enhancement, and marked them with //TODO: incubator dependency -- see the above comment.

I'm not sure yet what the best way is to show where to add the logic for a new incubator module. At least having an easy-to-find "incubator modules go here" comment would be helpful and seems like a good idea. As you suggest, I could go farther than that and add commented-out build logic for the sample module...need to think about that one. I would not be in favor of including the sample module, though, so I'm leaning towards just adding the appropriate markers in build.gradle, but not any actual build logic for an incubator modules. Anyone doing an incubator module would need to grab the sample patch anyway.

'graphics',
'controls',
// TODO: incubator template
'incubator.myfeature',
Copy link
Contributor

Choose a reason for hiding this comment

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

this change fixed the jenkins build of the rich text area incubator, thank you!

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 27, 2024

@kevinrushforth This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk
Copy link

openjdk bot commented Jul 26, 2024

⚠️ @kevinrushforth This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth force-pushed the javafx.incubator branch from 1f567a9 to 4bb1b59
now

I pushed the wrong merge commit, so corrected it before (I hope) anyone other than Skara noticed. I wouldn't do this except that this is a Draft PR that I don't intend to ever make rfr.

I plan to close this Draft PR soon anyway, and create a new PR from a clean branch.

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 21, 2024

@kevinrushforth This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@kevinrushforth
Copy link
Member Author

Keep alive

@kevinrushforth kevinrushforth mentioned this pull request Oct 29, 2024
3 tasks
@kevinrushforth
Copy link
Member Author

I am closing this PR in favor of #1617 (Draft PR for JavaFX Incubator Modules) and #1616 (PR to add support for JavaFX Incubator Modules)

@kevinrushforth kevinrushforth deleted the javafx.incubator branch October 29, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants