Re: RFR: 8087980: Add property to disable Monocle cursor

[John Neffenger]

On Tue, 8 Oct 2019 12:03:42 GMT, Dell Green 
<12861109+dellgr...@users.noreply.github.com> wrote:

> Often on embedded systems a cursor is not a valid input modality. On some of 
> these systems, when the javafx toolkit initialises the native hardware 
> cursor, it produces artefacts which can be seen on screen (in the framebuffer 
> for example). This change adds a system property "monocle.cursor.enabled" 
> that can disable the creation of a native cursor in each of the Monocle 
> NativePlatform implementations in favour of a NullCursor which is a no-op 
> implementation of the NativeCursor abstract class that all native cursors 
> have to implement.
> 
> NullCursor class already existed and was being returned for some 
> implementations like AndroidPlatform and HeadlessPlatform. This change builds 
> upon that and conditionally returns NullCursor for all platforms.
> 
> A system property "monocle.debugcursor" has also been added to turn on 
> logging of which NativeCursor has been selected when the toolkit initialises.
> 
> 
> 
> Commits:
>  - cfbbc7dd: JDK-8087980: Add property to disable Monocle cursor
> 
> Changes: https://git.openjdk.java.net/jfx/pull/5/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/5/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8087980
>   Stats: 49 lines in 8 files changed: 40 ins; 0 del; 9 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/5.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/5/head:pull/5

modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/NativePlatform.java
 line 162:

> 161: final String name = cursor == null ? null : 
> cursor.getClass().getSimpleName();
> 162: System.err.println("Using native cursor: " + name);
> 163: }

Here you would call `PlatformLogger.fine` unconditionally, for example, instead 
of the conditional call to `System.err.println`.

Changes requested by jgn...@github.com (no OpenJDK username).

PR: https://git.openjdk.java.net/jfx/pull/5

Re: RFR: 8087980: Add property to disable Monocle cursor

[John Neffenger]

On Wed, 13 Nov 2019 22:04:36 GMT, Dell Green 
 wrote:

