5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors

[aptmac]

This PR addresses JMC-5904 [0], in which it is noted that org.openjdk.jmc.ui.celleditors could be merged with org.openjdk.jmc.rjmx.ui.celleditors instead of separating the code.

This is achieved by combining the celleditors code into a new package; org.openjdk.jmc.ui.celleditors. Originally the intent was to throw the jmc.ui.celleditors code into rjmx.ui.celleditors. however this gets complicated because jmc.ui.misc.FilterEditor (which requires CommonCellEditors) gets used in flightrecorder.ui. As a result, moving the celleditors code into rjmx.ui.celleditors would add a dependency on rjmx.ui from flightrecorder.ui, which is not ideal.

To alleviate this, FilterEditor has been moved to flightrecorder.ui (which is the only place it's used currently anyways), and celleditors have been moved to it's own package so it can be imported by rjmx.ui and jmc.ui (and subsequently flightrecorder.ui) as required. This removes the dependency from rjmx.ui to flightrecorder.ui.

EDIT (November 19th): below is the original concept for this PR; to merge jmc.ui.celleditors into rjmx.ui.celleditors. However, this creates a situation where rjmx.ui becomes a dependency of flightrecorder.ui, which is not ideal. For reference, I'll leave the original PR body below.

For the most part this is a fairly straightforward migration of code. There are three classes in jmc.ui.celleditors that needed to be moved into rjmx.ui.celleditors:

1. ClearableTextCellEditor
2. CommonCellEditors
3. NullableTextCellEditor

Once these files are moved there is a slight problem: org.openjdk.jmc.ui.misc.FilterEditor uses CommonCellEditors, and org.openjdk.jmc.rjmx.ui requires org.openjdk.jmc.ui. In this case, adding a dependency from org.openjdk.jmc.rjmx.ui.celleditors.CommonCellEditors to org.openjdk.jmc.ui.misc.FilterEditors would introduce a circular dependency.

The solution I've gone with here is to move FilterEditor to org.openjdk.jmc.flightrecorder.ui, which IMO makes sense anyways because it's closer to the code it gets consumed by. FilterEditor is used in:

• org.openjdk.jmc.flightrecorder.ui.common.DataPageToolkit
• org.openjdk.jmc.flightrecorder.ui.common.FilterComponent
• org.openjdk.jmc.flightrecorder.ui.common.LaneEditor
• org.openjdk.jmc.flightrecorder.ui.pages.ItemHandlerPage

Note: this issue was originally targeted for 8.1.0, but isn't really critical (it's a P4 task), so if desired we can hold off on integrating it now and keep this around for integration into 8.2.0 as discussed in the jmc bi-weekly calls

[0] https://bugs.openjdk.java.net/browse/JMC-5904

---

Progress

• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JMC-5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors

Reviewers

