On Thu, 20 Jun 2024 17:59:13 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> You'll need to add an additional point after adding the one with duplicate
>> categories to see the exception in the console. Using the "Add Point" button
>> in the monkey tester after using the context menu entry should reproduce the
>> issue.
>> Regarding category hashtables, do you have any specific issues in mind, or
>> see any regressions due to this PR? Otherwise I'd suggest to create separate
>> issues / failing tests and do follow-up PRs.
>
> No, repeated invocation of the menu does not produce an exception. Maybe I
> am doing something wrong:
>
>
> void addDuplicateCategory() {
> var d = chart.getData();
> if (d.size() > 0) {
> var dd = d.get(0).getData();
> if (dd.size() > 0) {
> var v = dd.get(0);
> dd.add(new XYChart.Data(v.getXValue(),
> v.getYValue().doubleValue() + 1.0));
> }
> }
> }
>
>
> As for the hashtables, I was just surprised to see the issue not reproducible
> with the menu. It is a different scenario, and I am trying to understand why
> the difference.
>
> The reason for my comment is that the code that checks for the sizes looks
> suspicious on the grounds that, when things are accounted for correctly, it
> should be unnecessary. The current fix does look a bit artificial.
The second added Point specifically needs to be in another, later category for
this issue to occur - I created such a point in the monkey tester with the "Add
Point" button to the right, not by repeatedly using the new context menu. Maybe
try multiple times (depending on how the points random values are implemented
there?)
Regarding categories: My understanding is that `series.getData()` is supposed
to include all data, including duplicates. On the other hand,
`categoryAxis.getCategories()` only contains distinct categories. So the new
insertion index of the category of a (not necessarily duplicate) newly added
point needs to be shifted only if there are any previous duplicates with lower
indices. Thus, I think everything is accounted for correctly unless I'm missing
something.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1476#discussion_r1647976604
