RFR: 5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors [v6]
Marcus Hirt
hirt at openjdk.java.net
Thu Nov 25 09:57:07 UTC 2021
On Mon, 22 Nov 2021 21:06:57 GMT, Alex Macdonald <aptmac at openjdk.org> wrote:
>> This PR addresses JMC-5904 [[0]](https://bugs.openjdk.java.net/browse/JMC-5904), 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
>
> Alex Macdonald has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
> - move celleditors to org.openjdk.jmc.ui.celleditors
>
> This commit moves the celleditors code into it's own package: org.openjdk.jmc.ui.celleditors. This removes the dependency on rjmx.ui for flightrecorder.ui. Now, both rjmx.ui and flightrecorder.ui import org.openjdk.jmc.ui.celleditors. The l10n packages have also been updated with their respective strings.
> - update license headers
> - 5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors
Marked as reviewed by hirt (Lead).
-------------
PR: https://git.openjdk.java.net/jmc/pull/271
More information about the jmc-dev
mailing list