• Marcus Hirt (@thegreystone - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/271/head:pull/271
$ git checkout pull/271

Update a local copy of the PR:
$ git checkout pull/271
$ git pull https://git.openjdk.java.net/jmc pull/271/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 271

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/271.diff

[bridgekeeper]

👋 Welcome back aptmac! 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.

[mlbridge]

Webrevs

• 05: Full - Incremental (a510e72d)
• 04: Full - Incremental (347873cb)
• 03: Full (4829c15e)
• 02: Full - Incremental (6c0b8fa4)
• 01: Full (a2aad9eb)
• 00: Full (ab23d611)

[thegreystone]

I think it looks fairly straight forward, but at this point we're ramping down 8.1.0. Let's wait with integrating this until after we're done with 8.1.0.

[thegreystone]

It would be very nice if this dependency from rjmx.ui to flightrecorder.ui was not needed.

[aptmac]

I took another look into this, and I'm not quite sure where it makes sense to fix this.

This dependency happened because FilterEditor (currently in jmc.ui.misc) requires celleditors. FilterEditor is also only used in flightrecorder.ui.

rjmx.ui has a dependency on jmc.ui. If celleditors moves into rjmx.ui like this and the issue suggests, then FitlerEditor needs to move because it would rely on a cyclic dependency back onto rjmx.ui.

In this case, to have no introduced dependencies FilterEditor would need to move to a package that both imports rjmx.ui, and is imported by flightrecorder.ui.. which isn't any I believe. FilterEditor could be moved into something like jmc.console and add that dependency on flightrecorder.ui, but I'm not sure there's a happy path here.

In a different approach, celleditors could be consolidated into jmc.ui.celleditors instead and keep FilterEditor in jmc.ui.misc. This would require a rjmx.subscription.internal rjmx.services be made available to jmc.ui.celleditors, and a jmc.rjmx dependency added onto jmc.ui. I have a branch here that gives that a try: aptmac@90c54f2

[jiekang]

Would you be able to do a quick text diagram of the dependency chain? Like

a->b->c
d->c
...

Is there a parent org.openjdk.jmc.ui where any shared classes for rjmx.ui and flightrecorder.ui can go?

[aptmac]

Sure I'll give this a try.

So what I'm having trouble with here is that FilterEditor ultimately needs to be used by flightrecorder.ui.common

FilterEditor requires celleditors, which in HEAD is split between org.openjdk.jmc.ui.celleditors and org.openjdk.jmc.rjmx.ui. The only celleditors class that FilterEditor needs is CommonCellEditors which is in jmc.ui, so there's no dependency on rjmx.ui. rjmx.ui also has a dependency on jmc.ui.

So we have:

jmc.ui.celleditors.celleditors->jmc.ui.misc.FilterEditor->flightrecorder.ui.common
jmc.ui -> jmc.rjmx.ui

If we're consolidating celleditors into rjmx.ui, then we have:

jmc.rjmx.ui -> jmc.ui.misc.FilterEditor -> flightrecorder.ui.common
jmc.ui -> jmc.rjmx.ui

In this case, we can break the cycle by moving FilterEditor elsewhere else. In the approach here I placed it in flightrecorder.ui because that's ultimately where it gets used anyways. But either way I guess it'd go rjmx.ui.celleditors -> "something" -> flightrecorder.ui, so it's a matter of being a direct dependency or not.

If instead we consolidate celleditors into jmc.ui.celleditors, then we have:

jmc.ui -> jmc.rjmx.ui
jmc.rjmx.services + jmc.rjmx.subscription.internal -> jmc.ui.misc.FilterEditor -> flightrecorder.ui.common

because jmc.ui.celleditors would need the apporpriate files that rjmx.ui had access to from rjmx.

[jiekang]

So I do a -> b as in a depends on b, but I think you went the opposite is that right? I'm getting confused lol...

I think in the second case of consolidating celleditors into jmc.ui, a bunch of stuff appears that wasn't discussed before (rjmx.services, rjmx.subscription) Where are they coming from? Can they be moved to somewhere better?

[aptmac]

Er, yeah I guess I was thinking more of a flow diagram, I have it backwards here.

The reason it looks like most of the classes were in rjmx.ui anyways was because they use classes from rjmx. So if the classes are moved from rjmx.ui to jmc.ui, they'd still need access to those (specifically rjmx.services and subscription.internal), so we'd have to add a dependency on those to jmc.ui.

[jiekang]

Hm, okay. I took a step back and looked at the actual issue at hand.

I think it's more intended to migrate any cell editors from ooj.rjmx.ui to ooj.ui. The cell editors in ooj.rjmx.ui that have rjmx specific code (ie. import ooj.rjmx packages) should not be moved. (Should still be analyzed whether they can be modified and moved)

rjmx.ui is a specialized subset of ui for rjmx purposes, while ui is a general set for anybody to use (rjmx, flightrecorder, etc.). Why would we want to move things from ui to rjmx if they're not specialized for rjmx purposes? Especially as you note that there's already non-rjmx users of the things.

[thegreystone]

Here is some context for my comment.

Right now you could very well deliver a build of JMC that is only a JMX console. All you'd have to do was to have a few of the features bundled together (core, mbeanbrowser, console, blabla and leave out what you don't want, e.g. leaving out the joverflow and jfr features). Similarly you could package a new tool that is only a JFR tool, used exclusively to open flight recording files, and nothing else.

By requiring rjmx.ui to have a dependency on flightrecorder.ui, the dependency waters get muddied. For building a tool that uses rjmx.ui, you now always need to drag in the entire flightrecorder ui.

[aptmac]

I've rebased this PR, and shuffled the classes around again to remove the problematic dependencies.

Here the celleditors code has been extracted into it's own package (org.openjdk.jmc.ui.celleditors), which is used in rjmx.ui and flightrecorder.ui, and removes the need for a dependency between those two packages. I also updated the l10n strings to reflect these changes.

Previous commit:
rjmx.ui contains all the celleditors, flightrecorder.ui requires rjmx.ui

Updated commit:
jmc.ui.celleditors contains all the celleditors
rjmx.ui requires jmc.ui.celleditors
flightrecorder.ui requires jmc.ui.celleditors

[openjdk]

@aptmac This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors

Reviewed-by: hirt

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

• d218205: 7440: JMC fails on Apple M1

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[aptmac]

/integrate

[openjdk]

Going to push as commit 24f4cb0.
Since your change was applied there has been 1 commit pushed to the master branch:

• d218205: 7440: JMC fails on Apple M1

Your commit was automatically rebased without conflicts.

[openjdk]

@aptmac Pushed as commit 24f4cb0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.