Re: RFR: 8267554: Support loading stylesheets from data-URIs
On Fri, 18 Jun 2021 01:23:50 GMT, Michael Strauß wrote:
> This PR extends data URI support to allow stylesheets to be loaded from data
> URIs.
I looked at just the public API and spec, and have two comments:
* I don't see any justification for adding
`Stylesheet::loadBinary(InputStream)` to the public API. See my comments inline.
* Do you intend to support setting a user agent stylesheet via a data URL? The
docs should be explicit about whether or not a data URI can be used for the
following methods:
*
[Application::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/application/Application.java#L521)
*
[Scene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1697)
*
[SubScene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/SubScene.java#L687)
modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 301:
> 299: * css version or if an I/O error occurs while reading from the
> stream
> 300: */
> 301: public static Stylesheet loadBinary(InputStream stream) throws
> IOException {
Why do you need to add this public method to the API? I don't see any
discussion as to why JavaFX applications need it. It looks like it is just
being used internally by `StyleManager`. Unless there is a compelling reason to
add this to the API, you will need to make this method package-scope and use an
accessor to access it from `StyleManager`.
-
PR: https://git.openjdk.java.net/jfx/pull/536
Integrated: 8269026: PasswordField doesn't render bullet character on Android
On Fri, 18 Jun 2021 14:08:25 GMT, Jose Pereda wrote: > This PR modifies the PasswordField's bullet character used on Android, as the > current unicode code is not supported for most fonts, including Roboto. This pull request has now been integrated. Changeset: 13cffbaa Author:Jose Pereda URL: https://git.openjdk.java.net/jfx/commit/13cffbaad4068177d2d3239fa297302c3f94c217 Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod 8269026: PasswordField doesn't render bullet character on Android Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/537
Re: RFR: 8269026: PasswordField doesn't render bullet character on Android [v2]
On Fri, 18 Jun 2021 14:45:21 GMT, Jose Pereda wrote: >> This PR modifies the PasswordField's bullet character used on Android, as >> the current unicode code is not supported for most fonts, including Roboto. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback from reviewer Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/537
Re: RFR: 8269026: PasswordField doesn't render bullet character on Android [v2]
> This PR modifies the PasswordField's bullet character used on Android, as the > current unicode code is not supported for most fonts, including Roboto. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Address feedback from reviewer - Changes: - all: https://git.openjdk.java.net/jfx/pull/537/files - new: https://git.openjdk.java.net/jfx/pull/537/files/5cbe3372..be073acb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=537=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=537=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/537.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/537/head:pull/537 PR: https://git.openjdk.java.net/jfx/pull/537
Re: RFR: 8269026: PasswordField doesn't render bullet character on Android
On Fri, 18 Jun 2021 14:21:16 GMT, Kevin Rushforth wrote:
>> This PR modifies the PasswordField's bullet character used on Android, as
>> the current unicode code is not supported for most fonts, including Roboto.
>
> modules/javafx.controls/src/android/java/javafx/scene/control/skin/TextFieldSkinAndroid.java
> line 82:
>
>> 80: return String.valueOf(BULLET).repeat(txt.length());
>> 81: } else {
>> 82: return txt;
>
> Should this call `return super.maskText(txt);` in case the implementation of
> the superclass method ever changes? Either is OK with me.
I think that makes sense, even if it causes a double check of getSkinnable(),
but that seems negligible. I'll update it
-
PR: https://git.openjdk.java.net/jfx/pull/537
Re: RFR: 8269026: PasswordField doesn't render bullet character on Android
On Fri, 18 Jun 2021 14:08:25 GMT, Jose Pereda wrote:
> This PR modifies the PasswordField's bullet character used on Android, as the
> current unicode code is not supported for most fonts, including Roboto.
Marked as reviewed by kcr (Lead).
modules/javafx.controls/src/android/java/javafx/scene/control/skin/TextFieldSkinAndroid.java
line 82:
> 80: return String.valueOf(BULLET).repeat(txt.length());
> 81: } else {
> 82: return txt;
Should this call `return super.maskText(txt);` in case the implementation of
the superclass method ever changes? Either is OK with me.
-
PR: https://git.openjdk.java.net/jfx/pull/537
RFR: 8269026: PasswordField doesn't render bullet character on Android
This PR modifies the PasswordField's bullet character used on Android, as the current unicode code is not supported for most fonts, including Roboto. - Commit messages: - Modify bullet character for PasswordField on Android Changes: https://git.openjdk.java.net/jfx/pull/537/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=537=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269026 Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/537.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/537/head:pull/537 PR: https://git.openjdk.java.net/jfx/pull/537
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v26]
On Thu, 17 Jun 2021 19:31:29 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed method call in glsl shaders > > modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vsConstants.h line 58: > >> 56: float4x3mBones[MAX_BONES] : register(c35); >> 57: >> 58: float4 gReserved240[16] : register(c240); > > `gReserved240` is now at the wrong location (it should be 245), so if it were > ever used it would be a problem. It should be updated to avoid confusion at > least. I think the size should be updated to 11 as well (since we probably don't want to go past 256). - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
On Fri, 18 Jun 2021 10:14:40 GMT, Jeanette Winzenburg
wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java
>> line 310:
>>
>>> 308: (table != null && !table.isEditable()) ||
>>> 309: (column != null && !column.isEditable()) ||
>>> 310: (row != null) && !row.isEditable()) {
>>
>> Incorrect Line "(row != null) && !row.isEditable())"
>> Correction required "(row != null && !row.isEditable()))"
>>
>> Refer similar line which is rightly implemented in TreeTableCell.java.
>
> darn .. you certainly have the better eyes :)))
Hmm .. this should have been be found by a test .. having a combination table
== null, column == null, row == null would throw. But then, test coverage is ..
;)
-
PR: https://git.openjdk.java.net/jfx/pull/529
Re: RFR: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
On Sun, 6 Jun 2021 12:44:00 GMT, Marius Hanl wrote:
> I created a follow-up issues for fixing all the sub Tree- and TableCell
> implementation which do not count the row editability in:
> [JDK-8268295](https://bugs.openjdk.java.net/browse/JDK-8268295)
as already commented in the follow-up - I think that it will not be needed
after fixing the precondition violation (there are none for any of the
cell.xxEdit methods, so the concrete classes must not introduce any)
[JDK-8188026](https://bugs.openjdk.java.net/browse/JDK-8188026). The impl
pattern will be something like
public void startEdit() {
if (isEditing()) return;
super.startEdit();
if (!isEditing()) return;
// config edit state
}
Thus row editable will already be taking into account by super.
-
PR: https://git.openjdk.java.net/jfx/pull/529
Re: RFR: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
On Sun, 6 Jun 2021 12:44:00 GMT, Marius Hanl wrote:
> This PR enables Tree- and TableCells to also check the row editability when
> an edit should happen. With this a Tree- or TableCell is not editable, when
> the row where the cell is in is not.
>
> While this PR fixes the problem described in the ticket, it does not fix the
> example.
> This is due the example uses the **CheckBoxTableCell**, which is a
> ready-to-use subclass of **TableCell**.
>
> While looking into this, I found out that multiple sub implementations still
> have this issue, but the fix is not always the same, e.g. CheckBoxTableCell
> should disable the CheckBox (in **updateItem**), while the ChoiceBoxTableCell
> should check the row editability in the **startEdit** method (like this PR
> does).
>
> I created a follow-up issues for fixing all the sub Tree- and TableCell
> implementation which do not count the row editability in:
> [JDK-8268295](https://bugs.openjdk.java.net/browse/JDK-8268295)
fix looks good, verified new tests failing before and all tests passing after
left minor comments to the test methods
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
line 329:
> 327: public void testCellInUneditableRowIsNotEditable() {
> 328: table.setEditable(true);
> 329: TableRow row = new TableRow();
fix generic warning
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java
line 641:
> 639: row.setEditable(false);
> 640:
> 641: TreeTableColumn treeTableColumn = new TreeTableColumn();
fix generic warning
-
Changes requested by fastegal (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/529
Re: RFR: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
On Fri, 18 Jun 2021 10:10:42 GMT, Ajit Ghaisas wrote:
>> This PR enables Tree- and TableCells to also check the row editability when
>> an edit should happen. With this a Tree- or TableCell is not editable, when
>> the row where the cell is in is not.
>>
>> While this PR fixes the problem described in the ticket, it does not fix the
>> example.
>> This is due the example uses the **CheckBoxTableCell**, which is a
>> ready-to-use subclass of **TableCell**.
>>
>> While looking into this, I found out that multiple sub implementations still
>> have this issue, but the fix is not always the same, e.g. CheckBoxTableCell
>> should disable the CheckBox (in **updateItem**), while the
>> ChoiceBoxTableCell should check the row editability in the **startEdit**
>> method (like this PR does).
>>
>> I created a follow-up issues for fixing all the sub Tree- and TableCell
>> implementation which do not count the row editability in:
>> [JDK-8268295](https://bugs.openjdk.java.net/browse/JDK-8268295)
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java
> line 310:
>
>> 308: (table != null && !table.isEditable()) ||
>> 309: (column != null && !column.isEditable()) ||
>> 310: (row != null) && !row.isEditable()) {
>
> Incorrect Line "(row != null) && !row.isEditable())"
> Correction required "(row != null && !row.isEditable()))"
>
> Refer similar line which is rightly implemented in TreeTableCell.java.
darn .. you certainly have the better eyes :)))
-
PR: https://git.openjdk.java.net/jfx/pull/529
Re: RFR: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
On Sun, 6 Jun 2021 12:44:00 GMT, Marius Hanl wrote:
> This PR enables Tree- and TableCells to also check the row editability when
> an edit should happen. With this a Tree- or TableCell is not editable, when
> the row where the cell is in is not.
>
> While this PR fixes the problem described in the ticket, it does not fix the
> example.
> This is due the example uses the **CheckBoxTableCell**, which is a
> ready-to-use subclass of **TableCell**.
>
> While looking into this, I found out that multiple sub implementations still
> have this issue, but the fix is not always the same, e.g. CheckBoxTableCell
> should disable the CheckBox (in **updateItem**), while the ChoiceBoxTableCell
> should check the row editability in the **startEdit** method (like this PR
> does).
>
> I created a follow-up issues for fixing all the sub Tree- and TableCell
> implementation which do not count the row editability in:
> [JDK-8268295](https://bugs.openjdk.java.net/browse/JDK-8268295)
modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java line
310:
> 308: (table != null && !table.isEditable()) ||
> 309: (column != null && !column.isEditable()) ||
> 310: (row != null) && !row.isEditable()) {
Incorrect Line "(row != null) && !row.isEditable())"
Correction required "(row != null && !row.isEditable()))"
Refer similar line which is rightly implemented in TreeTableCell.java.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
line 327:
> 325:
> 326: @Test
> 327: public void testCellInUneditableRowIsNotEditable() {
I recommend adding tests for cell.startEdit() using all combinations of
TableView, TableColumn and TableRow editable states.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java
line 638:
> 636: @Test
> 637: public void testCellInUneditableRowIsNotEditable() {
> 638: tree.setEditable(true);
I recommend adding tests for cell.startEdit() using all combinations of
TreeTableView, TreeTableColumn and TreeTableRow editable states.
-
PR: https://git.openjdk.java.net/jfx/pull/529