> On Wed, 13 Nov 2019 21:34:06 GMT, John Neffenger 
>  wrote:
> 
>> On Tue, 8 Oct 2019 12:03:42 GMT, Dell Green 
>> <12861109+dellgr...@users.noreply.github.com> wrote:
>> 
>>> Often on embedded systems a cursor is not a valid input modality. On some 
>>> of these systems, when the javafx toolkit initialises the native hardware 
>>> cursor, it produces artefacts which can be seen on screen (in the 
>>> framebuffer for example). This change adds a system property 
>>> "monocle.cursor.enabled" that can disable the creation of a native cursor 
>>> in each of the Monocle NativePlatform implementations in favour of a 
>>> NullCursor which is a no-op implementation of the NativeCursor abstract 
>>> class that all native cursors have to implement.
>>> 
>>> NullCursor class already existed and was being returned for some 
>>> implementations like AndroidPlatform and HeadlessPlatform. This change 
>>> builds upon that and conditionally returns NullCursor for all platforms.
>>> 
>>> A system property "monocle.debugcursor" has also been added to turn on 
>>> logging of which NativeCursor has been selected when the toolkit 
>>> initialises.
>>> 
>>> 
>>> 
>>> Commits:
>>>  - cfbbc7dd: JDK-8087980: Add property to disable Monocle cursor
>>> 
>>> Changes: https://git.openjdk.java.net/jfx/pull/5/files
>>>  Webrev: https://webrevs.openjdk.java.net/jfx/5/webrev.00
>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8087980
>>>   Stats: 49 lines in 8 files changed: 40 ins; 0 del; 9 mod
>>>   Patch: https://git.openjdk.java.net/jfx/pull/5.diff
>>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/5/head:pull/5
>> 
>> modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/NativePlatform.java
>>  line 50:
>> 
>>> 49: AccessController.doPrivileged((PrivilegedAction) () -> 
>>> {
>>> 50: final String str =
>>> 51: System.getProperty("monocle.debugcursor", "");
>> 
>> Just a nit, but why `monocle.debugcursor` rather than `monocle.cursor.debug` 
>> (my preference), or at least `monocle.debugCursor`? Below is the full list, 
>> for comparison, including the two added by this pull request.
>> 
>> monocle.cursor.enabled
>> monocle.debugcursor
>> monocle.epd.bitsPerPixel
>> monocle.epd.enableInversion
>> monocle.epd.forceMonochrome
>> monocle.epd.noWait
>> monocle.epd.rotate
>> monocle.epd.useDitheringY1
>> monocle.epd.useDitheringY4
>> monocle.epd.waveformMode
>> monocle.epd.y8inverted
>> monocle.input..flipXY
>> monocle.input..maxX
>> monocle.input..maxY
>> monocle.input..minX
>> monocle.input..minY
>> monocle.input..touchFilters
>> monocle.input.touchFilters
>> monocle.input.touchRadius
>> monocle.input.traceEvents
>> monocle.input.traceEvents.verbose
>> monocle.maliSignedStruct
>> monocle.platform
>> monocle.platform.traceConfig
>> monocle.screen.fb
>> monocle.stackSize
>> 
>> I'm nervous about our hidden API of system properties, and I'm as guilty as 
>> anyone with the nine properties I added for Monocle EPD. I think it might be 
>> okay as long as the code gets the property values only during class 
>> initialization. That should restrict their use to startup scripts and keep 
>> them out of application code trying to modify them *on-the-fly* at run time.
> 
> if i recall i originally started with the format you recommend as it made 
> more sense, and when looking for other debug logging across the javafx stack 
> I picked up on somewhat of a loose existing convention so changed to match 
> it. I guess it can be whatever everyone agrees upon. :)

On second thought, let's remove `monocle.debugcursor` and use a 
[PlatformLogger](https://github.com/openjdk/jfx/blob/master/modules/javafx.base/src/main/java/com/sun/javafx/logging/PlatformLogger.java).
 The JavaFX loggers are available from 
[Logging](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/javafx/util/Logging.java).
 In retrospect, that's how I managed to avoid any new *debug* properties for 
Monocle EPD even though it's packed with debugging and trace messages. For 
examples, see the variable `logger` in 
[EPDFrameBuffer](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/EPDFrameBuffer.java),
 where `logger.fine` is called for messages printed once per run, while 
`logger.finer` is called for messages printed once per rendered frame.

PR: https://git.openjdk.java.net/jfx/pull/5

Re: [Rev 01] RFR: 8234239: [TEST_BUG] Reenable few ignored web tests

[Kevin Rushforth]

On Fri, 15 Nov 2019 10:01:51 GMT, Arun Joseph  wrote:

> The pull request has been updated with additional changes.
> 
> 
> 
> Added commits:
>  - ca460353: Remove ignore imports and update copyright
> 
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/42/files
>   - new: https://git.openjdk.java.net/jfx/pull/42/files/f2f121d9..ca460353
> 
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/42/webrev.01
>  - incr: https://webrevs.openjdk.java.net/jfx/42/webrev.00-01
> 
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8234239
>   Stats: 6 lines in 3 files changed: 0 ins; 3 del; 3 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/42.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/42/head:pull/42

I added some questions and one requested change below.

modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java line 82:

> 81: }
> 82: 
> 83: @Test public void testGarbageCollectability() throws 
> InterruptedException {

RT-26710, aka [JDK-8088139](https://bugs.openjdk.java.net/browse/JDK-8088139), 
is still listed as open. Since these tests no longer hang, can you close 
[JDK-8088139](https://bugs.openjdk.java.net/browse/JDK-8088139) as "Cannot 
reproduce"?

modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java line 58:

> 57: 
> 58: @Test public void testOleg() throws InterruptedException{
> 59: final String URL = new 
> File("src/test/resources/test/html/guimark2-vector.html").toURI().toASCIIString();

This test takes 80 seconds (`16 * 5 * 1sec`) to run, which is too long for a 
simple unit test, when the entire rest of the test suite takes about 5 minutes. 
I recommend decreasing the duration of the key frame to 200 msec, which would 
allow the test to run in 16 seconds.

modules/javafx.web/src/test/java/test/javafx/scene/web/CallbackTest.java line 
90:

> 89: 
> 90: @Test public void testDefaultPopup() {
> 91: clear();

This test, along with `testCustomPopup` is listed as unstable. Have you run it 
multiple times on different systems?



Changes requested by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/42

Re: [Approved] RFR: 8234194: [TEST_BUG] Reenable few graphics unit tests

[Kevin Rushforth]

On Fri, 15 Nov 2019 10:33:16 GMT, Ambarish Rapte  wrote:

> The pull request has been updated with additional changes.
> 
> 
> 
> Added commits:
>  - e7856f6e: reenable some more tests and fix review comment
> 
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/39/files
>   - new: https://git.openjdk.java.net/jfx/pull/39/files/5df00884..e7856f6e
> 
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/39/webrev.01
>  - incr: https://webrevs.openjdk.java.net/jfx/39/webrev.00-01
> 
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8234194
>   Stats: 14 lines in 3 files changed: 6 ins; 3 del; 5 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/39.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/39/head:pull/39

Looks good.

modules/javafx.graphics/src/test/java/test/javafx/css/Node_cssStyleMap_Test.java
 line 102:

> 101: }
> 102: 
> 103: @Ignore("JDK-8234241")

Based on the description in JBS, I presume that 
[JDK-8234241](https://bugs.openjdk.java.net/browse/JDK-8234241) is a test bug? 
If so, can you label the new bug as such?



Approved by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/39

Re: [Approved] RFR: 8234150: Address ignored tests in ComboBoxTest, LabeledTest, HyperLinkTest and TextInputControlTest

[Kevin Rushforth]

On Thu, 14 Nov 2019 09:33:39 GMT, Ajit Ghaisas  wrote:

> This PR is to address ignored tests in ComboBoxTest, LabeledTest, 
> HyperLinkTest and TextInputControlTest.
> 
> strategy is as follows -
> 
> 1) Enable tests marked with @Ignore by removing that tag
> 2) Run the test
> 3) If test Passes - remove the @Ignore tag
> 4) If test fails - if test is invalid - remove it, else fix the test
> 5) In case if the failure cannot be fixed, leave the test ignored (keep 
> @Ignore tag) 
> 
> 
> With these corrections - here are the results:
> 
> Results BEFORE this FIX
> 1. ComboBoxTest -- tests:156, failures:0, ignored:4
> 2. LabeledTest -- tests:93, failures:0, ignored:5
> 3. HyperlinkTest -- tests:34, failures:0, ignored:3
> 4. TextInputControlTest -- tests:594, failures:0, ignored:12
> 
> Results AFTER this FIX
> 1. ComboBoxTest -- tests:156, failures:0, ignored:2
> 2. LabeledTest -- tests:93, failures:0, ignored:3
> 3. HyperlinkTest -- tests:31, failures:0, ignored:0
> 4. TextInputControlTest -- tests:585, failures:0, ignored:0
> 
> 
> 
> Commits:
>  - 6709837b: Fix-Remove-Keep @Ignore tests
> 
> Changes: https://git.openjdk.java.net/jfx/pull/36/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/36/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8234150
>   Stats: 63 lines in 4 files changed: 0 ins; 57 del; 6 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/36.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/36/head:pull/36

Approved by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/36

Re: Ad NashornScriptEngine (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

On 14.11.2019 22:57, Kevin Rushforth wrote:
> On 11/14/2019 10:12 AM, Rony G. Flatscher wrote:
>> On 14.11.2019 16:34, Rony G. Flatscher wrote:
>>> On 13.11.2019 19:50, Kevin Rushforth wrote:
 On 11/13/2019 9:42 AM, Rony G. Flatscher wrote:
>> ... cut ...
> To reproduce the testcase one would need ooRexx and the Java bridge 
> BSF4ooRexx (all
> opensource) for
> which I could come up with a zip-archive (assuming binaries within should 
> be 64-bit) and a
> script to
> set up the environment either for Windows, Linux or MacOS, whatever you 
> advise. Would that be
> o.k.?
 We prefer not to rely on third-party libraries for test cases. In any case 
 we would not be able to
 use that for a regression test / unit test.
>> If test units really seem to be important in this particular case, one 
>> possibility would be to
>> create a minimalistic ScriptEngine implementation in pure Java just for the 
>> sole purpose to allow
>> the creation of a test unit that is able to assert that FXMLLoader puts the 
>> ScriptEngine.ARGV and
>> ScriptEngine.FILENAME entries into the ENGINE_SCOPE Bindings. E.g. having 
>> the ScriptEngine's eval()
>> methods return the ScriptContext at invocation time in order to allow 
>> inspection of the Bindings.
>> This way it would become also possible to write in addition test units that 
>> also check whether all
>> FXML elements that carry a fx:id are really placed into the GLOBAL_SCOPE 
>> Bindings.
>
> Something like that seems reasonable, and would avoid a dependence on 
> Nashorn, which in addition
> to having all the problems you mentioned, is deprecated for removal.
>
>> However,
>
> Did you have something more to add?

No, sorry for that. Rewrote my e-mail and had sent it too early by mistake and 
without noticing.

Will study all the procedures and create a testcase to be submitted at 

as per your advice (and will report back under this thread once submitted). The 
testcase would use
an artificial ScriptEngine implementation that could be used for testunit 
testing as well. This
might take a while due to other obligations that I will have to meet during the 
next few days.

---rony

Re: Refire event while it is delivered is evil - always?!

[Jeanette Winzenburg]

*muttering to myself ..

Meanwhile, my understanding evolved a bit - there are actually two  
refiring scenarios, both wrecking the event dispatch sequence, the  
first  more evil than the second


1. getParent.fireEvent(receivedKeyEvent) which leads to serious  
misbehaviour (as reported f.i. in  
https://bugs.openjdk.java.net/browse/JDK-8207759 ) - fix understood,  
implemented and waiting for review


2. child.fireEvent(receivedKeyEvent) which leads to misbehavior (as  
reported in https://bugs.openjdk.java.net/browse/JDK-8229924 ) - still  
don't know how to fix it: the child will never get a keyEvent  
"naturally" because it's never focusOwner. So the parent must deliver  
it somehow .. Currently experimenting with custom implementation of  
buildEventDispatchChain (append the editor's chain? or a custom  
dispatcher doing so?)


More questions than answers ;)

-- Jeanette

Zitat von Jeanette Winzenburg :

While fixing https://bugs.openjdk.java.net/browse/JDK-8207759 it  
turned out that the underlying reason for the bug was a broken event  
dispatch sequence introduced by behavior.forwardToParent. Which is a  
call to parent.fireEvent with the event that was received. This  
builds a nested chain and delivers  the event to all handlers in  
that new chain - down and up again - _before_ the current chain is  
completed. Consequently, the consuming singleton handler for the  
same event is notified _after_ the scene-level handlers (in  
particular the accelerators) have seen and handled it.


Looks like it happens for any control (not only for a TextField as  
in the referenced issue, nor only for controls with a  
FakeFocusTextField which refire while processing keyEvents), as the  
example below demonstrates.


My current understanding of event dispatch is, that a  chain has a  
life-cycle consisting of separate (?) states:


- building the chain from eventTargets
- delivering the event along the dispatchers in the chain

There's a contract for dispatch sequence (like  capturing/bubbling  
phase, dispatch from specialized to super event types and other  
rules). That can be guaranteed as long as chain building and event  
delivering are separate phases, it seems to break down if they are  
mixed (there are other issues with a similar/same underlying reason,  
f.i. in all controls with a FakeFocusTextField).


Now the questions:

- is there any specification about not mixing the life-cycle states?  
if so, where?

- or is there a way to safely re-fire an event at the moment of receiving it?
- or maybe I got it all wrong, if so please guide me :)

-- Cheers, Jeanette


The example:

public class NestedEventDispatchChain extends Application {


private KeyCode key = DIGIT1;
private Parent createContent() {
Button button = new Button("the evil button!");
// re-firing handler
button.addEventHandler(KEY_PRESSED, e -> {
if (key == e.getCode()) {
System.out.println("before refire " + e);
button.getParent().fireEvent(e);
System.out.println("after refire " + e);
}
});

// consuming singleton handler
button.setOnKeyPressed(e -> {
if (key == e.getCode()) {
e.consume();
System.out.println("consumed in singleton " + e.getCode());
}
 });
BorderPane content = new BorderPane(button);
return content;
}

@Override
public void start(Stage stage) throws Exception {
Scene scene = new Scene(createContent());
// accelerator that shouldn't be triggered because singleton  
handler consumed
 
scene.getAccelerators().put(KeyCombination.keyCombination(key.getName()), ()  
-> {

System.out.println("accelerator triggered for " + key);
});
stage.setScene(scene);
stage.show();
}

public static void main(String[] args) {
launch(args);
}
}

Re: RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Kevin Rushforth]

