Integrated: 8090158: Wrong implementation of adjustValue in scrollBars

[Hadzic Samir]

On Tue, 20 Jul 2021 09:12:05 GMT, Hadzic Samir  wrote:

> JBS bug: [JDK-8090158](https://bugs.openjdk.java.net/browse/JDK-8090158)
> 
> The javadoc of the ScrollBar#adjustValue specifically says that it will 
> adjust the value based on the block increment value. Therefore, there is no 
> reason to stop at the given value when reaching an end.

This pull request has now been integrated.

Changeset: 5c355fa2
Author:    Hadzic Samir 
Committer: Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/5c355fa231e862a9db9b524cf14715066df142f5
Stats: 32 lines in 2 files changed: 28 ins; 3 del; 1 mod

8090158: Wrong implementation of adjustValue in scrollBars

Reviewed-by: aghaisas, kcr

-

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

Re: RFR: 8090158: Wrong implementation of adjustValue in scrollBars [v2]

[Hadzic Samir]

> JBS bug: [JDK-8090158](https://bugs.openjdk.java.net/browse/JDK-8090158)
> 
> The javadoc of the ScrollBar#adjustValue specifically says that it will 
> adjust the value based on the block increment value. Therefore, there is no 
> reason to stop at the given value when reaching an end.

Hadzic Samir has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix upon review

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/582/files
  - new: https://git.openjdk.java.net/jfx/pull/582/files/748b9242..06680e30

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=582=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=582=00-01

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/582.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/582/head:pull/582

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

Re: RFR: 8090158: Wrong implementation of adjustValue in scrollBars

[Hadzic Samir]

On Tue, 20 Jul 2021 09:12:05 GMT, Hadzic Samir  wrote:

> JBS bug: [JDK-8090158](https://bugs.openjdk.java.net/browse/JDK-8090158)
> 
> The javadoc of the ScrollBar#adjustValue specifically says that it will 
> adjust the value based on the block increment value. Therefore, there is no 
> reason to stop at the given value when reaching an end.

Sorry for the delay, I have missed the notifications somehow.
I've noticed that the order of the `assertEquals` are also in the wrong order 
to the other tests of this file.
Should I correct them also or leave it like that to avoid a large diff?

-

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

Re: RFR: WIP: 8230231: font-family not updated in HTMLEditor

[Hadzic Samir]

On Wed, 12 Feb 2020 16:54:08 GMT, Kevin Rushforth  wrote:

>> Applying this patch creates a new bug: Selecting text with multiple fonts in 
>> HTMLEditor sets the text to a single font.
>> 
>> Steps to reproduce:
>> Run the same sample program.
>> Type "Hello world".
>> Set "Hello" to FontA and "world" to FontB
>> Select all (or right to left) using keyboard
>> 
>> Expectation: "Hello world" is selected
>> Observation: "Hello world" is selected and font changed to FontA
> 
> @Maxoudela are you interesting in pursuing this?

I have indeed let this bug on the side. I would like to investigate but my time 
is limited these days. Especially that my patch is apparently creating another 
issue. I will try to give it another shot in the upcoming week.
But if anyone has some ideas or is willing to carry the issue, feel free

-

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

Re: [Rev 07] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Wed, 18 Dec 2019 17:32:05 GMT, Kevin Rushforth  wrote:

>> Looks good.
>> 
>> Once the CSR is approved, this can be integrated.
> 
> @Maxoudela the CSR has been approved. Go ahead and `/integrate` this and I 
> will sponsor it.

`/integrate`

-

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

Re: [Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Wed, 11 Dec 2019 00:32:32 GMT, Kevin Rushforth  wrote:

>> Thanks for the review. I do not have access to a computer right now, I'll
>> update next week on Monday.
>> 
>> Le lun. 9 déc. 2019 à 17:01, Kevin Rushforth  a
>> écrit :
>> 
>>> I would like both @aghaisas  and myself to
>>> review / approve this. I will sponsor it.
>>>
>>> In addition, the CSR needs to be approved before this can be integrated.
>>>
>>> —
>>> You are receiving this because you authored the thread.
>>> Reply to this email directly, view it on GitHub
>>> ,
>>> or unsubscribe
>>> 
>>> .
>>>
> 
> I took the liberty of updating the CSR to add the missing comma, so I could 
> mark it as Reviewed. You can Finalize the CSR when you are ready.

> 
> 
> I took the liberty of updating the CSR to add the missing comma, so I could 
> mark it as Reviewed. You can Finalize the CSR when you are ready.

Thank you, I have committed new changes according to your review. I have also 
moved the CSR to "finalize".

Let me know if I need to do something else, sorry for the delay.

-

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

Re: [Rev 07] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

> Allright, this is a fix for JDK-8207957

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 79817025: Remove trailing spaces

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/9991ec48..79817025

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.07
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.06-07

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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

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

Re: [Rev 06] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

> Allright, this is a fix for JDK-8207957

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 9991ec48: Minor typo fix upon Kevin's review

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/e1a9d2d0..9991ec48

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.06
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.05-06

  Stats: 27 lines in 2 files changed: 8 ins; 3 del; 16 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

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

Re: [Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Mon, 9 Dec 2019 21:01:40 GMT, Kevin Rushforth  wrote:

>> The fix looks good to me.
>> 
>> I left a few minor comments, including adding a missing comma in the API 
>> docs, which will also need to be changed in the CSR. Once this is updated 
>> I'll review the CSR and then you can move the CSR to Finalize.
> 
> I would like both @aghaisas and myself to review / approve this. I will 
> sponsor it.
> 
> In addition, the CSR needs to be approved before this can be integrated.

Thanks for the review. I do not have access to a computer right now, I'll
update next week on Monday.

Le lun. 9 déc. 2019 à 17:01, Kevin Rushforth  a
écrit :

> I would like both @aghaisas  and myself to
> review / approve this. I will sponsor it.
>
> In addition, the CSR needs to be approved before this can be integrated.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
>

-

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

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

[Hadzic Samir]

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
>>> <https://github.com/openjdk/jfx/pull/6#discussion_r341492115>:
>>>
>>> > + * 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
>>> <https://github.com/openjdk/jfx/pull/6?email_source=notifications_token=ACEL43PISZ6DM46QLSYDLR3QRPURHA5CNFSM4I5EYYAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ7XYEQ#discussion_r341492115>,
>>> or unsubscribe
>>> <https://github.com/notifications/unsubscribe-auth/ACEL43PROHPIZYN7WA5M6KDQRPURHANCNFSM4I5EYYAA>
>>> .
>>>
> 
> 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.

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

Re: [Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

The pull request has been updated with additional changes.



Added commits:
 - e1a9d2d0: Add more unit tests upon review

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/2b088993..e1a9d2d0

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.05
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.04-05

  Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
  Stats: 185 lines in 2 files changed: 117 ins; 48 del; 20 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

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

Re: [Rev 04] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Thu, 7 Nov 2019 12:48:47 GMT, Jeanette Winzenburg  
wrote:

> On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir  wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> 
>> 
>> Added commits:
>>  - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 54:
> 
>> 53: private MyTableColumnHeader tableColumnHeader;
>> 54: 
>> 55: @Before
> 
> If I understand the intention correctly, the only reason for subclassing the 
> TableView with a custom skin, headerRow, etc deep down into a custom 
> columnHeader is to access its protected resizeColumnToFitContent? If so, an 
> alternative approach might be to 
> 
> a) add a accessing method to TableColumnHeaderUtil in the shims hierarchy
> b) don't subclass but use that new accessor
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 61:
> 
>> 60: @Test
>> 61: public void test_resizeColumnToFitContent() {
>> 62: ObservableList model = FXCollections.observableArrayList(
> 
> This method actually is three-in-one :) It's testing
> 
> a) trigger a resize initially doesn't change the with 
> b) change content to larger and trigger a resize increases column width
> c) change content to smaller and trigger a resize decreases column width
> 
> Not sure about conventions here, but I would separate them out into distinct 
> methods.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 117:
> 
>> 116: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
>> 117: tableView, new TablePosition(tableView, 
>> 0, column), (EventType) eventType, "This is a big text inside that column"));
>> 118: tableColumnHeader.resizeCol();
> 
> that looks like a rather convoluted way of changing content - what's wrong 
> with straightforward changing the firstName of the first item :)
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 121:
> 
>> 120: width < column.getWidth());
>> 121: 
>> 122: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
> 
> feels a bit unspecific (though I can't think of how to do it better - the 
> internal measuring is ... doohoo ;)
> 
> what I usually do in such a case is to take this as a first step, then change 
> content back to original and again trigger a resize: now the current width 
> should be the same as the initial width.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 133:
> 
>> 132: assertTrue("Column width must be smaller",
>> 133: width > column.getWidth());
>> 134: }
> 
> same as above but the other way round.
> 
> Until, the tests verified that the column width is effected by cell content 
> and goes into the course direction of what is expected. What's still missing 
> are tests that cover the complete specification (we now have one :), that is 
> verify that/how resize is indeed depend on
> 
> - header content 
> - maxRows 
> 
> Plus maybe that custom implementations are respected: f.i. doing nothing, 
> making them constant, header only
> 
> An open question for me is: what is the standard procedure here? 
> 
> a) there are no tests around sizing (that I could find, maybe overlooked them)
> b) the change in this pull request is (more or less) simply moving around old 
> code and pulling from private into protected scope, no changes in 
> implementation/functionality
> 
> Now is the contributor responsible for writing those missing tests? It can be 
> considered a good opportunity ;) And strictly speaking is mandatory 
> (probably?) for all public/protected api. On the other extreme, doing nothing 
> (no tests because basically nothing changed) might be appropriate as well.
> 
> 
> 
> Disapproved by fastegal (Author).

