Re: [Rev 02] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
> Issue : > https://bugs.openjdk.java.net/browse/JDK-8244418 > > Root Cause : > Incorrect assumption about menu list size. > > Fix : > Added check for empty menu list before trying to access it. > > Test : > Added a unit test that fails before fix and passes after it. Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: review fixes and test addition - Changes: - all: https://git.openjdk.java.net/jfx/pull/216/files - new: https://git.openjdk.java.net/jfx/pull/216/files/15c4de98..683849c0 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/216/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/216/webrev.01-02 Stats: 35 lines in 3 files changed: 25 ins; 4 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/216.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/216/head:pull/216 PR: https://git.openjdk.java.net/jfx/pull/216
Re: [Rev 02] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
On Fri, 15 May 2020 09:29:27 GMT, Jeanette Winzenburg wrote: >> 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. > > hmm .. yeah I'm aware of getContainer vs. getMenus - but they should be the > same size, shouldn't they? > > Anyway, if focusedIndex != getMenus().indexOf all users of focusedIndex have > to include a check for validity. That > might be prevented by not allowing it here in the method, in setting its > value not unconditionally to the given index > but guard it against being valid: > focusedMenuIndex = index >= getMenus().size() ? -1 : index > > Then focused is either valid or -1. Good suggestion on not setting the foucusedMenuIndex unconditionally. Also, we need to check for index < -1 : just to tighten up this method. I have added this check. - PR: https://git.openjdk.java.net/jfx/pull/216
Re: [Rev 01] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
On Sun, 17 May 2020 10:04:58 GMT, Jeanette Winzenburg
wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review_fixes
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java
> line 134:
>
>> 133: int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
>> 134: assertEquals(focusedIndex, -1);
>> 135: }
>
> the assert should be the other way round, expected value should be the first
> parameter :)
>
> didn't notice on first read and now only when writing a test case against our
> argument, c&p'ed the test as-is, replaced
> the requestFocus with simulating the notification from traversalListener and
> was confused about its value being -1
> Here's that modified test method (requires test api in MenuBarSkin and shim):
>
> @Test public void testSimulateTraverseIntoOnEmptyMenubar() {
> menuBar.setFocusTraversable(true);
>
> AnchorPane root = new AnchorPane();
> root.getChildren().add(menuBar);
> startApp(root);
>
> MenuBarSkin skin = (MenuBarSkin)menuBar.getSkin();
> assertTrue(skin != null);
>
> // simulate notification from traversalListener
> MenuBarSkinShim.setFocusedIndex(skin, 0);
> int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
> assertEquals(-1, focusedIndex);
> }
Corrected the similar asserts in this file.
Thanks for suggestion on test. Yes. At best, we can simulate this with Shim.
I have added it now.
-
PR: https://git.openjdk.java.net/jfx/pull/216
Re: [Rev 02] RFR: 8218973: SVG with masking is not rendering image with mask effect
On Sun, 10 May 2020 20:49:07 GMT, Bhawesh Choudhary
wrote:
>> Root cause of issue is Specifying a image mask from GraphicsContextJava.cpp
>> in WebKit was not implemented, so masking
>> doesn't take place at all while rendering SVGRect. to fix this issue add
>> implementation of function clipToImageBuffer()
>> in GraphicsContextJava.cpp and send clip image to
>> WCGraphicsPrismContext.java While rendering in
>> WCGraphicsPrismContext.java if image clip mask is available, use it for
>> rendering using MaskTextureGraphics interface
>> otherwise use usual way of rendering.
>
> Bhawesh Choudhary has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Moved Printing drawing path to non MaskTextureGraphics interface
modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/WCGraphicsPrismContext.java
line 560:
> 559: } else {
> 560: Screen screen = g.getAssociatedScreen();
> 561: FilterContext filterContext;
This logic is already present in getFilterContext(). You can call the function
instead.
-
PR: https://git.openjdk.java.net/jfx/pull/213
Re: [Rev 01] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
On Tue, 12 May 2020 12:27:11 GMT, Ajit Ghaisas wrote:
>> Issue :
>> https://bugs.openjdk.java.net/browse/JDK-8244418
>>
>> Root Cause :
>> Incorrect assumption about menu list size.
>>
>> Fix :
>> Added check for empty menu list before trying to access it.
>>
>> Test :
>> Added a unit test that fails before fix and passes after it.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review_fixes
modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java
line 134:
> 133: int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
> 134: assertEquals(focusedIndex, -1);
> 135: }
the assert should be the other way round, expected value should be the first
parameter :)
didn't notice on first read and now only when writing a test case against our
argument, c&p'ed the test as-is, replaced
the requestFocus with simulating the notification from traversalListener and
was confused about its value being -1
Here's that modified test method (requires test api in MenuBarSkin and shim):
@Test public void testSimulateTraverseIntoOnEmptyMenubar() {
menuBar.setFocusTraversable(true);
AnchorPane root = new AnchorPane();
root.getChildren().add(menuBar);
startApp(root);
MenuBarSkin skin = (MenuBarSkin)menuBar.getSkin();
assertTrue(skin != null);
// simulate notification from traversalListener
MenuBarSkinShim.setFocusedIndex(skin, 0);
int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
assertEquals(-1, focusedIndex);
}
-
PR: https://git.openjdk.java.net/jfx/pull/216