On Fri, 15 Nov 2019 12:59:15 GMT, Jeanette Winzenburg  
wrote:

> On Wed, 13 Nov 2019 11:26:54 GMT, Hadzic Samir  wrote:
> 
>> On Thu, 7 Nov 2019 12:07:55 GMT, Jeanette Winzenburg  
>> wrote:
>> 
>>> On Fri, 1 Nov 2019 10:59:57 GMT, Hadzic Samir  wrote:
>>> 
 On Tue, 29 Oct 2019 13:19:27 GMT, Hadzic Samir  wrote:
 
> On Wed, 9 Oct 2019 16:01:38 GMT, Kevin Rushforth  wrote:
> 
>> On Wed, 9 Oct 2019 12:26:31 GMT, Hadzic Samir  
>> wrote:
>> 
>>> On Mon, 7 Oct 2019 10:22:11 GMT, Jeanette Winzenburg 
>>>  wrote:
>>> 
 On Fri, 4 Oct 2019 06:13:48 GMT, Hadzic Samir  
 wrote:
 
> Allright, this is a fix for JDK-8207957
> 
> 
> 
> Commits:
>  - 969ebb51: Fixing TableColumnHeaderTest
>  - 9d379619: Removing Tablecolumnbasehelper
>  - 4fe020fc: Fix javadoc for TableColumnHeader
>  - c422c80f: Minor modification and uni test added.
>  - b2bdfb5b: Change resizeColumn to protected without static in order 
> to be able to override
>  - 3b9b7903: Second option for resizeColumnToFitContent in 
> TableColumnHeader
>  - d91c56e5: First attempt to extract resizeColumnToFitContent
> 
> Changes: https://git.openjdk.java.net/jfx/pull/6/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/6/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>   Stats: 523 lines in 4 files changed: 330 ins; 187 del; 6 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/6.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/6/head:pull/6
 
 modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
  line 600:
 