But should I be able to call the protected method? I'll take a look to see what 
I can do

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

Re: [Rev 04] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Thu, 7 Nov 2019 12:48:47 GMT, Jeanette Winzenburg  
wrote:

> On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir  wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> 
>> 
>> Added commits:
>>  - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 54:
> 
>> 53: private MyTableColumnHeader tableColumnHeader;
>> 54: 
>> 55: @Before
> 
> If I understand the intention correctly, the only reason for subclassing the 
> TableView with a custom skin, headerRow, etc deep down into a custom 
> columnHeader is to access its protected resizeColumnToFitContent? If so, an 
> alternative approach might be to 
> 
> a) add a accessing method to TableColumnHeaderUtil in the shims hierarchy
> b) don't subclass but use that new accessor
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 61:
> 
>> 60: @Test
>> 61: public void test_resizeColumnToFitContent() {
>> 62: ObservableList model = FXCollections.observableArrayList(
> 
> This method actually is three-in-one :) It's testing
> 
> a) trigger a resize initially doesn't change the with 
> b) change content to larger and trigger a resize increases column width
> c) change content to smaller and trigger a resize decreases column width
> 
> Not sure about conventions here, but I would separate them out into distinct 
> methods.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 117:
> 
>> 116: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
>> 117: tableView, new TablePosition(tableView, 
>> 0, column), (EventType) eventType, "This is a big text inside that column"));
>> 118: tableColumnHeader.resizeCol();
> 
> that looks like a rather convoluted way of changing content - what's wrong 
> with straightforward changing the firstName of the first item :)
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 121:
> 
>> 120: width < column.getWidth());
>> 121: 
>> 122: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
> 
> feels a bit unspecific (though I can't think of how to do it better - the 
> internal measuring is ... doohoo ;)
> 
> what I usually do in such a case is to take this as a first step, then change 
> content back to original and again trigger a resize: now the current width 
> should be the same as the initial width.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 133:
> 
>> 132: assertTrue("Column width must be smaller",
>> 133: width > column.getWidth());
>> 134: }
> 
> same as above but the other way round.
> 
> Until, the tests verified that the column width is effected by cell content 
> and goes into the course direction of what is expected. What's still missing 
> are tests that cover the complete specification (we now have one :), that is 
> verify that/how resize is indeed depend on
> 
> - header content 
> - maxRows 
> 
> Plus maybe that custom implementations are respected: f.i. doing nothing, 
> making them constant, header only
> 
> An open question for me is: what is the standard procedure here? 
> 
> a) there are no tests around sizing (that I could find, maybe overlooked them)
> b) the change in this pull request is (more or less) simply moving around old 
> code and pulling from private into protected scope, no changes in 
> implementation/functionality
> 
> Now is the contributor responsible for writing those missing tests? It can be 
> considered a good opportunity ;) And strictly speaking is mandatory 
> (probably?) for all public/protected api. On the other extreme, doing nothing 
> (no tests because basically nothing changed) might be appropriate as well.
> 
> 
> 
> Disapproved by fastegal (Author).

