On Wed, 13 May 2020 10:42:41 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>> This was the place where exception was occurring. Hence, I added this check. >> When I ran against the test, I still got the exception at caller lambda. I >> added that check later. >> Wanted to remove this check - but simple code scan revealed this method gets >> called from engine.addTraverseListener() >> listener with index = 0. Hence, I have kept this check. Covering this >> scenario in test seems tough. > > Hmm .. re-reading my comment: was assuming and then formulating round a > corner, sry. Will try again > > My assumption (from the method implementation before the fix) was that the > method expects only valid indices (either -1 > or an index in range) such that > assertEquals(focusedMenuIndex, getMenus().indexOf(focusedMenu); > > Which would leave the validity check to the caller. > > With that assumption, the old implementation were just fine. The one location > that calls it with an invalid index is > the one you fixed (not called if empty), the other is the traversalListener > (which I silently and incorrectly dropped > from my mind, because it's never notified anyway - which is nothing but a > bold guess ;) With the change, the > constraint doesn't hold (as it didn't before if the caller passed in an > invalid index) it just doesn't blow. Not sure > what - if any - bad might happen if we have a focusedMenuIndex >= > getMenu().size() (in particular empty menus). The > other way round: the method might take the responsibility to protect itself > (no precondition and the postcondition to > guarantee the constraint above). My preference would be to keep the method > as is, and change the callers to check for > a valid index. Hard to test, one way or other ;) I differ on this suggestion. My thinking is - list access in setFocusedMenuIndex() method should have this check. It is not up to the caller to know the internal details of the method. That's the root cause of Exception. I added another check in menuBarFocusedPropertyListener because, it accesses the different list - container.getChildren(). In general, I feel, the validity check near the list usage is logical and readable as well. ------------- PR: https://git.openjdk.java.net/jfx/pull/216