> 599:  * expensive if the number of rows is large. Subclass can 
> either call this method or override it (no need to call
> 600:  * {@code super()}) to provide their custom algorithm.
> 601:  *
 
 see 
 https://github.com/javafxports/openjdk-jfx/pull/289#discussion_r245482213
  - I think I made a suggestion (that probably needs some native 
 speaker's fine tuning :)
>>> 
>>> Allright @kleopatra , I have indeed removed the TableColumn argument. 
>>> It is clearer now that we are resizing the TableColumn of the header.
>>> 
>>> I have updated the description a bit but really I'm really not good at 
>>> method description so I'm open to all suggestions..
>> 
>> A Draft CSR was filed here: 
>> [JDK-8215554](https://bugs.openjdk.java.net/browse/JDK-8215554). It will 
>> need to be updated once the API review is complete.
> 
> Do you think this looks good now @kleopatra @nlisker ?
 
 Yes I was waiting for the final 'go'. I will update it asap, thanks
 
 Le ven. 1 nov. 2019 à 09:49, Jeanette Winzenburg 
 a écrit :
 
> *@kleopatra* commented on this pull request.
> --
>
> In
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
> :
>
> > + * Resizes this {@code TableColumnHeader}'s column to fit the 
> > width of its content.
> + *
> + * @implSpec The resulting column width for this implementation is 
> the maximum of the preferred width of the header
> + * cell and the preferred width of the first {@code maxRow} cells.
> + * 
> + * Subclasses can either use this method or override it (without the 
> need to call {@code super()}) to provide their
> + * custom implementation (such as ones that exclude the header, 
> exclude {@code null} content, compute the minimum
> + * width etc.).
> + *
> + * @param maxRows the number of rows considered when resizing. If -1 
> is given, all rows are considered.
> + * @since 14
>
> looks good to me :) You would have to update the crs eventually, right?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
>
>>> 
>>> As to the test, it's a bit ... confusing, will try to suggest some changes 
>>> - take them with a grain of salt, though, because I'm still groping to find 
>>> my path through the wilderness here :)
>> 
>> I have changed the unit test according to @kleopatra review and also add two 
>> more unit test regarding the 