Allright, I can do it like that if you prefer

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

Re: [Rev 04] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Thu, 7 Nov 2019 12:48:47 GMT, Jeanette Winzenburg  
wrote:

> On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir  wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> 
>> 
>> Added commits:
>>  - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 54:
> 
>> 53: private MyTableColumnHeader tableColumnHeader;
>> 54: 
>> 55: @Before
> 
> If I understand the intention correctly, the only reason for subclassing the 
> TableView with a custom skin, headerRow, etc deep down into a custom 
> columnHeader is to access its protected resizeColumnToFitContent? If so, an 
> alternative approach might be to 
> 
> a) add a accessing method to TableColumnHeaderUtil in the shims hierarchy
> b) don't subclass but use that new accessor
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 61:
> 
>> 60: @Test
>> 61: public void test_resizeColumnToFitContent() {
>> 62: ObservableList model = FXCollections.observableArrayList(
> 
> This method actually is three-in-one :) It's testing
> 
> a) trigger a resize initially doesn't change the with 
> b) change content to larger and trigger a resize increases column width
> c) change content to smaller and trigger a resize decreases column width
> 
> Not sure about conventions here, but I would separate them out into distinct 
> methods.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 117:
> 
>> 116: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
>> 117: tableView, new TablePosition(tableView, 
>> 0, column), (EventType) eventType, "This is a big text inside that column"));
>> 118: tableColumnHeader.resizeCol();
> 
> that looks like a rather convoluted way of changing content - what's wrong 
> with straightforward changing the firstName of the first item :)
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 121:
> 
>> 120: width < column.getWidth());
>> 121: 
>> 122: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
> 
> feels a bit unspecific (though I can't think of how to do it better - the 
> internal measuring is ... doohoo ;)
> 
> what I usually do in such a case is to take this as a first step, then change 
> content back to original and again trigger a resize: now the current width 
> should be the same as the initial width.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 133:
> 
>> 132: assertTrue("Column width must be smaller",
>> 133: width > column.getWidth());
>> 134: }
> 
> same as above but the other way round.
> 
> Until, the tests verified that the column width is effected by cell content 
> and goes into the course direction of what is expected. What's still missing 
> are tests that cover the complete specification (we now have one :), that is 
> verify that/how resize is indeed depend on
> 
> - header content 
> - maxRows 
> 
> Plus maybe that custom implementations are respected: f.i. doing nothing, 
> making them constant, header only
> 
> An open question for me is: what is the standard procedure here? 
> 
> a) there are no tests around sizing (that I could find, maybe overlooked them)
> b) the change in this pull request is (more or less) simply moving around old 
> code and pulling from private into protected scope, no changes in 
> implementation/functionality
> 
> Now is the contributor responsible for writing those missing tests? It can be 
> considered a good opportunity ;) And strictly speaking is mandatory 
> (probably?) for all public/protected api. On the other extreme, doing nothing 
> (no tests because basically nothing changed) might be appropriate as well.
> 
> 
> 
> Disapproved by fastegal (Author).

Allright, allright, I'll add the initial width Assert then

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

Re: [Rev 04] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Thu, 7 Nov 2019 12:48:47 GMT, Jeanette Winzenburg  
wrote:

> On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir  wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> 
>> 
>> Added commits:
>>  - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 54:
> 
>> 53: private MyTableColumnHeader tableColumnHeader;
>> 54: 
>> 55: @Before
> 
> If I understand the intention correctly, the only reason for subclassing the 
> TableView with a custom skin, headerRow, etc deep down into a custom 
> columnHeader is to access its protected resizeColumnToFitContent? If so, an 
> alternative approach might be to 
> 
> a) add a accessing method to TableColumnHeaderUtil in the shims hierarchy
> b) don't subclass but use that new accessor
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 61:
> 
>> 60: @Test
>> 61: public void test_resizeColumnToFitContent() {
>> 62: ObservableList model = FXCollections.observableArrayList(
> 
> This method actually is three-in-one :) It's testing
> 
> a) trigger a resize initially doesn't change the with 
> b) change content to larger and trigger a resize increases column width
> c) change content to smaller and trigger a resize decreases column width
> 
> Not sure about conventions here, but I would separate them out into distinct 
> methods.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 117:
> 
>> 116: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
>> 117: tableView, new TablePosition(tableView, 
>> 0, column), (EventType) eventType, "This is a big text inside that column"));
>> 118: tableColumnHeader.resizeCol();
> 
> that looks like a rather convoluted way of changing content - what's wrong 
> with straightforward changing the firstName of the first item :)
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 121:
> 
>> 120: width < column.getWidth());
>> 121: 
>> 122: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
> 
> feels a bit unspecific (though I can't think of how to do it better - the 
> internal measuring is ... doohoo ;)
> 
> what I usually do in such a case is to take this as a first step, then change 
> content back to original and again trigger a resize: now the current width 
> should be the same as the initial width.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 133:
> 
>> 132: assertTrue("Column width must be smaller",
>> 133: width > column.getWidth());
>> 134: }
> 
> same as above but the other way round.
> 
> Until, the tests verified that the column width is effected by cell content 
> and goes into the course direction of what is expected. What's still missing 
> are tests that cover the complete specification (we now have one :), that is 
> verify that/how resize is indeed depend on
> 
> - header content 
> - maxRows 
> 
> Plus maybe that custom implementations are respected: f.i. doing nothing, 
> making them constant, header only
> 
> An open question for me is: what is the standard procedure here? 
> 
> a) there are no tests around sizing (that I could find, maybe overlooked them)
> b) the change in this pull request is (more or less) simply moving around old 
> code and pulling from private into protected scope, no changes in 
> implementation/functionality
> 
> Now is the contributor responsible for writing those missing tests? It can be 
> considered a good opportunity ;) And strictly speaking is mandatory 
> (probably?) for all public/protected api. On the other extreme, doing nothing 
> (no tests because basically nothing changed) might be appropriate as well.
> 
> 
> 
> Disapproved by fastegal (Author).

Changing the firstName of the firstItem would be another test IMHO. It would 
test the fact that the backing model has changed and needs to be reflected in 
the view.

Here I want to test the fact that a user has modified a cell through the view, 
and it impacts the model.

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

Re: [Rev 04] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Thu, 7 Nov 2019 12:48:47 GMT, Jeanette Winzenburg  
wrote:

> On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir  wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> 
>> 
>> Added commits:
>>  - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 54:
> 
>> 53: private MyTableColumnHeader tableColumnHeader;
>> 54: 
>> 55: @Before
> 
> If I understand the intention correctly, the only reason for subclassing the 
> TableView with a custom skin, headerRow, etc deep down into a custom 
> columnHeader is to access its protected resizeColumnToFitContent? If so, an 
> alternative approach might be to 
> 
> a) add a accessing method to TableColumnHeaderUtil in the shims hierarchy
> b) don't subclass but use that new accessor
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 61:
> 
>> 60: @Test
>> 61: public void test_resizeColumnToFitContent() {
>> 62: ObservableList model = FXCollections.observableArrayList(
> 
> This method actually is three-in-one :) It's testing
> 
> a) trigger a resize initially doesn't change the with 
> b) change content to larger and trigger a resize increases column width
> c) change content to smaller and trigger a resize decreases column width
> 
> Not sure about conventions here, but I would separate them out into distinct 
> methods.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 117:
> 
>> 116: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
>> 117: tableView, new TablePosition(tableView, 
>> 0, column), (EventType) eventType, "This is a big text inside that column"));
>> 118: tableColumnHeader.resizeCol();
> 
> that looks like a rather convoluted way of changing content - what's wrong 
> with straightforward changing the firstName of the first item :)
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 121:
> 
>> 120: width < column.getWidth());
>> 121: 
>> 122: column.getOnEditCommit().handle(new 
>> TableColumn.CellEditEvent(
> 
> feels a bit unspecific (though I can't think of how to do it better - the 
> internal measuring is ... doohoo ;)
> 
> what I usually do in such a case is to take this as a first step, then change 
> content back to original and again trigger a resize: now the current width 
> should be the same as the initial width.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 133:
> 
>> 132: assertTrue("Column width must be smaller",
>> 133: width > column.getWidth());
>> 134: }
> 
> same as above but the other way round.
> 
> Until, the tests verified that the column width is effected by cell content 
> and goes into the course direction of what is expected. What's still missing 
> are tests that cover the complete specification (we now have one :), that is 
> verify that/how resize is indeed depend on
> 
> - header content 
> - maxRows 
> 
> Plus maybe that custom implementations are respected: f.i. doing nothing, 
> making them constant, header only
> 
> An open question for me is: what is the standard procedure here? 
> 
> a) there are no tests around sizing (that I could find, maybe overlooked them)
> b) the change in this pull request is (more or less) simply moving around old 
> code and pulling from private into protected scope, no changes in 
> implementation/functionality
> 
> Now is the contributor responsible for writing those missing tests? It can be 
> considered a good opportunity ;) And strictly speaking is mandatory 
> (probably?) for all public/protected api. On the other extreme, doing nothing 
> (no tests because basically nothing changed) might be appropriate as well.
> 
> 
> 
> Disapproved by fastegal (Author).

Well that depends on the point of view. From my point of view, I'm testing the 
"resize to fit content" feature. So I'm testing one feature and therefore the 
same size, a bit smaller and a bit greater. 
But we could split this into 3 separate tests if that is the convention.

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

