Re: RFR: 8214699: Node.getPseudoClassStates must return the same instance on every call [v2]
On Mon, 22 Jun 2020 11:50:05 GMT, Jeanette Winzenburg
wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review corrections
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9375:
>
>> 9374: public final ObservableSet getPseudoClassStates() {
>> 9375:
>> 9376: return
>> FXCollections.unmodifiableObservableSet(pseudoClassStates);
>
> missing override annotation (says my IDE :) - not entirely certain about
> guidelines here: it was missing before as
> well, but now might be a good time to fix it (except if we generally leave
> such cleanup to a dedicated cleanup commit)
Corrected in the next commit, please take a look.
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9371:
>
>> 9370: final ObservableSet unmodifiablePseudoClassStates =
>> 9371: FXCollections.unmodifiableObservableSet(pseudoClassStates);
>> 9372: /**
>
> the unmodifiable field can be private, or is there any reason for being
> package?
It is not required to be package, changed it to private.
-
PR: https://git.openjdk.java.net/jfx/pull/253
Integrated: 8214699: Node.getPseudoClassStates must return the same instance on every call
On Thu, 18 Jun 2020 16:30:42 GMT, Ambarish Rapte wrote: > Node.getPseudoClassStates() returns a new UnmodifiableObservableSet of > PseudoClassState on each call. So in order to > listen to any changes in this set, user must call the method > Node.getPseudoClassStates() only once and keep a strong > reference to the returned UnmodifiableObservableSet. > > So the fix is that the method Node.getPseudoClassStates() should return the > same UnmodifiableObservableSet on every > call. As the returned set is an UnmodifiableObservableSet, it will not have > any impact on it's usage. > > Added a small unit test. and, > Altered(minor) a test which was modified in > past(https://github.com/openjdk/jfx/commit/0ac98695a1b11443c342fad4f009d6e03a052522) > (https://github.com/openjdk/jfx/commit/62323e0a9c5817b33daa262d6914eba0e8d274ff) > along with this method and should be > updated in view of this change. This pull request has now been integrated. Changeset: a5878e05 Author:Ambarish Rapte URL: https://git.openjdk.java.net/jfx/commit/a5878e05 Stats: 94 lines in 5 files changed: 7 ins; 84 del; 3 mod 8214699: Node.getPseudoClassStates must return the same instance on every call Reviewed-by: fastegal, kcr - PR: https://git.openjdk.java.net/jfx/pull/253
Re: RFR: 8214699: Node.getPseudoClassStates must return the same instance on every call [v2]
On Tue, 23 Jun 2020 10:33:05 GMT, Ambarish Rapte wrote: >> Node.getPseudoClassStates() returns a new UnmodifiableObservableSet of >> PseudoClassState on each call. So in order to >> listen to any changes in this set, user must call the method >> Node.getPseudoClassStates() only once and keep a strong >> reference to the returned UnmodifiableObservableSet. >> >> So the fix is that the method Node.getPseudoClassStates() should return the >> same UnmodifiableObservableSet on every >> call. As the returned set is an UnmodifiableObservableSet, it will not have >> any impact on it's usage. >> >> Added a small unit test. and, >> Altered(minor) a test which was modified in >> past(https://github.com/openjdk/jfx/commit/0ac98695a1b11443c342fad4f009d6e03a052522) >> (https://github.com/openjdk/jfx/commit/62323e0a9c5817b33daa262d6914eba0e8d274ff) >> along with this method and should be >> updated in view of this change. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > review corrections looks fine :) - Marked as reviewed by fastegal (Committer). PR: https://git.openjdk.java.net/jfx/pull/253
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Fri, 19 Jun 2020 23:04:51 GMT, Kevin Rushforth wrote:
>> Marked as reviewed by kcr (Lead).
>
> Pending a second reviewer.
>
> @aghaisas or @kleopatra can one of you review it?
confirmed test failures before and passing after the fix - impressive :)
While digging a bit (mainly around my [earlier
concerns](https://github.com/openjdk/jfx/pull/244#issuecomment-638702570) -
which still hold but could be discussed in
a follow-up issue :) I came up with a NPE thrown by the newly added code block
ins sort (the one walking up the parent
hierarchy).
@Test
public void testNPEOnSortAfterSetAll() {
// stand-alone setup
TreeTableView treeTableView = new TreeTableView<>();
TreeTableColumn col = new TreeTableColumn("column");
col.setSortType(ASCENDING);
col.setCellValueFactory(param -> new
ReadOnlyObjectWrapper(param.getValue().getValue()));
treeTableView.getColumns().add(col);
TreeItem root = new TreeItem("root");
root.setExpanded(true);
root.getChildren().addAll(
new TreeItem("Apple"),
new TreeItem("Orange"),
new TreeItem("Banana"));
treeTableView.setRoot(root);
// add expanded children
root.getChildren().forEach(child -> {
child.setExpanded(true);
String value = child.getValue();
for (int i = 1; i <= 3; i++) {
child.getChildren().add(new TreeItem<>(value + i));
}
});
assertEquals("sanity", 13, treeTableView.getExpandedItemCount());
TreeTableViewSelectionModel selectionModel =
treeTableView.getSelectionModel();
selectionModel.setSelectionMode(SelectionMode.MULTIPLE);
selectionModel.selectIndices(2, 5, 8, 10);
assertEquals(10, selectionModel.getSelectedIndex());
TreeItem lastRootChild = root.getChildren().get(2);
assertEquals("sanity: selectedItem child of last",
lastRootChild, selectionModel.getSelectedItem().getParent());
// replace children of last root child
List> childrenOfLastRootChild = new
ArrayList<>(lastRootChild.getChildren());
childrenOfLastRootChild.remove(0);
lastRootChild.getChildren().setAll(childrenOfLastRootChild);
treeTableView.sort();
}
(before the fix it throws an IOOB, plus the behavior completely rotten in a
visual test)
Did nothing to go to the bottom of this - looks like somehow the state of the
selection is (still?) broken after
setAll.
Which might bring us back to the replace != permutation (can't resist to try :)
actually there's no reason to special
case a replace with all same items against a replace with only some same items
(doing so introduces a discontinuity in
behavior) - we should either try to keep selected items or not.
-
PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8214699: Node.getPseudoClassStates must return the same instance on every call [v2]
On Tue, 23 Jun 2020 10:33:05 GMT, Ambarish Rapte wrote: >> Node.getPseudoClassStates() returns a new UnmodifiableObservableSet of >> PseudoClassState on each call. So in order to >> listen to any changes in this set, user must call the method >> Node.getPseudoClassStates() only once and keep a strong >> reference to the returned UnmodifiableObservableSet. >> >> So the fix is that the method Node.getPseudoClassStates() should return the >> same UnmodifiableObservableSet on every >> call. As the returned set is an UnmodifiableObservableSet, it will not have >> any impact on it's usage. >> >> Added a small unit test. and, >> Altered(minor) a test which was modified in >> past(https://github.com/openjdk/jfx/commit/0ac98695a1b11443c342fad4f009d6e03a052522) >> (https://github.com/openjdk/jfx/commit/62323e0a9c5817b33daa262d6914eba0e8d274ff) >> along with this method and should be >> updated in view of this change. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > review corrections Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/253
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Tue, 23 Jun 2020 10:07:37 GMT, Ajit Ghaisas wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove the un-required flag > > modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java > line 1856: > >> 1855: Callback, Boolean> sortPolicy = >> getSortPolicy(); >> 1856: if (sortPolicy == null) return; >> 1857: Boolean success = sortPolicy.call(this); > > Do we need to set sortingInProgress to false before returning from here? Yes, I think so. Good catch. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: OpenJFX: Follow HiDPI Scaling Settings in Fedora Workstation 32 (Gnome)
Dear Kevin, Dear Phil, Dear Developers, the bug is now listed as JDK-8248126 http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8248126 . Kind regards, wurstsemmel Am Freitag, den 19.06.2020, 13:29 +0200 schrieb wurstsem...@mailbox.org: > Dear Kevin, Dear Phil, > > thank you for your quick responses. I submitted a bug report > (internal > review ID: 9065523). Within the description I listed JDK-8137050 and > additionally bug reports from Cryptomator which describe the issue > also > in Ubuntu 20.04 LTS and pop!OS. > > Kind regards and thanks, > > wurstsemmel > > Am Donnerstag, den 18.06.2020, 09:37 -0700 schrieb Kevin Rushforth: > > On 6/18/2020 9:26 AM, Philip Race wrote: > > > Although we only support integer scaling on Linux you have 200% > > > which > > > should work. > > > So sounds like it could be a breaking change in Fedora, where it > > > passes around / defines > > > the scale in some new setting and doesn't set the old ones needed > > > for > > > compatibility. > > > > That could be. > > > > > > (either via gsettings or the GDI_SCALE env variable) > > > > > > GDK_SCALE, not GDI_SCALE > > > > Oops. :) Of course. > > > > -- Kevin > > > > > > > -phil > > > > > > > > > > > > On 6/18/20, 5:12 AM, Kevin Rushforth wrote: > > > > We will need a new bug ID for this (we never reuse a bug ID for > > > > which > > > > a commit has been pushed). You can file a new bug, mentioning > > > > JDK-8137050 in the Description, at: > > > > > > > > https://bugreport.java.com/ > > > > > > > > Question for other Linux users on this list: are any of you > > > > using > > > > a > > > > Linux machine with Hi-DPI scaling? Is it detecting it properly > > > > or > > > > do > > > > you have to set the scaling property (either via gsettings or > > > > the > > > > GDI_SCALE env variable) yourself? > > > > > > > > -- Kevin > > > > > > > > > > > > On 6/18/2020 1:51 AM, wurstsem...@mailbox.org wrote: > > > > > Dear OpenJFX Developers, > > > > > > > > > > on Fedora 32 Workstation (Gnome) the AppImage from > > > > > Cryptomator > > > > > (cryptomator.org) does not follow the system-wide scale > > > > > settings from > > > > > Gnome Settings. > > > > > > > > > > *System Setup > > > > > Linux: Fedora Workstation 32 (Gnome) > > > > > Hardware: Dell XPS 13 with HiDPI (200 % scaling) > > > > > Cryptomator version: 1.5.5-x86_64 AppImage > > > > > > > > > > *Expected Behavior > > > > > App window should scale according to system-wide settings. > > > > > > > > > > *Actual Behavior > > > > > App window scales at 100 %. > > > > > > > > > > *Two workarounds exist: > > > > > $gsettings set org.gnome.desktop.interface scaling-factor 2 > > > > > $GDK_SCALE=2 ./cryptomator-1.5.5-x86_64.AppImage > > > > > > > > > > According to the developers from Cryptomator (see > > > > > https://github.com/cryptomator/cryptomator/issues/1242#issuecomment-643095195 > > > > > > > > > > > > > > > ) this is a bug which was intended to be fixed with > > > > > https://bugs.openjdk.java.net/browse/JDK-8137050. However, > > > > > the > > > > > bug > > > > > still occurs in OpenJFX 14.0.1 and 11. > > > > > > > > > > I am happy to help if anything is unclear. Please re-open the > > > > > bug JDK- > > > > > 8137050 and make OpenJFX follow the system-wide scaling > > > > > settings in > > > > > Gnome. > > > > > > > > > > Thank you! > > > > > > > > > > wurstsemmel > > > > > > > > > > > > > > > > > > > > > > > > >
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Wed, 17 Jun 2020 11:38:45 GMT, Ambarish Rapte wrote: >> Issue: >> In TreeTableView, in case of Multiple selection mode, if nested items are >> selected, then TreeTableView does not >> retain/update the selection correctly when the tree items are >> permuted(either by `sort()` or by reordering using >> `setAll()`). Cause: >> >> 1. For permutation, the current implementation uses `TreeModificationEvent` >> to update the selection. >> 2. The indices from these TreeModificationEvents are not reliable. >> 3. It uses the non public `TreeTablePosition` constructor to create >> intermediate `TreeItem` positions, this constructor >> results in another unexpected TreeModificationEvent while one for sorting is >> already being processed. 4. In case of >> sorting, there can be multiple intermediate TreeModificationEvents >> generated, and for each TreeModificationEvent, the >> selection gets updated and results in selection change events being >> generated. 5. Each time a TreeItem is expanded or >> collapsed, the selection must be shifted, but shifting is not necessary in >> case of permutation. All these issues >> combine in wrong update of the selection. Fix: >> >> 1. On each TreeModificationEvent for permutation, for updating the >> selection, use index of TreeItem from the >> TreeTableView but not from the TreeModificationEvent. 2. Added a new non >> public TreeTablePosition constructor, which is >> almost a copy constructor but accepts a different row. 3. In case of >> sorting, send out the set of selection change >> events only once after the sorting is over. 4. In case of setAll, send out >> the set of selection change events same as >> before.(setAll results in only one TreeModificationEvent, which effectively >> results in only one set of selection change >> events). `shiftSelection()` should not be called in case of permutation i.e. >> call `if (shift != 0)` >> Verification: >> The change is very limited to updating of selection of TreeTableView items >> when the TreeItems are permuted, so the >> change should not cause any other failures. Added unit tests which fail >> before and pass after the fix. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > Remove the un-required flag modules/javafx.controls/src/main/java/javafx/scene/control/TreeTablePosition.java line 79: > 78: } > 79: > 80: // Copy-like constructor with a different row. Suggest to add // Not public API before this description. (Similar to the constructor above this newly added constructor) modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java line 1856: > 1855: Callback, Boolean> sortPolicy = > getSortPolicy(); > 1856: if (sortPolicy == null) return; > 1857: Boolean success = sortPolicy.call(this); Do we need to set sortingInProgress to false before returning from here? - PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8214699: Node.getPseudoClassStates must return the same instance on every call [v2]
On Mon, 22 Jun 2020 12:08:21 GMT, Jeanette Winzenburg
wrote:
>> modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line
>> 168:
>>
>>> 167: assertSame("getPseudoClassStates() should always return the
>>> same instance",
>>> 168: node.getPseudoClassStates(),
>>> node.getPseudoClassStates());
>>> 169: }
>>
>> To make this more clear, I recommend to store the expected value in a local
>> variable and then call assert.
>
> would suggest some more tests (overcautious maybe, be have seen horses vomit
> at unbelievable places ;)
>
> Test that the returned set
>
> - is indeed unmodifiable
> - not gc'ed
> - is a wrapper around the backing set
>
> Test code:
>
> @Test(expected=UnsupportedOperationException.class)
> public void testPseudoClassesUnmodifiable() {
> Node node = new Rectangle();
> node.getPseudoClassStates().add(PseudoClass.getPseudoClass("dummy"));
> }
>
> @Test
> public void testPseudoClassesNotGCed() {
> Node node = new Rectangle();
> WeakReference> weakRef = new
> WeakReference<>(node.getPseudoClassStates());
> ControlSkinFactory.attemptGC(weakRef);
> assertNotNull("pseudoClassStates must not be gc'ed", weakRef.get());
> }
>
> // requires additional method in NodeShim
> @Test
> public void testUnmodifiablePseudoClassStatesEqualsBackingStates() {
> Rectangle node = new Rectangle();
> PseudoClass pseudo = PseudoClass.getPseudoClass("somepseudo");
> node.pseudoClassStateChanged(pseudo, true);
> assertEquals(1, node.getPseudoClassStates().size());
> assertEquals(NodeShim.pseudoClassStates(node).size(),
> node.getPseudoClassStates().size());
> assertTrue(NodeShim.pseudoClassStates(node).contains(pseudo));
> assertTrue(node.getPseudoClassStates().contains(pseudo));
> }
Corrected the existing test and added these new tests, with minor changes.
ControlSkinFactory is a file from control tests. So have added a copy of
`attemptGC()` method in
`test.javafx.scene.shape.TestUtils` class. The class does not seem to be a
perfect location for `attemptGC()`. But
wanted to avoid a new util kind off class just for this one method.
-
PR: https://git.openjdk.java.net/jfx/pull/253
Re: RFR: 8214699: Node.getPseudoClassStates must return the same instance on every call [v2]
> Node.getPseudoClassStates() returns a new UnmodifiableObservableSet of > PseudoClassState on each call. So in order to > listen to any changes in this set, user must call the method > Node.getPseudoClassStates() only once and keep a strong > reference to the returned UnmodifiableObservableSet. > > So the fix is that the method Node.getPseudoClassStates() should return the > same UnmodifiableObservableSet on every > call. As the returned set is an UnmodifiableObservableSet, it will not have > any impact on it's usage. > > Added a small unit test. and, > Altered(minor) a test which was modified in > past(https://github.com/openjdk/jfx/commit/0ac98695a1b11443c342fad4f009d6e03a052522) > (https://github.com/openjdk/jfx/commit/62323e0a9c5817b33daa262d6914eba0e8d274ff) > along with this method and should be > updated in view of this change. Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision: review corrections - Changes: - all: https://git.openjdk.java.net/jfx/pull/253/files - new: https://git.openjdk.java.net/jfx/pull/253/files/2c4cbfba..7eac1d30 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/253/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/253/webrev.00-01 Stats: 77 lines in 4 files changed: 75 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/253.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/253/head:pull/253 PR: https://git.openjdk.java.net/jfx/pull/253
