Integrated: 8252060: gstreamer fails to build with gcc 10
On Tue, 25 Aug 2020 01:00:31 GMT, Alexander Matveev wrote: > Fixed by defining GST_API as GST_EXPORT for gcc compiler as per Kevin > proposed solution. On Windows we do not need to > define GST_API, since we using .def file to export symbols. This pull request has now been integrated. Changeset: 7a4bd9b2 Author:Alexander Matveev URL: https://git.openjdk.java.net/jfx/commit/7a4bd9b2 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod 8252060: gstreamer fails to build with gcc 10 Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/287
Re: Bulk request to backport 14 fixes to 11-dev for 11.0.9
Approved. - Johan On Tue, Aug 25, 2020 at 10:03 PM Kevin Rushforth wrote: > Hi Johan, > > I request approval to backport the following 14 fixes to openjfx11: > > JDK-8247963: Update SQLite to version 3.32.3 > JDK-8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars > JDK-8247947: Build DirectShow Samples (Base Classes) from source checked > into repo > JDK-8245284: Update to 610.1 version of WebKit > JDK-8249839: Cherry pick GTK WebKit 2.28.3 changes > JDK-8242507: Add support for Visual Studio 2019 > JDK-8244487: One Windows 10 SDK file missing from FX build > JDK-8191758: Match WebKit's font weight rendering with JavaFX (to keep > native WebKit in sync) > JDK-8239095: Upgrade libFFI to the latest 3.3 version > JDK-8193800: TreeTableView selection changes on sorting > JDK-8246204: No 3D support for newer Intel graphics drivers on Linux > JDK-8248490: [macOS] Undecorated stage does not minimize > JDK-8250238: Media fails to load libav 58 library when using modules > from maven central > JDK-8252326: Update to Visual Studio 2017 version 15.9.24 > > -- Kevin > >
Bulk request to backport 14 fixes to 11-dev for 11.0.9
Hi Johan, I request approval to backport the following 14 fixes to openjfx11: JDK-8247963: Update SQLite to version 3.32.3 JDK-8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars JDK-8247947: Build DirectShow Samples (Base Classes) from source checked into repo JDK-8245284: Update to 610.1 version of WebKit JDK-8249839: Cherry pick GTK WebKit 2.28.3 changes JDK-8242507: Add support for Visual Studio 2019 JDK-8244487: One Windows 10 SDK file missing from FX build JDK-8191758: Match WebKit's font weight rendering with JavaFXÂ (to keep native WebKit in sync) JDK-8239095: Upgrade libFFI to the latest 3.3 version JDK-8193800: TreeTableView selection changes on sorting JDK-8246204: No 3D support for newer Intel graphics drivers on Linux JDK-8248490: [macOS] Undecorated stage does not minimize JDK-8250238: Media fails to load libav 58 library when using modules from maven central JDK-8252326: Update to Visual Studio 2017 version 15.9.24 -- Kevin
Re: RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [v6]
On Wed, 19 Aug 2020 13:06:34 GMT, Ambarish Rapte wrote:
>> The issue occurs because the key events are consumed by the `ListView` in
>> `Popup`, which displays the items.
>> This is a regression of
>> [JDK-8077916](https://bugs.openjdk.java.net/browse/JDK-8077916). This change
>> aadded several
>> `KeyMapping`s for focus traversals to `ListView`, which consume the `Left`,
>> `Right` and `Ctrl+A` key events.
>> Fix:
>> 1. Remove the four focus traversal arrow `KeyMapping`s from
>> `ListViewBehavior`.
>> 2. Add the `Ctrl + A` `KeyMapping` to `ListViewBehavior` only if the
>> `ListView`'s selection mode is set to
>> `SelectionMode.MULTIPLE`. `ComboBox` uses the `ListView` with
>> `SelectionMode.SINGLE` mode.
>> Change unrelated to fix:
>> `ComboBoxListViewBehavior` adds `KeyMapping` for `Up` and `Down` keys, which
>> are not invoked when the `ComboBox` popup
>> is showing. When the popup is shown, the Up and Down key events are handled
>> by the `ListView` and the `KeyMapping` code
>> from `ComboBoxListViewBehavior` does not get executed. These KeyMapping are
>> removed. However this change is not needed
>> for the fix. But this seems to be dead code. Verification:
>> Added new unit tests to verify the change.
>> Also verified that the behavior ListView behaves same before and after this
>> change.
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change approach from removing to excluding
fix and tests look okay (added minor inline comments), verified that the tests
for the fix are failing before and
passing after, those added for completeness are fine also.
As noted in one of my comments (again? :), I don't like underscores .. not even
in test methods - but as they are wide
spread nothing to really complain about: just be consistent with yourself :)
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 181:
> 180:
> 181: @Test public void test_SwitchingSelectionModel() {
> 182: ListView listView = new ListView<>();
maybe the name could be improved by adding _what_ it's testing: something like
testCtrlAWhenSwitchingSelectionModel?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 1509:
> 1508:
> 1509: @Test public void test_switchingSelectionMode() {
> 1510: ListView listView = new ListView<>();
same as naming suggestion to the other Switching test above
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 1536:
> 1535: assertEquals(4, sm.getSelectedItems().size());
> 1536:
> 1537: sl.dispose();
wondering about these repeated blocks? The flow is single: (default on start)
-> multiple -> single -> multiple. Looks
like the last two are repeating the first two .. what do I overlook?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2055:
> 2054: .observableArrayList("Item1", "Item2"));
> 2055: listView.setCellFactory(TextFieldListCell.forListView());
> 2056: StageLoader sl = new StageLoader(listView);
hmm .. why the textFieldListCell? It's just a plain listCell if not in editing
state .. or not?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java
line 1344:
> 1343:
> 1344: @Test public void test_EditorKeyInputsWhenPopupIsShowing() {
> 1345: final ComboBox cb = new
> ComboBox<>(FXCollections.observableArrayList("a", "b", "c"));
minor nit: naming is inconsistent to the other added test below
(testExcludeKeyMappingsForComboBoxEditor) - either use
an underscore or not (my personal preference is to not use them at all,
following general java naming conventions also
in tests .. but definitely not overly important :)
-
Changes requested by fastegal (Committer).
PR: https://git.openjdk.java.net/jfx/pull/172
Re: RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [v6]
On Tue, 25 Aug 2020 13:54:14 GMT, Jeanette Winzenburg
wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> change approach from removing to excluding
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
> line 1509:
>
>> 1508:
>> 1509: @Test public void test_switchingSelectionMode() {
>> 1510: ListView listView = new ListView<>();
>
> same as naming suggestion to the other Switching test above
if you prefer not to, the capitalizing should be consistent - here it's
test_switch, above it's test_Switch :)
-
PR: https://git.openjdk.java.net/jfx/pull/172
Re: RFR: 8236651: Simplify and update glass gtk backend
On Wed, 29 Jul 2020 21:01:42 GMT, Tor (torbuntu) wrote: >> @Torbuntu Not to this PR, I don't want to delay it too much. But can be done >> (I just do not own a touch device >> currently). > > Sounds good! I have a few devices I'd be more than excited to test on, and > even help add the feature myself if I can > figure it out if time is a big issue? I would starting hooking gtk event signals (https://developer.gnome.org/gtk3/stable/GtkWidget.html), specially the "touch-event" to JavaFx events (probably need to add through JNI). Should be simple. Contact me at thiago.sayao (gmail). - PR: https://git.openjdk.java.net/jfx/pull/77
Re: RFR: 8252060: gstreamer fails to build with gcc 10
On Tue, 25 Aug 2020 01:00:31 GMT, Alexander Matveev wrote: > Fixed by defining GST_API as GST_EXPORT for gcc compiler as per Kevin > proposed solution. On Windows we do not need to > define GST_API, since we using .def file to export symbols. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/287
Integrated: 8251555: Remove unused focusedWindow field in glass Window to avoid leak
On Sat, 22 Aug 2020 19:39:33 GMT, Kevin Rushforth wrote: > This is a follow-up to > [JDK-8241840](https://bugs.openjdk.java.net/browse/JDK-8241840) that removes > an unused static > `focusedWindow` field and its associated getter and setter from the > platform-independent glass Window class. This is > entirely unused by any of the glass platforms. The Monocle platform keeps > track of the `focusedWindow` attribute > itself. The existing `test.javafx.stage.FocusedWindowNativeTest` automated > test, which was added as part of > [JDK-8241840](https://bugs.openjdk.java.net/browse/JDK-8241840), was failing > fairly consistently on one of our test > machines before this fix, and now passes. This pull request has now been integrated. Changeset: ddf8814e Author:Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/ddf8814e Stats: 15 lines in 1 file changed: 15 ins; 0 del; 0 mod 8251555: Remove unused focusedWindow field in glass Window to avoid leak Reviewed-by: arapte - PR: https://git.openjdk.java.net/jfx/pull/286
RFR: 8251941: ListCell: visual artifact when items contain null values
The issue describes the makroscopic effect/s, namely content showing in cells that are off range. The base reason is missing cleanup of the cell on transition from not-empty to empty when the old item is a null contained in the items. Fixed by changing the logic to cope with that special case. Added tests that fail before and pass after the fix. - Commit messages: - 8251941: ListCell: visual artifact when items contain null values Changes: https://git.openjdk.java.net/jfx/pull/288/files Webrev: https://webrevs.openjdk.java.net/jfx/288/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251941 Stats: 90 lines in 2 files changed: 87 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/288.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/288/head:pull/288 PR: https://git.openjdk.java.net/jfx/pull/288
Re: RFR: 8251555: Remove unused focusedWindow field in glass Window to avoid leak
On Sat, 22 Aug 2020 19:39:33 GMT, Kevin Rushforth wrote: > This is a follow-up to > [JDK-8241840](https://bugs.openjdk.java.net/browse/JDK-8241840) that removes > an unused static > `focusedWindow` field and its associated getter and setter from the > platform-independent glass Window class. This is > entirely unused by any of the glass platforms. The Monocle platform keeps > track of the `focusedWindow` attribute > itself. The existing `test.javafx.stage.FocusedWindowNativeTest` automated > test, which was added as part of > [JDK-8241840](https://bugs.openjdk.java.net/browse/JDK-8241840), was failing > fairly consistently on one of our test > machines before this fix, and now passes. Looks good to me. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/286
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v3]
> Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: Reverted changes for class Selector and ShapeConverter as per review comment - Changes: - all: https://git.openjdk.java.net/jfx/pull/283/files - new: https://git.openjdk.java.net/jfx/pull/283/files/16d8b9ad..38a7551d Webrevs: - full: https://webrevs.openjdk.java.net/jfx/283/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/283/webrev.01-02 Stats: 14 lines in 2 files changed: 0 ins; 14 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/283.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/283/head:pull/283 PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Wed, 19 Aug 2020 00:12:01 GMT, Kevin Rushforth wrote: > I think that two of the classes have implicit constructors that are there by > accident. Once we get agreement, I'll file > a follow-on bug for those, and those changes should be reverted. I finished reviewing the rest of the classes and I had no additional comments about them, so I agree about deprecating for removal those 2 classes' constructors. > As for the other comments, I would prefer a follow-up bug for any doc cleanup > that isn't part of adding in an explicit > constructor. Tempting as it might be to fix it, it seems out of scope. That's fine. I left inline comments about those. > That leaves the question about the wording. The more I think about it the > more I like what the JDK did as opposed to > what we did. The question then becomes: if we agree that it's a better > pattern, do we adopt it here and then clean it > up for the other two batches or just leave it as is for now, and file one > cleanup bug for later. I'd like to hear from > @aghaisas and @nlisker Since it will require an additional cleanup issue anyway, I don't think it matters when we do it, but since we're here I see no reason not to start already. - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Tue, 18 Aug 2020 23:41:23 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/application/Preloader.java line
>> 121:
>>
>>> 120: public Preloader() {
>>> 121: }
>>> 122:
>>
>> Not sure that "default" means anything here. I don't see any configuration.
>
> Right, but isn't that true of most of the classes, including those in the two
> related bugs that were fixed?
>
> Worth noting is that the JDK chose different language for abstract classes
> than concrete ones. For abstract classes,
> they just use the following language:
> * Constructor for subclasses to call.
>
> And for concrete ones:
>
> Constructs a {@code Foo}.
>
> This gets around the problem of whether or not `default` adds any useful
> information since it is the only constructor.
> Not saying we should adopt this now, but just adding another data point.
I didn't look closely at whether "default" was suitable in the previous cases.
I like the JDK's wording, but in cases
where there are constructors that allow configuration, the empty constructor
could use "default", though specifying
what the default values are is more beneficial anyway.
-
PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Tue, 18 Aug 2020 23:47:07 GMT, Kevin Rushforth wrote: >> modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 83: >> >>> 82: >>> 83: /** >>> 84: * There is only one PseudoClass instance for a given pseudoClass. >> >> 1. Is having a public constructor for this class a good idea? Instances are >> created by a factory method. >> 2. Both methods in this class need documentation update. `getPseudoClass` >> has a poor method description and the >> formatting of the `@return` tag is wrong (lowercase and no period). >> `getPseudoClassName` is missing a description. > > Yes, this constructor is needed. `PseudoClass` is abstract, so it's > constructor is just there for subclasses to call > (note that for a constructor of an abstract class, `public` and `protected` > mean the same thing). > As for the other methods in the class, I agree that they should be > changed...but in a follow-up issue. Maybe we can add the method docs fixes to [JDK-8250590](https://bugs.openjdk.java.net/browse/JDK-8250590)? - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Tue, 18 Aug 2020 23:31:42 GMT, Kevin Rushforth wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/TableFocusModel.java >> line 51: >> >>> 50: >>> 51: /** >>> 52: * Causes the item at the given index to receive the focus. >> >> Please add a missing `)` for the class docs on line 30. > > The change to the class header seems unrelated. It would be a good thing to > fix, but could be done as part of a > follow-on doc cleanup issue. Alright, we could add them to this version's doc fixes issue. - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Wed, 19 Aug 2020 00:12:01 GMT, Kevin Rushforth wrote: >> Bhawesh Choudhary has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Marked few class constructor as deprecated as per review > > I think that two of the classes have implicit constructors that are there by > accident. Once we get agreement, I'll file > a follow-on bug for those, and those changes should be reverted. > As for the other comments, I would prefer a follow-up bug for any doc cleanup > that isn't part of adding in an explicit > constructor. Tempting as it might be to fix it, it seems out of scope. > That leaves the question about the wording. The more I think about it the > more I like what the JDK did as opposed to > what we did. The question then becomes: if we agree that it's a better > pattern, do we adopt it here and then clean it > up for the other two batches or just leave it as is for now, and file one > cleanup bug for later. I'd like to hear from > @aghaisas and @nlisker @bhaweshkc You can't deprecate the constructors in this PR since it will need a separate CSR. You should revert the changes on these classes completely and file a new bug (and CSR) dealing with deprecations. - PR: https://git.openjdk.java.net/jfx/pull/283
