Withdrawn: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer
On Tue, 18 Jul 2023 06:05:06 GMT, Prasanta Sadhukhan wrote: > Due to transient datatype of scenePeer, it can become null which can result > in NPE in scenarios where scene is continuously been reset and set, which > warrants a null check, as is done in other places for the same variable. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jfx/pull/1178
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation [v2]
On Thu, 26 Oct 2023 18:16:26 GMT, Andy Goryachev wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Optimization
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 817:
>
>> 815: ComponentOrientation cor = this.getComponentOrientation();
>> 816: if (!cor.equals(ComponentOrientation.UNKNOWN)) {
>> 817:
>> getScene().setNodeOrientation(cor.equals(ComponentOrientation.RIGHT_TO_LEFT)
>> ?
>
> 1. I think getScene() might return null (sorry, did not see this immediately)
> 2. if possible, could we try limit code to one statement per line, i.e.
>
>
> Scene scene = getScene(); // it's a volatile, so better assign to a variable
> if(scene != null) {
> boolean rtl = cor.equals();
> scene.setNodeOrientation(rtl ? ...);
> }
ok..updated..
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1271#discussion_r1374027490
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation [v4]
> FX Nodes embedded in a Swing JFXPanel does not track the component > orientation and FX nodes remain unaffected when component orientation changes. > Fix made sure JavaFX scene embedded in a JFXPanel should inherit the value > from the JFXPanel. Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision: Code optimize - Changes: - all: https://git.openjdk.org/jfx/pull/1271/files - new: https://git.openjdk.org/jfx/pull/1271/files/d5a395c7..ad42e9d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1271=03 - incr: https://webrevs.openjdk.org/?repo=jfx=1271=02-03 Stats: 7 lines in 1 file changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1271.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1271/head:pull/1271 PR: https://git.openjdk.org/jfx/pull/1271
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation [v3]
> FX Nodes embedded in a Swing JFXPanel does not track the component > orientation and FX nodes remain unaffected when component orientation changes. > Fix made sure JavaFX scene embedded in a JFXPanel should inherit the value > from the JFXPanel. Prasanta Sadhukhan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8317836 - Optimization - 8317836: FX nodes embedded in JFXPanel need to track component orientation - Changes: - all: https://git.openjdk.org/jfx/pull/1271/files - new: https://git.openjdk.org/jfx/pull/1271/files/b3097c1b..d5a395c7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1271=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1271=01-02 Stats: 4666 lines in 104 files changed: 4095 ins; 364 del; 207 mod Patch: https://git.openjdk.org/jfx/pull/1271.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1271/head:pull/1271 PR: https://git.openjdk.org/jfx/pull/1271
Re: RFR: 8185831: Pseudo selectors do not appear to work in Node.lookupAll() [v11]
> **Issue:** > Using pseudo classes in programmatic query using Node.lookupAll() or > Node.lookup() gives unexpected results. > > **Cause:** > There is no check for checking the psuedo states matching in the applies() > method of SimpleSelector.java. So checking for "applies()" alone is not > sufficient in lookup() method. > > **Fix:** > Included an extra check for the psuedo states to match. Sai Pradeep Dandem has updated the pull request incrementally with one additional commit since the last revision: 8185831: Changes to doc as per review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1245/files - new: https://git.openjdk.org/jfx/pull/1245/files/1bd2b77b..cf92cecd Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1245=10 - incr: https://webrevs.openjdk.org/?repo=jfx=1245=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1245.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1245/head:pull/1245 PR: https://git.openjdk.org/jfx/pull/1245
Re: RFR: 8185831: Pseudo selectors do not appear to work in Node.lookupAll() [v10]
> **Issue:** > Using pseudo classes in programmatic query using Node.lookupAll() or > Node.lookup() gives unexpected results. > > **Cause:** > There is no check for checking the psuedo states matching in the applies() > method of SimpleSelector.java. So checking for "applies()" alone is not > sufficient in lookup() method. > > **Fix:** > Included an extra check for the psuedo states to match. Sai Pradeep Dandem has updated the pull request incrementally with one additional commit since the last revision: 8185831: Changes to doc as per review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1245/files - new: https://git.openjdk.org/jfx/pull/1245/files/96a891a5..1bd2b77b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1245=09 - incr: https://webrevs.openjdk.org/?repo=jfx=1245=08-09 Stats: 8 lines in 1 file changed: 2 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1245.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1245/head:pull/1245 PR: https://git.openjdk.org/jfx/pull/1245
Withdrawn: 8313556: Create implementation of NSAccessibilitySlider protocol
On Tue, 29 Aug 2023 10:10:54 GMT, Alexander Zuev wrote: > Create implementation for Slider and Stepper accessibility protocols. > Fix mapping. > Fix performAction parameter type in declaration. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jfx/pull/1226
Re: RFR: 8185831: Pseudo selectors do not appear to work in Node.lookupAll() [v9]
On Mon, 23 Oct 2023 23:11:53 GMT, Sai Pradeep Dandem wrote:
>> **Issue:**
>> Using pseudo classes in programmatic query using Node.lookupAll() or
>> Node.lookup() gives unexpected results.
>>
>> **Cause:**
>> There is no check for checking the psuedo states matching in the applies()
>> method of SimpleSelector.java. So checking for "applies()" alone is not
>> sufficient in lookup() method.
>>
>> **Fix:**
>> Included an extra check for the psuedo states to match.
>
> Sai Pradeep Dandem has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Minor change
> - Updated documentation in regards with pseudo states lookup
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1984:
> 1982: * a pseudo state "myPseudo", then to find all nodes with
> "myPseudo" state, the lookupAll method can be used as follows:
> 1983: * scene.lookupAll(".myStyle:myPseudo"); or
> scene.lookupAll(":myPseudo");
> 1984: *
Added explanation is very good! I would add one more thing - that if no pseudo
class is specified by the lookup selector, the result will contain nodes with
pseudo classes (that is, pseudo classes are ignored).
Minor note: should we be using {@code ... } instead of < code > ?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 2005:
> 2003: * @param selector The Selector.
> 2004: * @param results The results.
> 2005: * @return List of matching nodes. The returned value can be null.
minor: I don't think we should capitalize text in @ param and @ return
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1373630223
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1373632086
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation [v2]
On Thu, 26 Oct 2023 16:27:03 GMT, Prasanta Sadhukhan wrote: >> FX Nodes embedded in a Swing JFXPanel does not track the component >> orientation and FX nodes remain unaffected when component orientation >> changes. >> Fix made sure JavaFX scene embedded in a JFXPanel should inherit the value >> from the JFXPanel. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Optimization Please merge in the master branch, as it has new changes in native: Exception in thread "Thread-1" java.lang.UnsatisfiedLinkError: 'boolean com.sun.glass.ui.mac.MacApplication._isNormalTaskbarApp()' - PR Comment: https://git.openjdk.org/jfx/pull/1271#issuecomment-1781620688
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation [v2]
On Thu, 26 Oct 2023 16:27:03 GMT, Prasanta Sadhukhan
wrote:
>> FX Nodes embedded in a Swing JFXPanel does not track the component
>> orientation and FX nodes remain unaffected when component orientation
>> changes.
>> Fix made sure JavaFX scene embedded in a JFXPanel should inherit the value
>> from the JFXPanel.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Optimization
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 817:
> 815: ComponentOrientation cor = this.getComponentOrientation();
> 816: if (!cor.equals(ComponentOrientation.UNKNOWN)) {
> 817:
> getScene().setNodeOrientation(cor.equals(ComponentOrientation.RIGHT_TO_LEFT) ?
1. I think getScene() might return null (sorry, did not see this immediately)
2. if possible, could we try limit code to one statement per line, i.e.
Scene scene = getScene(); // it's a volatile, so better assign to a variable
if(scene != null) {
boolean rtl = cor.equals();
scene.setNodeOrientation(rtl ? ...);
}
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1271#discussion_r1373573244
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation [v2]
On Thu, 26 Oct 2023 15:31:02 GMT, Andy Goryachev wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Optimization
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 817:
>
>> 815: ComponentOrientation cor = this.getComponentOrientation();
>> 816: if (!cor.equals(ComponentOrientation.UNKNOWN)) {
>> 817: String orient =
>> cor.equals(ComponentOrientation.LEFT_TO_RIGHT)
>
> Why a String?? Should it be a simple boolean?
Yes, Optimized...
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1271#discussion_r1373435929
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation [v2]
> FX Nodes embedded in a Swing JFXPanel does not track the component > orientation and FX nodes remain unaffected when component orientation changes. > Fix made sure JavaFX scene embedded in a JFXPanel should inherit the value > from the JFXPanel. Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision: Optimization - Changes: - all: https://git.openjdk.org/jfx/pull/1271/files - new: https://git.openjdk.org/jfx/pull/1271/files/ae67f673..b3097c1b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1271=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1271=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1271.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1271/head:pull/1271 PR: https://git.openjdk.org/jfx/pull/1271
Re: RFR: 8185831: Pseudo selectors do not appear to work in Node.lookupAll() [v2]
On Thu, 19 Oct 2023 15:04:59 GMT, Andy Goryachev wrote: >> I must say I'm again baffled at how this is implemented. >> >> Red flag one: it uses a `LinkedList` which are known to be rarely the right >> choice, and I doubt this case is any different. >> >> Red flag two: a `List` is converted to a `Set`; being backed by a >> `LinkedList` means set type operations will be terribly slow if that list >> has any kind of size. >> >> Red flag three: The choice to return a `Set` in the public API seems to be >> only motivated to indicate there won't be any duplicates, which is a >> terrible reason. >> >> Red flag four: `UnmodifiableListSet` talks about insertion speed and hashing >> overhead, yet uses a `LinkedList` which are slower than `ArrayList` when it >> comes to insertion speed (not to mention that they require more object >> allocations and more memory(!)). > > You are right, @hjohn - LinkedList seems like a bad choice! It should be > HashSet from the very beginning, shouldn't it? > > But Set as a return value for lookups is correct, I think. > > We could (should?) fix it in a separate PR. created * [JDK-8318842](https://bugs.openjdk.org/browse/JDK-8318842) Node.lookupAll to use Set instead of List (Enhancement) - PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1373382130
Re: RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation
On Thu, 26 Oct 2023 09:34:17 GMT, Prasanta Sadhukhan
wrote:
> FX Nodes embedded in a Swing JFXPanel does not track the component
> orientation and FX nodes remain unaffected when component orientation changes.
> Fix made sure JavaFX scene embedded in a JFXPanel should inherit the value
> from the JFXPanel.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 817:
> 815: ComponentOrientation cor = this.getComponentOrientation();
> 816: if (!cor.equals(ComponentOrientation.UNKNOWN)) {
> 817: String orient =
> cor.equals(ComponentOrientation.LEFT_TO_RIGHT)
Why a String?? Should it be a simple boolean?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1271#discussion_r1373366241
Re: RFR: 8316419: [macos] Setting X/Y makes Stage maximization not work before show [v2]
On Sat, 21 Oct 2023 00:10:50 GMT, Martin Fox wrote: >> When a window is visible the maximized, iconified, and fullscreen properties >> are two-way streets; changes made in Java are sent on to the platform window >> and changes in the platform window are sent back into Java. >> >> When a window is hidden these properties (and others, like location and >> sizing information) are not sent on to the platform window until the window >> is made visible. In other words, the properties don't reflect the actual >> state of the window but the intended state after it's shown. >> >> There's a period when the window is transitioning from hidden to shown where >> the core Stage code is using the stored properties to configure the platform >> window and the platform window is simultaneously calling back in to update >> the properties. This was causing the intended state to be wiped out before >> it could be sent on to the platform window. >> >> The problem is specific to Mac because on that platform any change to the >> size or location of a window can cause it to enter or leave the maximized >> (zoomed) state. So just setting the location can cause the maximized flag to >> be altered. >> >> The proposed solution is to copy the intended state bits to Stage local >> variables to be used later in the configuration. > > Martin Fox has updated the pull request incrementally with one additional > commit since the last revision: > > Revert core changes. Fix Mac glass code so it reports maximized state > correctly. I'm also adding [JDK-8305675](https://bugs.openjdk.org/browse/JDK-8305675) which covers `setIconified`. The bogus `isZoomed` results were causing the core to think that the window had been maximized and then restored and that last step wiped out both the maximized and iconified properties. JDK-8305675 should be tested using the StartIconified test in tests/manual/stage/StartIconified.java. Despite what the test says the window should pop up briefly before iconifying. I have no idea why that isn't happening on Linux given the way the core code works. If you de-iconify the window it might be blank. That also happens on Linux and is related to a timing issue. I'll make sure a bug is entered against it. JDK-8305675 mentions exceptions raised by SceneChangeShouldNotFocusStageTest but that's not Mac-specific and has been spun off to another bug. - PR Comment: https://git.openjdk.org/jfx/pull/1258#issuecomment-1781305434
RFR: 8317836: FX nodes embedded in JFXPanel need to track component orientation
FX Nodes embedded in a Swing JFXPanel does not track the component orientation and FX nodes remain unaffected when component orientation changes. Fix made sure JavaFX scene embedded in a JFXPanel should inherit the value from the JFXPanel. - Commit messages: - 8317836: FX nodes embedded in JFXPanel need to track component orientation Changes: https://git.openjdk.org/jfx/pull/1271/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1271=00 Issue: https://bugs.openjdk.org/browse/JDK-8317836 Stats: 10 lines in 1 file changed: 10 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1271.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1271/head:pull/1271 PR: https://git.openjdk.org/jfx/pull/1271
Re: [External] : Re: Proof of concept pull request for Behavior API (PR 1265)
The normal procedure I think is also to first provide a JEP for review, before starting on the implementation... Given the doubts raised, feedback given and potential alternatives proposed, I don't see why you are still moving forward with your own proposal. The critiques I've given have been mostly hand waved with arguments that have no place in JEP evaluation (time restrictions, existing code already works this way, false equivalency with MVC pattern), and therefore have IMHO not been taken serious at all. This leaves me in the position of putting in a lot of work that will essentially be ignored as I feel an (internal) decision has already been reached, regardless of the feedback on the mailinglist. The (partial) proposal I've made, and also simpler proposals so that 3rd parties could do a keybinding implementation, should be sufficient to reconsider the current proposal that is being moved forward. I'll reiterate my problems with your proposal: - Introduces a lot of API for what is essentially the configuration of internal event handlers - The proposed API partially overlaps with the existing event handler API, meaning that some keys could be changed with just event handlers, while some can only be changed with the BaseBehavior API; it also provides for creating new functions and assigning them to keys, essentially a new (very limited) API for what was already possible in the much more flexible event handling API - Introduces the term "Behavior" in public API without clearly specifying what that is, nor showing enough forethought that may make it possible in the future to have public Behaviors - Introduces the term "InputMap" in public API, which is just an implementation detail of the internal event handlers - Doesn't address the real issue IMHO, which is that JavaFX Skins/Behaviors install their Event Handlers directly on Controls, mixing them with user event handlers leading to all sorts of unpredictable behavior due to call order and internal handlers essentially stealing and consuming events before the user has a chance to look at them (and thus blocking any 3rd party key alterations) which leads to the (false) need to change key bindings and Behaviors directly... So if you want me to work on such a proposal, fully fleshing it out, I would like to know if it will be given consideration. I would also like some more feedback on what is already there, as I think it is sufficient to decide if a full proposal is worth it or not. My proposals in short: 1. - Fix the issues with Events being stolen before users can get a them - Users should be able to have priority on Events, Michael Strauss already has a PR that fixes the issue in part - Events should not be consumed when not used (navigation does this) as this precludes the user being able to change their meaning - Even better would be if internal event handlers were isolated and did not mix themselves with user event handlers at all The above can be done separately, and should already make it possible to do a lot of things that were close to impossible before when it comes to changing key handling, but certainly not everything. - Building on top of the improved event handling system, introduce a flag to indicate an event is not to be consumed by internal event handlers These two together can form the basis for a 3rd party Behavior implementation as standard behavior can be prevented from occurring. It leaves platform dependent behavior to be addressed by such a 3rd party / user implementation as it is a very low level API. Any key remapping logic would be provided by the 3rd party API. 2. I also have a more fleshed out alternative proposal that attempts to introduce Behaviors into JavaFX as a first class concept, instead of a potential 3rd party add-on. Recap: - Introduce a Behavior interface with a single method "install" to be called by a Control - The "install" method is provided a context object, BehaviorContext. This indirects any changes the Behavior can make to a Control, so the Control is fully aware of all changes and can uninstall them without further co-operation from the behavior. - The BehaviorContext provides low level functions to add/remove event handlers and listeners, but can also provide higher level functions (in perhap a later PR) to allow for some kind of control provided input map system - Standard Behaviors can be made public and can be easily subclassed or composed as they need not have any state. State is tracked inside the behavorial installed listeners and handlers themselves (either directly or by referring to some shared State object). - Clear separation of concerns; Behaviors, a resuable concept that can be applied to a control; BehaviorContext, manages behavior lifecycle by abstracting away direct Control access; behavior state management left up to the implementation and created (on demand and as needed) when "install" is
Re: RFR: 8316419: [macos] Setting X/Y makes Stage maximization not work before show [v2]
On Sat, 21 Oct 2023 00:10:50 GMT, Martin Fox wrote: >> When a window is visible the maximized, iconified, and fullscreen properties >> are two-way streets; changes made in Java are sent on to the platform window >> and changes in the platform window are sent back into Java. >> >> When a window is hidden these properties (and others, like location and >> sizing information) are not sent on to the platform window until the window >> is made visible. In other words, the properties don't reflect the actual >> state of the window but the intended state after it's shown. >> >> There's a period when the window is transitioning from hidden to shown where >> the core Stage code is using the stored properties to configure the platform >> window and the platform window is simultaneously calling back in to update >> the properties. This was causing the intended state to be wiped out before >> it could be sent on to the platform window. >> >> The problem is specific to Mac because on that platform any change to the >> size or location of a window can cause it to enter or leave the maximized >> (zoomed) state. So just setting the location can cause the maximized flag to >> be altered. >> >> The proposed solution is to copy the intended state bits to Stage local >> variables to be used later in the configuration. > > Martin Fox has updated the pull request incrementally with one additional > commit since the last revision: > > Revert core changes. Fix Mac glass code so it reports maximized state > correctly. I can confirm this also solves JDK-8255835 - both MaximizeUndecorated test and the test program attached to the issue work as they are supposed to after these changes on both Mac and Linux. **EDIT:** Right, I forgot only authors can do that. @beldenfox could you add JDK-8255835? - PR Comment: https://git.openjdk.org/jfx/pull/1258#issuecomment-1780687999
Re: Prioritized event handlers
Hi Michael,
Yeah, I also think it is an oversight, or may even be qualified as a
bug. Now you have to be really careful to start event handlers with a
`if (!event.isConsumed())` check, just in case there is another handler
that is before you at the same level that may have consumed the event...
There docs are really light on details, and only this part seems
relevant and could be a bit ambigious depending on what "propagation"
would mean here (to another handler or to a different eventhandler manager):
/**
* Marks this {@code Event} as consumed. This stops its further propagation.
*/
publicvoidconsume() {
consumed= true;
}
--John
On 26/10/2023 04:06, Michael Strauß wrote:
- The consumed flag doesn't change any other behavior? Consumed events
only are passed to handlers at the same level, but are not bubbled up
(or captured down); having consumed events not be passed to event
handlers (even at the same level) seems like a more sane default for sure
I pulled on this string a little bit more and decided to remove the
`EventHandlerPolicy` record from the API, because I think the
`EventHandlerPolicy.handleConsumedEvents` toggle is not required. Have
a look at the PR to see the simplified API:
https://github.com/openjdk/jfx/pull/1266
While the current behavior of JavaFX events is strange with regards to
handler invocation for consumed events, it doesn't need to be solved
with the priority API. Here's an interesting thing: if I simply don't
invoke any event handlers for an event that is already consumed, no
tests break. This suggests to me that the current behavior is an
oversight rather than a purposeful implementation, and we could change
it in the future.
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Thu, 26 Oct 2023 00:31:52 GMT, Michael Strauß wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Migrate old tests to JUnit 5
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 414:
>
>> 412: final Insets insets = getInsets();
>> 413: if (width != -1) {
>> 414: width -= (insets.getLeft() + insets.getRight());
>
> Let's say we call `computeMinHeight(10)`, but the left and right insets are
> 20. This means that `width` is now -10, which probably means "ignore the
> value" (the spec isn't entirely clear about that, but -10 is not a valid
> width in any case).
>
> I think the following code might be better:
>
> if (width >= 0) {
> width = Math.max(0, width - insets.getLeft() - insets.getRight());
> }
The `width` value passed to `computeMinHeight` (if not -1) should be the result
of a call to `computeMinWidth(-1)` (depending on the result of
`getContentBias`; this is specified somewhere); if that function always
includes the insets, then subtracting them here should not result in a negative
value.
Of course, one can never be too careful.
I don't like the changing of the meaning of the `width` parameter here though
using parameter assignment.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1372711490
Integrated: 8205067: Resizing window with TextField hides text value
On Tue, 17 Oct 2023 12:15:14 GMT, Karthik P K wrote: > Because of a missing conditional check in the `updateTextPos()`, the > `textTranslateX` value was not getting updated when TextField size was > changed as a result of resizing window. > > Updated the CENTER and LEFT cases in the `updateTextPos()` method to fix the > issue. > > The fix can be validated using MonkeyTester. > Steps to select TextField option in Monkey Tester. > > - Open the MonkeyTester app and select TextField from the left option pane. > - Select Long from Text selection option and TOP_LEFT from Alignment dropdown. > - After above step, follow the steps given in the bug to validate the fix in > monkey tester. This pull request has now been integrated. Changeset: 9b93c962 Author:Karthik P K URL: https://git.openjdk.org/jfx/commit/9b93c962f45e5cf0b3a9f1090f307603be130a0e Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod 8205067: Resizing window with TextField hides text value Reviewed-by: angorya - PR: https://git.openjdk.org/jfx/pull/1263
Re: RFR: 8205067: Resizing window with TextField hides text value [v2]
On Wed, 25 Oct 2023 15:28:14 GMT, Andy Goryachev wrote: > Can you provide an automated test for this fix? I agree on the points shared by @andy-goryachev-oracle on this. I also tried to write both unit test and system test but couldn't succeed because the issue could not be reproduced in the unit test framework and there is no convenient way to figure out the actually text position on changing the text field size in system test. Monkey tester seems to test many scenarios related to this bug. Created [JDK-8318860](https://bugs.openjdk.org/browse/JDK-8318860) to add wider range of tests as suggested above. - PR Comment: https://git.openjdk.org/jfx/pull/1263#issuecomment-1780465816