Re: To write tests or not to write tests ...

[Jeanette Winzenburg]

Zitat von Kevin Rushforth :

In general, I would say it's OK to rely on existing tests for  
trivial refactoring, that is a refactoring where it seems fairly  
obvious that there are not going to be changes in behavior (even if  
such tests may be inadequate). For less trivial refactoring, if  
there are clearly missing tests, I would think they should be added  
to ensure no unintended behavioral changes, although I wouldn't go  
overboard.




reasonable middle ground :)

Bug fixes and new features are a different matter. In the specific  
case you cite, there is new public API, so at the very least,  
testing the new API is needed. I haven't (yet) looked closely enough  
to know whether the refactoring is a possible source of concern that  
would warrant additional tests.




in that specific case, I don't see much of a risk - and now there are  
tests for all aspects of the behavior as specified by the new api (as  
far as it can go, IMO, testing actual sizes in the context of  
column/cell/header is .. tricky to impossible)


-- Jeanette


-- Kevin


On 11/8/2019 11:54 AM, Jeanette Winzenburg wrote:


... if changes are "just" a re-arrangement of code (like  
https://github.com/openjdk/jfx/pull/6 - 8207957: TableSkinUtils  
should not contain actual code implementation)?


In an ideal world, there would be a safety-net of tests (they would  
have been written at the time the old code was pushed - wouldn't  
they ;). All we would need to do is run them to ensure that our  
re-arrangement doesn't break anything.


In the real world and that particular pull request above, there are  
not tests (that I could find, maybe overlooked them).


Now the question: is the contributor responsible for writing those  
missing tests? The two extremes are


- yes, it's a good opportunity ;) And it is mandatory (probably?)  
for all public/protected api. A new protected method with  
specification was condensed and should be tested.
- no, nothing changed, the tests were always missing and all still  
live ;) so doing nothing is just fine


What's the procedure here?

-- Jeanette

Re: RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Jeanette Winzenburg]