Re: [Rev 01] RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it

[Hadzic Samir]

On Tue, 5 Nov 2019 11:17:55 GMT, Arun Joseph 
 wrote:

> The pull request has been updated with additional changes.
> 
> 
> 
> Added commits:
>  - afc7f17a: Minor formatting
> 
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/27/files
>   - new: https://git.openjdk.java.net/jfx/pull/27/files/5a1fbade..afc7f17a
> 
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/27/webrev.01
>  - incr: https://webrevs.openjdk.java.net/jfx/27/webrev.00-01
> 
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8230492
>   Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/27.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27

Approved by shadzic (Author).

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

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

[Hadzic Samir]

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
> <https://github.com/openjdk/jfx/pull/6#discussion_r341492115>:
>
> > + * 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
> <https://github.com/openjdk/jfx/pull/6?email_source=notifications_token=ACEL43PISZ6DM46QLSYDLR3QRPURHA5CNFSM4I5EYYAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ7XYEQ#discussion_r341492115>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACEL43PROHPIZYN7WA5M6KDQRPURHANCNFSM4I5EYYAA>
> .
>

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

Re: [Rev 04] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir  wrote:

> The pull request has been updated with additional changes.
> 
> 
> 
> Added commits:
>  - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader
> 
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>   - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993
> 
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04
> 
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>   Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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 611:

> 610: protected void resizeColumnToFitContent(int maxRows) {
> 611: TableColumnBase tc = getTableColumn();
> 612: if (!tc.isResizable()) return;

@nlisker I have added it this way. The auto-format of IntelliJ wanted to put 
the param before the implSpec but the documentation shows an example like that 
: 

> /**
>  * ... API specifications ...
>  *
>  * @apiNote
>  * ... API notes ...
>  *
>  * @implSpec
>  * ... implementation specification ...
>  *
>  * @implNote
>  * ... implementation notes ...
>  *
>  * @param ...
>  * @return ...
>  * @throws ...
>  */

So I thought this was the right way, let me know if I'm mistaken

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

Re: [Rev 04] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

The pull request has been updated with additional changes.



Added commits:
 - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04

  Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
  Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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

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

Re: RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it

[Hadzic Samir]

On Wed, 30 Oct 2019 10:07:42 GMT, Arun Joseph 
 wrote:

> In the HTMLEditor, when positioning the caret in a text and trying to set a 
> font-family that has a number in it is not working.
> 
> Issue: In 
> [CSSPropertyParser.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/css/parser/CSSPropertyParser.cpp),
>  concatenateFamilyName() function parses only identifiers. So, when a number 
> is introduced in a font-name, it fails.
> 
> Fix: Pass the font-name as a string in HTMLEditorSkin.java by adding quotes.
> 
> A new font is added as a resource for the test. This font is same as 
> modules/javafx.web/src/main/native/Tools/DumpRenderTree/fonts/WebKit Layout 
> Tests 2.ttf
> 
> 
> 
> Commits:
>  - 5a1fbade: 8230492: font-family not set in HTMLEditor if font name has a 
> number in it
> 
> Changes: https://git.openjdk.java.net/jfx/pull/27/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/27/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8230492
>   Stats: 62 lines in 3 files changed: 60 ins; 0 del; 2 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/27.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27

Will this be in conflict with https://github.com/openjdk/jfx/pull/12 ? I'll 
look at your unit tests in order to implement one similar

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

Re: [Rev 01] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Tue, 29 Oct 2019 14:38:10 GMT, Nir Lisker  wrote:

> On Wed, 9 Oct 2019 16:18:49 GMT, Kevin Rushforth  wrote:
> 
>> On Wed, 9 Oct 2019 16:11:49 GMT, Hadzic Samir  wrote:
>> 
>>> On Wed, 9 Oct 2019 12:25:26 GMT, Hadzic Samir  wrote:
>>> 
>>>> The pull request has been updated with additional changes.
>>>> 
>>>> 
>>>> 
>>>> Added commits:
>>>>  - e846e51c: Remove TableColumn argument for resizeColumnToFitContent for 
>>>> clarification on TableColumnHeader
>>>> 
>>>> Changes:
>>>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/969ebb51..e846e51c
>>>> 
>>>> Webrevs:
>>>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.01
>>>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.00-01
>>>> 
>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>>>   Stats: 27 lines in 3 files changed: 13 ins; 1 del; 13 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 608:
>>> 
>>>> 607: protected void resizeColumnToFitContent(int maxRows) {
>>>> 608: TableColumnBase tc = getTableColumn();
>>>> 609: if (!tc.isResizable()) return;
>>> 
>>> I have put the `since 14`, because if merged, it will be available in 
>>> OpenJFX 14 right? (It was 13 before)
>> 
>> Correct.
> 
> The "The resulting column width for this implementation..." part might belong 
> in an `@implSpec` section.

>From https://openjdk.java.net/jeps/8068562

> Implementation Specification. This is where the default implementation (or an 
> overrideable implementation in a class) is specified. Interface implementors 
> or class subclassers use the information here in order to decide whether it 
> is sensible or necessary to override a particular method, and what behavior 
> they can rely on if a method is called via super.

So maybe the whole part beginning from "The resulting column [..]width etc.)." 
should be in the `@implSpec` section.

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

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

[Hadzic Samir]

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 ?

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

Re: [Rev 03] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

The pull request has been updated with additional changes.



Added commits:
 - 1f1f7c44: Javadoc update upon review for tableColumnHeader

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/49abc7c7..1f1f7c44

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.03
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.02-03

  Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
  Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 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

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

Re: [Rev 01] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Wed, 9 Oct 2019 15:49:21 GMT, Nir Lisker  wrote:

> On Wed, 9 Oct 2019 12:25:26 GMT, Hadzic Samir  wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> 
>> 
>> Added commits:
>>  - e846e51c: Remove TableColumn argument for resizeColumnToFitContent for 
>> clarification on TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/969ebb51..e846e51c
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.01
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.00-01
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 27 lines in 3 files changed: 13 ins; 1 del; 13 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/NestedTableColumnHeader.java
>  line 170:
> 
>> 169: TableColumnHeader columnHeader = 
>> tableHeader.getColumnHeaderFor(column);
>> 170: if(columnHeader != null){
>> 171: columnHeader.resizeColumnToFitContent(-1);
> 
> Space after `if` and before `{`.
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 608:
> 
>> 607: protected void resizeColumnToFitContent(int maxRows) {
>> 608: TableColumnBase tc = getTableColumn();
>> 609: if (!tc.isResizable()) return;
> 
> Here's my suggestion (mind the max character length per line):
> 
> Resizes this {@code TableColumnHeader}'s column to fit the width of its 
> content. The resulting column width 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.).
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 618:
> 
>> 617: }
>> 618: 
>> 619: private  void resizeColumnToFitContent(TableView tv, 
>> TableColumn tc, TableViewSkinBase tableSkin, int maxRows) {
> 
> I think we put a space when casting, like `(TableView) control`. Not sure if 
> it is a rule.
> 
> Since you are adding new API you will need to file a CSR once the API review 
> id done.
> 
> 
> 
> Disapproved by nlisker (Committer).

I wouldn't know if there are enough tests on that part. I know I added one.

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

Re: RFR: WIP: 8230231: font-family not updated in HTMLEditor

[Hadzic Samir]

On Thu, 10 Oct 2019 12:19:18 GMT, Kevin Rushforth  wrote:

> On Thu, 10 Oct 2019 09:12:57 GMT, Hadzic Samir  wrote:
> 
>> On Wed, 9 Oct 2019 20:09:58 GMT, Kevin Rushforth  wrote:
>> 
>>> On Wed, 9 Oct 2019 16:09:58 GMT, Hadzic Samir  wrote:
>>> 
>>>> On Wed, 9 Oct 2019 16:09:07 GMT, Kevin Rushforth  wrote:
>>>> 
>>>>> On Wed, 9 Oct 2019 16:09:06 GMT, Hadzic Samir  wrote:
>>>>> 
>>>>>> Fix for https://github.com/javafxports/openjdk-jfx/issues/573
>>>>>> 
>>>>>> Issue on JDK bug tracking : 
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8230231
>>>>>> 
>>>>>> I tried to add a test but I do not succeed at even running the existing 
>>>>>> Web tests.. I will need some help on that side..
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> Commits:
>>>>>>  - e9df9db5: Adding double-quote for HTMLEditorSkin font-family
>>>>>> 
>>>>>> Changes: https://git.openjdk.java.net/jfx/pull/12/files
>>>>>>  Webrev: https://webrevs.openjdk.java.net/jfx/12/webrev.00
>>>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8230231
>>>>>>   Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
>>>>>>   Patch: https://git.openjdk.java.net/jfx/pull/12.diff
>>>>>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/12/head:pull/12
>>>>> 
>>>>> @Maxoudela please edit the title as follows:
>>>>> 
>>>>> 1. Remove the space before the `:` (that extra space is why jcheck failed)
>>>>> 2. Change the text of the title to match the JBS bug summary exactly. You 
>>>>> can edit the JBS bug summary if you feel it needs to be changed, but in 
>>>>> this case, the JBS bug has a title that is more in line with our usual 
>>>>> practice of having the bug title be descriptive of what the problem is 
>>>>> and not what the solution happens to be.
>>>>> 
>>>>> As for unit tests, you will very likely need to add this as a system test 
>>>>> under `tests/system/src/main/test`. See 
>>>>> [tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java).
>>>>>  Presuming that you can add your test to that existing class, you would 
>>>>> run it as follows:
>>>>> 
>>>>> gradle -PFULL_TEST=true :systemTests:test --tests HTMLEditorTest
>>>> 
>>>> Thanks @kevinrushforth . I'm sorry for posting the Pull request like that, 
>>>> I will thoroughly read the contributing guidelines and updates my PR 
>>>> accordingly.
>>>> 
>>>> I'll try to add a test asap, thanks for the pointer.
>>> 
>>>> I'm sorry for posting the Pull request like that
>>> 
>>> No problem. I mainly wanted to make sure that you knew why the RFR wasn't 
>>> sent. As for the note about the title matching, the contributing guidelines 
>>> don't mention that and I now realize that they should -- I'll add that 
>>> along with some other improvements I'll be making.
>>> 
>>>> I'll try to add a test asap, thanks for the pointer.
>>> 
>>> Great, thanks.
>> 
>> Hum I do not succeed in running the existing test either. Here is my log. 
>> Apparently, the tests are failing because the WebView is null and not 
>> initialized. Do you have any clue of what I've been doing wrong? 
>> 
>> Should I try to update my gradle version and JDK version maybe?
>> gradle -PFULL_TEST=true :systemTests:test --tests HTMLEditorTest
>> Starting a Gradle Daemon (subsequent builds will be faster)
>> 
>>> Configure project :
>> MACOSX_MIN_VERSION = 10.9
>> MACOSX_SDK_PATH = 
>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
>> *
>> Unsupported gradle version 4.8 in use.
>> Only version 5.3 is supported. Use this version at your own risk
>> *
>> gradle.gradleVersion: 4.8
>> OS_NAME: mac os x
>> OS_ARCH: x86_64
>> JAVA_HOME: /Library/Java/JavaVirtualMachines/jdk-11.0.1.jdk/Contents/Home
>> JDK_HOME: /Library/Java/JavaVirtualMachines/jdk

Re: RFR: WIP: 8230231: font-family not updated in HTMLEditor

[Hadzic Samir]

On Wed, 9 Oct 2019 20:09:58 GMT, Kevin Rushforth  wrote:

> On Wed, 9 Oct 2019 16:09:58 GMT, Hadzic Samir  wrote:
> 
>> On Wed, 9 Oct 2019 16:09:07 GMT, Kevin Rushforth  wrote:
>> 
>>> On Wed, 9 Oct 2019 16:09:06 GMT, Hadzic Samir  wrote:
>>> 
>>>> Fix for https://github.com/javafxports/openjdk-jfx/issues/573
>>>> 
>>>> Issue on JDK bug tracking : 
>>>> https://bugs.openjdk.java.net/browse/JDK-8230231
>>>> 
>>>> I tried to add a test but I do not succeed at even running the existing 
>>>> Web tests.. I will need some help on that side..
>>>> 
>>>> 
>>>> 
>>>> Commits:
>>>>  - e9df9db5: Adding double-quote for HTMLEditorSkin font-family
>>>> 
>>>> Changes: https://git.openjdk.java.net/jfx/pull/12/files
>>>>  Webrev: https://webrevs.openjdk.java.net/jfx/12/webrev.00
>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8230231
>>>>   Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
>>>>   Patch: https://git.openjdk.java.net/jfx/pull/12.diff
>>>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/12/head:pull/12
>>> 
>>> @Maxoudela please edit the title as follows:
>>> 
>>> 1. Remove the space before the `:` (that extra space is why jcheck failed)
>>> 2. Change the text of the title to match the JBS bug summary exactly. You 
>>> can edit the JBS bug summary if you feel it needs to be changed, but in 
>>> this case, the JBS bug has a title that is more in line with our usual 
>>> practice of having the bug title be descriptive of what the problem is and 
>>> not what the solution happens to be.
>>> 
>>> As for unit tests, you will very likely need to add this as a system test 
>>> under `tests/system/src/main/test`. See 
>>> [tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java).
>>>  Presuming that you can add your test to that existing class, you would run 
>>> it as follows:
>>> 
>>> gradle -PFULL_TEST=true :systemTests:test --tests HTMLEditorTest
>> 
>> Thanks @kevinrushforth . I'm sorry for posting the Pull request like that, I 
>> will thoroughly read the contributing guidelines and updates my PR 
>> accordingly.
>> 
>> I'll try to add a test asap, thanks for the pointer.
> 
>> I'm sorry for posting the Pull request like that
> 
> No problem. I mainly wanted to make sure that you knew why the RFR wasn't 
> sent. As for the note about the title matching, the contributing guidelines 
> don't mention that and I now realize that they should -- I'll add that along 
> with some other improvements I'll be making.
> 
>> I'll try to add a test asap, thanks for the pointer.
> 
> Great, thanks.

Hum I do not succeed in running the existing test either. Here is my log. 
Apparently, the tests are failing because the WebView is null and not 
initialized. Do you have any clue of what I've been doing wrong? 

Should I try to update my gradle version and JDK version maybe?
gradle -PFULL_TEST=true :systemTests:test --tests HTMLEditorTest
Starting a Gradle Daemon (subsequent builds will be faster)

> Configure project :
MACOSX_MIN_VERSION = 10.9
MACOSX_SDK_PATH = 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
*
Unsupported gradle version 4.8 in use.
Only version 5.3 is supported. Use this version at your own risk
*
gradle.gradleVersion: 4.8
OS_NAME: mac os x
OS_ARCH: x86_64
JAVA_HOME: /Library/Java/JavaVirtualMachines/jdk-11.0.1.jdk/Contents/Home
JDK_HOME: /Library/Java/JavaVirtualMachines/jdk-11.0.1.jdk/Contents/Home
java.runtime.version: 11.0.1+13
java version: 11.0.1
java build number: 13
jdk.runtime.version: 11.0.1+13
jdk version: 11.0.1
jdk build number: 13
minimum jdk version: 11
minimum jdk build number: 28
XCODE version: Xcode10.1-MacOSX10.14+1.0
cmake version: 3.13.3
ninja version: 1.8.2
ant version: 1.10.5
HAS_JAVAFX_MODULES: false
STUB_RUNTIME: /Library/Java/JavaVirtualMachines/jdk-11.0.1.jdk/Contents/Home
CONF: Debug
NUM_COMPILE_THREADS: 1
COMPILE_TARGETS: mac
COMPILE_FLAGS_FILES: buildSrc/mac.gradle
HUDSON_JOB_NAME: not_hudson
HUDSON_BUILD_NUMBER: 
PROMOTED_BUILD_NUMBER: 0
PRODUCT_NAME: OpenJFX
RELEASE_VERSION: 14
RELEASE_SUFFIX: -internal
RELEASE_VERSION_SHORT: 14-internal
RELEASE_VERSION_LONG: 14-internal+0-2019-10-10-110056
RELEASE_VERSION_PADDED: 14.0.0.0
MAVEN_VERSION: 14-in

Re: [Rev 02] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

The pull request has been updated with additional changes.



Added commits:
 - 49abc7c7: Add space and update Javadoc for tableColumnHeader and 
NestedTableColumnHeader

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/e846e51c..49abc7c7

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.02
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.01-02

  Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
  Stats: 9 lines in 2 files changed: 1 ins; 0 del; 8 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

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

Re: [Rev 01] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Wed, 9 Oct 2019 15:49:21 GMT, Nir Lisker  wrote:

> On Wed, 9 Oct 2019 12:25:26 GMT, Hadzic Samir  wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> 
>> 
>> Added commits:
>>  - e846e51c: Remove TableColumn argument for resizeColumnToFitContent for 
>> clarification on TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/969ebb51..e846e51c
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.01
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.00-01
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 27 lines in 3 files changed: 13 ins; 1 del; 13 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/NestedTableColumnHeader.java
>  line 170:
> 
>> 169: TableColumnHeader columnHeader = 
>> tableHeader.getColumnHeaderFor(column);
>> 170: if(columnHeader != null){
>> 171: columnHeader.resizeColumnToFitContent(-1);
> 
> Space after `if` and before `{`.
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 608:
> 
>> 607: protected void resizeColumnToFitContent(int maxRows) {
>> 608: TableColumnBase tc = getTableColumn();
>> 609: if (!tc.isResizable()) return;
> 
> Here's my suggestion (mind the max character length per line):
> 
> Resizes this {@code TableColumnHeader}'s column to fit the width of its 
> content. The resulting column width 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.).
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 618:
> 
>> 617: }
>> 618: 
>> 619: private  void resizeColumnToFitContent(TableView tv, 
>> TableColumn tc, TableViewSkinBase tableSkin, int maxRows) {
> 
> I think we put a space when casting, like `(TableView) control`. Not sure if 
> it is a rule.
> 
> Since you are adding new API you will need to file a CSR once the API review 
> id done.
> 
> 
> 
> Disapproved by nlisker (Committer).

Seems good but I have a minor doubt on "header cell". Can the header be 
considered as a "cell"?

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

Re: [Rev 01] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Wed, 9 Oct 2019 12:25:26 GMT, Hadzic Samir  wrote:

> The pull request has been updated with additional changes.
> 
> 
> 
> Added commits:
>  - e846e51c: Remove TableColumn argument for resizeColumnToFitContent for 
> clarification on TableColumnHeader
> 
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>   - new: https://git.openjdk.java.net/jfx/pull/6/files/969ebb51..e846e51c
> 
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.01
>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.00-01
> 
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>   Stats: 27 lines in 3 files changed: 13 ins; 1 del; 13 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 608:

> 607: protected void resizeColumnToFitContent(int maxRows) {
> 608: TableColumnBase tc = getTableColumn();
> 609: if (!tc.isResizable()) return;

I have put the `since 14`, because if merged, it will be available in OpenJFX 
14 right? (It was 13 before)

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

Re: RFR: 8230231: font-family not updated in HTMLEditor

[Hadzic Samir]

On Wed, 9 Oct 2019 16:09:07 GMT, Kevin Rushforth  wrote:

> On Wed, 9 Oct 2019 16:09:06 GMT, Hadzic Samir  wrote:
> 
>> Fix for https://github.com/javafxports/openjdk-jfx/issues/573
>> 
>> Issue on JDK bug tracking : https://bugs.openjdk.java.net/browse/JDK-8230231
>> 
>> I tried to add a test but I do not succeed at even running the existing Web 
>> tests.. I will need some help on that side..
>> 
>> 
>> 
>> Commits:
>>  - e9df9db5: Adding double-quote for HTMLEditorSkin font-family
>> 
>> Changes: https://git.openjdk.java.net/jfx/pull/12/files
>>  Webrev: https://webrevs.openjdk.java.net/jfx/12/webrev.00
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8230231
>>   Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
>>   Patch: https://git.openjdk.java.net/jfx/pull/12.diff
>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/12/head:pull/12
> 
> @Maxoudela please edit the title as follows:
> 
> 1. Remove the space before the `:` (that extra space is why jcheck failed)
> 2. Change the text of the title to match the JBS bug summary exactly. You can 
> edit the JBS bug summary if you feel it needs to be changed, but in this 
> case, the JBS bug has a title that is more in line with our usual practice of 
> having the bug title be descriptive of what the problem is and not what the 
> solution happens to be.
> 
> As for unit tests, you will very likely need to add this as a system test 
> under `tests/system/src/main/test`. See 
> [tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java).
>  Presuming that you can add your test to that existing class, you would run 
> it as follows:
> 
> gradle -PFULL_TEST=true :systemTests:test --tests HTMLEditorTest

Thanks @kevinrushforth . I'm sorry for posting the Pull request like that, I 
will thoroughly read the contributing guidelines and updates my PR accordingly.

I'll try to add a test asap, thanks for the pointer.

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

RFR: 8230231: font-family not updated in HTMLEditor

[Hadzic Samir]

Fix for https://github.com/javafxports/openjdk-jfx/issues/573

Issue on JDK bug tracking : https://bugs.openjdk.java.net/browse/JDK-8230231

I tried to add a test but I do not succeed at even running the existing Web 
tests.. I will need some help on that side..



Commits:
 - e9df9db5: Adding double-quote for HTMLEditorSkin font-family

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

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

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

[Hadzic Samir]

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..

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

Re: [Rev 01] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

The pull request has been updated with additional changes.



Added commits:
 - e846e51c: Remove TableColumn argument for resizeColumnToFitContent for 
clarification on TableColumnHeader

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/969ebb51..e846e51c

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

  Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
  Stats: 27 lines in 3 files changed: 13 ins; 1 del; 13 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

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

RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

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

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