On Wed, 13 Nov 2019 11:26:54 GMT, Hadzic Samir  wrote:

> On Thu, 7 Nov 2019 12:07:55 GMT, Jeanette Winzenburg  
> wrote:
> 
>> On Fri, 1 Nov 2019 10:59:57 GMT, Hadzic Samir  wrote:
>> 
>>> On Tue, 29 Oct 2019 13:19:27 GMT, Hadzic Samir  wrote:
>>> 
 On Wed, 9 Oct 2019 16:01:38 GMT, Kevin Rushforth  wrote:
 
> On Wed, 9 Oct 2019 12:26:31 GMT, Hadzic Samir  wrote:
> 
>> On Mon, 7 Oct 2019 10:22:11 GMT, Jeanette Winzenburg 
>>  wrote:
>> 
>>> On Fri, 4 Oct 2019 06:13:48 GMT, Hadzic Samir  
>>> wrote:
>>> 
 Allright, this is a fix for JDK-8207957
 
 
 
 Commits:
  - 969ebb51: Fixing TableColumnHeaderTest
  - 9d379619: Removing Tablecolumnbasehelper
  - 4fe020fc: Fix javadoc for TableColumnHeader
  - c422c80f: Minor modification and uni test added.
  - b2bdfb5b: Change resizeColumn to protected without static in order 
 to be able to override
  - 3b9b7903: Second option for resizeColumnToFitContent in 
 TableColumnHeader
  - d91c56e5: First attempt to extract resizeColumnToFitContent
 
 Changes: https://git.openjdk.java.net/jfx/pull/6/files
  Webrev: https://webrevs.openjdk.java.net/jfx/6/webrev.00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
   Stats: 523 lines in 4 files changed: 330 ins; 187 del; 6 mod
   Patch: https://git.openjdk.java.net/jfx/pull/6.diff
   Fetch: git fetch https://git.openjdk.java.net/jfx pull/6/head:pull/6
>>> 
>>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>>>  line 600:
>>> 
 599:  * expensive if the number of rows is large. Subclass can 
 either call this method or override it (no need to call
 600:  * {@code super()}) to provide their custom algorithm.
 601:  *
>>> 
>>> see 
>>> https://github.com/javafxports/openjdk-jfx/pull/289#discussion_r245482213
>>>  - I think I made a suggestion (that probably needs some native 
>>> speaker's fine tuning :)
>> 
>> Allright @kleopatra , I have indeed removed the TableColumn argument. It 
>> is clearer now that we are resizing the TableColumn of the header.
>> 
>> I have updated the description a bit but really I'm really not good at 
>> method description so I'm open to all suggestions..
> 
> A Draft CSR was filed here: 
> [JDK-8215554](https://bugs.openjdk.java.net/browse/JDK-8215554). It will 
> need to be updated once the API review is complete.
 
 Do you think this looks good now @kleopatra @nlisker ?
>>> 
>>> Yes I was waiting for the final 'go'. I will update it asap, thanks
>>> 
>>> Le ven. 1 nov. 2019 à 09:49, Jeanette Winzenburg 
>>> a écrit :
>>> 
 *@kleopatra* commented on this pull request.
 --

 In
 modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
 :

 > + * Resizes this {@code TableColumnHeader}'s column to fit the width 
 > of its content.
 + *
 + * @implSpec The resulting column width for this implementation is 
 the maximum of the preferred width of the header
 + * cell and the preferred width of the first {@code maxRow} cells.
 + * 
 + * Subclasses can either use this method or override it (without the 
 need to call {@code super()}) to provide their
 + * custom implementation (such as ones that exclude the header, 
 exclude {@code null} content, compute the minimum
 + * width etc.).
 + *
 + * @param maxRows the number of rows considered when resizing. If -1 
 is given, all rows are considered.
 + * @since 14

 looks good to me :) You would have to update the crs eventually, right?

 —
 You are receiving this because you authored the thread.
 Reply to this email directly, view it on GitHub
 ,
 or unsubscribe
 
 .

>> 
>> As to the test, it's a bit ... confusing, will try to suggest some changes - 
>> take them with a grain of salt, though, because I'm still groping to find my 
>> path through the wilderness here :)
> 
> I have changed the unit test according to @kleopatra review and also add two 
> more unit test regarding the headerContent and maxRows.

looks good to me - would approve if I could :)

PR: https://git.openjdk.java.net/jfx/pull/6

Re: [Integrated] RFR: 8234189: [TEST_BUG] Remove ignored and invalid graphics unit tests

[Ambarish Rapte]

Changeset: 3d0cb496
Author:Ambarish Rapte 
Date:  2019-11-15 10:53:31 +
URL:   https://git.openjdk.java.net/jfx/commit/3d0cb496

8234189: [TEST_BUG] Remove ignored and invalid graphics unit tests

Reviewed-by: kcr

! 
modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageLoaderScalingTest.java
- 
modules/javafx.graphics/src/test/java/test/javafx/scene/input/PasteboardTest.java
! 
modules/javafx.graphics/src/test/java/test/javafx/scene/input/TouchEventTest.java
! 
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundSizeTest.java

Re: RFR: 8234194: [TEST_BUG] Reenable few graphics unit tests

[Ambarish Rapte]

On Fri, 15 Nov 2019 10:34:58 GMT, Ambarish Rapte  wrote:

> On Thu, 14 Nov 2019 23:55:45 GMT, Kevin Rushforth  wrote:
> 
>> On Thu, 14 Nov 2019 19:56:16 GMT, Ambarish Rapte  wrote:
>> 
>>> Following graphics unit tests can be re-enabled.
>>> 
>>>  1. 
>>> test.com.sun.javafx.scene.layout.region.BackgroundRepeatConverterTest.scenario2
>>> RepeatStructConverter.convert() is an internal method and does not verify 
>>> the parameters. Passing null value to this method results in NPE.
>>> 
>>> 2. test.javafx.scene.layout.RegionCSSTest.borderImageWidth_auto
>>> test.javafx.scene.layout.RegionCSSTest.borderImageWidth_1_auto
>>> test.javafx.scene.layout.RegionCSSTest.borderImageWidth_1_2Percent_auto
>>> It is not certain if the value of auto for -fx-border-image-width is 1 or 2 
>>> by defaut. But the tests seem correct and they pass.
>>> 
>>> 3. test.javafx.scene.Node_cssMethods_Test
>>> [JDK-8094155](https://bugs.openjdk.java.net/browse/JDK-8094155) is fixed 
>>> and the tests can be re-enabled.
>>> 
>>> 
>>> 
>>> Commits:
>>>  - 5df00884: Reenable few of the graphics unit tests
>>> 
>>> Changes: https://git.openjdk.java.net/jfx/pull/39/files
>>>  Webrev: https://webrevs.openjdk.java.net/jfx/39/webrev.00
>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8234194
>>>   Stats: 12 lines in 3 files changed: 0 ins; 7 del; 5 mod
>>>   Patch: https://git.openjdk.java.net/jfx/pull/39.diff
>>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/39/head:pull/39
>> 
>> Most of this looks OK, but there is one I had a question on.
>> 
>> Have you run this on all platforms?
>> 
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/scene/layout/region/BackgroundRepeatConverterTest.java
>>  line 57:
>> 
>>> 56: @Test(expected=NullPointerException.class)
>>> 57: public void scenario2() {
>>> 58: ParsedValue[][] values = new 
>>> ParsedValueImpl[][] {
>> 
>> This is at odds with the body of the test, since presumably, the 
>> `assertEquals` will never be called.
>> 
>> An expect exception annotation is best used in the case where you do a 
>> single thing that is expected to generate that exception. Otherwise and 
>> unexpected exception could end up making the test look like it's passing.
> 
> Hi Kevin, the test corrected now.
> Removed `(expected=NullPointerException.class)` and added try, catch block.

> Have you run this on all platforms?
Yes, The change is verified on platforms. and It shows same result.
tests: 22988, fails: 0, ignored: 279

Also I have included few more tests to enable, please take a look.

PR: https://git.openjdk.java.net/jfx/pull/39

Re: [Rev 01] RFR: 8234194: [TEST_BUG] Reenable few graphics unit tests

[Ambarish Rapte]

The pull request has been updated with additional changes.



Added commits:
 - e7856f6e: reenable some more tests and fix review comment

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/39/files
  - new: https://git.openjdk.java.net/jfx/pull/39/files/5df00884..e7856f6e

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/39/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/39/webrev.00-01

  Issue: https://bugs.openjdk.java.net/browse/JDK-8234194
  Stats: 14 lines in 3 files changed: 6 ins; 3 del; 5 mod
  Patch: https://git.openjdk.java.net/jfx/pull/39.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/39/head:pull/39

PR: https://git.openjdk.java.net/jfx/pull/39

Re: [Rev 01] RFR: 8234239: [TEST_BUG] Reenable few ignored web tests

[Arun Joseph]

The pull request has been updated with additional changes.



Added commits:
 - ca460353: Remove ignore imports and update copyright

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/42/files
  - new: https://git.openjdk.java.net/jfx/pull/42/files/f2f121d9..ca460353

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/42/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/42/webrev.00-01

  Issue: https://bugs.openjdk.java.net/browse/JDK-8234239
  Stats: 6 lines in 3 files changed: 0 ins; 3 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/42.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/42/head:pull/42

PR: https://git.openjdk.java.net/jfx/pull/42

RFR: 8234239: [TEST_BUG] Reenable few ignored web tests

[Arun Joseph]

The following tests can be reenabled:

CookieManager.testPutOverwriteExpired
CookieManager.testPutPurgeDomainAfterExpiry
CookieManager.testPutPurgeCookiesGlobally2
CookieManager.testPutPurgeCookiesGlobally3
CookieManager.testPutPurgeCookiesGloballyAfterExpiry
CallbackTest.testCustomPopup
CallbackTest.testDefaultPopup
LeakTest.testGarbageCollectability
LeakTest.testOleg
MiscellaneousTest.testRT30835



Commits:
 - f2f121d9: 8234239: Reenable few ignored web tests

Changes: https://git.openjdk.java.net/jfx/pull/42/files
 Webrev: https://webrevs.openjdk.java.net/jfx/42/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8234239
  Stats: 20 lines in 4 files changed: 0 ins; 20 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/42.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/42/head:pull/42

PR: https://git.openjdk.java.net/jfx/pull/42

Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation

[Ajit Ghaisas]

On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth  wrote:

> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas  wrote:
> 
>> **Issue :**
>> https://bugs.openjdk.java.net/browse/JDK-8193445
>> 
>> **Background :**
>> The CSS performance improvement done in 
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to be 
>> backed out due to functional regressions reported in 
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), 
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and 
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
>> Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for 
>> more details on this backout. 
>> 
>> **Description :**
>> This PR reintroduces the CSS performance improvement fix done in 
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while 
>> addressing the functional regressions that were reported in 
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), 
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and 
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
>> For ease of review, I have made two separate commits -
>> 1) [Commit 
>> 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334)
>>  - Reintroduces the CSS performance improvement fix done in 
>> [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most of 
>> the patch applied cleanly.
>> 2) [Commit 2 
>> ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)-
>>  fixes the functional regressions keeping performance improvement intact + 
>> adds a system test
>> 
>> **Root Cause :**
>> CSS performance improvement fix proposed in [JDK-8151756 
>> ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the 
>> redundant CSS reapplication to children of a Parent. 
>> What was missed earlier in [JDK-8151756 
>> ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS reapplication 
>> to the Parent itself”. 
>> This missing piece was the root cause of all functional regressions reported 
>> against [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756)
>> 
>> **Fix :**
>> Fixed the identified root cause. See commit 2.
>> 
>> **Testing :**
>> 1. All passing unit tests continue to pass
>> 2. New system test (based on 
>> [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added in 
>> this PR - fails before this fix and passes after the fix
>> 3. System test JDK8183100Test continues to pass
>> 4. All test cases attached to regressions 
>> [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), 
>> [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and 
>> [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass with 
>> this fix
>> 
>> In addition, testing by community with specific CSS performance / 
>> functionality will be helpful.
>> 
>> 
>> 
>> Commits:
>>  - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem test
>>  - d964675e: Reintroduce JDK-8151756 CSS performance fix
>> 
>> Changes: https://git.openjdk.java.net/jfx/pull/34/files
>>  Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8193445
>>   Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod
>>   Patch: https://git.openjdk.java.net/jfx/pull/34.diff
>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34
> 
> While we are still discussing the fix itself, I added a few comments on the 
> new test. It generally looks good, but should be run on a variety of systems, 
> with and without the fix (once we have a final fix that we are satisfied 
> with).
> 
> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
>  line 26:
> 
>> 25: 
>> 26: package test.robot.javafx.scene;
>> 27: 
> 
> There is no need for this test to require robot. I recommend moving it to 
> `test.javafx.scene` (and not inherit from `VisualTestBase`).
> 
> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
>  line 55:
> 
>> 54: 
>> 55: public class CSSPerf_JDK8193445Test extends VisualTestBase {
>> 56: 
> 
> We have moved away from putting the bug ID in the test class name, so I 
> recommend renaming it.
> 
> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
>  line 78:
> 
>> 77: HBox hbox = new HBox();
>> 78: for (int i = 0; i < 300; i++) {
>> 79: hbox = new HBox(new Text("y"), hbox);
> 
> In my testing on various machines, the bug is more pronounced, and less prone 
> to system differences with `500` nodes instead of `300`.
> 
> tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
>  line 94:
> 
>> 93: // It is good enough to catch the regression in performance, if 
>> any
>> 94: assertTrue("Time to add 300