Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 22:18:58 GMT, John Hendrikx wrote: >> This PR does not add any additional feature. It restores the behavior of >> jfx16, where any character is acceptable as a mnemonic, and the first >> mnemonic is selected. > > Okay, the wording of the JBS ticket suggested something else, but I see it is > just a regression fix. Yes, exactly. There were two bugs introduced with the previous fix: First, digits or other symbols are no longer recognized as mnemonics. Second, the last `_` is used rather than the first. It looks like this fixes both problems. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Sat, 30 Oct 2021 14:46:50 GMT, Kevin Rushforth wrote: > > Perhaps it would also be a good idea to move these files to the user's > > temporary directory, so that they can be cleaned up by the system. > > Otherwise they probably won't be removed in most cases when the user > > removes the application that created them. > > Maybe. If so, this would need to be done as part of a separate bug fix. I don't think that would be needed. We currently use the same approach as maven, and have a "permanent" cache for artifacts. There is a difference between the JavaFX libraries and application-specific components. The JavaFX jars are not linked to a specific application, but can be used by a number of applications (at build time as well as at runtime). I agree that application-specific components need to be removed (when possible) from the user's storage, but that is at runtime, and only when these components are only used by the specific application. - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Sat, 30 Oct 2021 14:48:14 GMT, Kevin Rushforth wrote: > > If we want to be really safe we can replace a + by - or something else, but > > not sure if really needed. > > I had the same thought as a possibility, but I'm also not sure it is needed. > @johanvos what do you think? I don't think it's needed. Either there is a potential issue with a "+" or there is not. If there is not an issue, then it is only confusing to replace the "+" with another character. Hence, I am +1 on the PR as it is now. - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Wed, 27 Oct 2021 16:01:10 GMT, Marius Hanl wrote: > If we want to be really safe we can replace a + by - or something else, but > not sure if really needed. I had the same thought as a possibility, but I'm also not sure it is needed. @johanvos what do you think? - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 14:18:42 GMT, Michael Strauß wrote: > there is no need for the class rename, is there? > > Even though it's formally internal api, I think we shouldn't do code breaking > changes except if there's a very compelling reason - there are too many apps > out in the wild that use internal api. Well, internal interfaces aren't API. With JDK 9 - 16 it requires relaxing encapsulation to get at it. As of JDK 17, it requires an explicit export of the package to access it. Having said that, since this is a fix we are likely to want to backport to at least JavaFX 17.0.X, I think we should minimize the scope of the fix. So I agree with @kleopatra (albeit for a different reason) that we shouldn't make this change. > I don't know how `TextBinding` describes in any way what this class is doing. > Naming is important, and I think `MnemonicParser`, on the other hand, > describes exactly what is going on. Agreed. > I would disagree that treating internal APIs as if they were public APIs is a > good place to be. Completely agree. > Cleaning up and refactoring internal APIs is important for the long-term > health of a codebase. Yes, but I would prefer to see it as a separate fix, rather than as part of a (critical) regression bug fix. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> This PR fixes an issue with mnemonic parsing by removing the restriction >> that a mnemonic symbol must be a letter. Now, it can be any character except >> whitespace. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc there is no need for the class rename, is there? Even though it's formally internal api, I think we shouldn't do code breaking changes except if there's a very compelling reason - there are too many apps out in the wild that use internal api. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Tue, 26 Oct 2021 12:09:19 GMT, Marius Hanl wrote: > This problem can happen when using multiple instances with different jfx > early access (ea) versions. > > Example: > Instance 1 uses 18-ea+4 and Instance 2 uses 18-ea+1. > Instance 1 is started (and will cache and use libraries), then instance 2. > Now instance 2 detects via a hash comparison that the cached libraries are > not the same as the supplied ones. > With this information instance 2 tries to delete the old libraries but fails > while doing so (as instance 1 uses them currently) and will terminate right > after. > > The problem here is that instance 1 and 2 are using the same cache folder: > **18-ea**. This is because the `NativeLibLoader` uses the `javafx.version` > property for determining the folder name, which in case of an ea version will > always be **18-ea** (for all ea versions starting with 18 obviously). > > Fix as also mentioned in the ticket is to use the `javafx.runtime.version` > property instead. > With this the folders will be named 18-ea+1 and 18-ea+4. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/654
Integrated: 8222455: JavaFX error loading glass.dll from cache
On Tue, 26 Oct 2021 12:09:19 GMT, Marius Hanl wrote: > This problem can happen when using multiple instances with different jfx > early access (ea) versions. > > Example: > Instance 1 uses 18-ea+4 and Instance 2 uses 18-ea+1. > Instance 1 is started (and will cache and use libraries), then instance 2. > Now instance 2 detects via a hash comparison that the cached libraries are > not the same as the supplied ones. > With this information instance 2 tries to delete the old libraries but fails > while doing so (as instance 1 uses them currently) and will terminate right > after. > > The problem here is that instance 1 and 2 are using the same cache folder: > **18-ea**. This is because the `NativeLibLoader` uses the `javafx.version` > property for determining the folder name, which in case of an ea version will > always be **18-ea** (for all ea versions starting with 18 obviously). > > Fix as also mentioned in the ticket is to use the `javafx.runtime.version` > property instead. > With this the folders will be named 18-ea+1 and 18-ea+4. This pull request has now been integrated. Changeset: 6c881063 Author:Marius Hanl Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/6c8810634ec63af8116dd978656805b985eec800 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8222455: JavaFX error loading glass.dll from cache Reviewed-by: jvos, kcr - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Sat, 30 Oct 2021 14:54:16 GMT, Johan Vos wrote: > I don't think that would be needed. We currently use the same approach as > maven, and have a "permanent" cache for artifacts. For users, JavaFX comes as an invisible part of an application in 99% of cases. That's different from Maven, which is consciously installed by users either manually or as part of a software development suite. Most users of a JavaFX application did not consent to permanent changes to their system that persist even after they uninstall the application. > There is a difference between the JavaFX libraries and application-specific > components. No, not from the perspective of a user of an application that happens to use JavaFX. The same argument could be made for every third-party library used by an application, and you wouldn't want every one of those to make permanent changes to your system, too. - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 14:28:09 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - revert rename
>> - revert rename
>> - test for KeyCombination
>> - renamed TextBinding to MnemonicParser, refactored tests
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/MnemonicParser.java
> line 63:
>
>> 61: *
>> 62: */
>> 63: public class MnemonicParser {
>
> As mentioned in the comments, I prefer this rename to be reverted. It could
> be done separately as cleanup.
I created [JDK-8276206](https://bugs.openjdk.java.net/browse/JDK-8276206) to
track this.
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 14:49:24 GMT, Michael Strauß wrote:
>> This PR fixes an issue with mnemonic parsing by removing the restriction
>> that a mnemonic symbol must be a letter. Now, it can be any character except
>> whitespace.
>
> Michael Strauß has updated the pull request incrementally with four
> additional commits since the last revision:
>
> - revert rename
> - revert rename
> - test for KeyCombination
> - renamed TextBinding to MnemonicParser, refactored tests
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/MnemonicParser.java
line 63:
> 61: *
> 62: */
> 63: public class MnemonicParser {
As mentioned in the comments, I prefer this rename to be reverted. It could be
done separately as cleanup.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/MnemonicParser.java
line 194:
> 192: } else if (isExtendedMnemonic(s, i)) {
> 193: mnemonic = String.valueOf(s.charAt(i + 2));
> 194: mnemonicIndex = i;
Good catch. This is needed to make extended mnemonics work.
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> This PR fixes an issue with mnemonic parsing by removing the restriction >> that a mnemonic symbol must be a letter. Now, it can be any character except >> whitespace. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc The latest changes look good (aside from the renaming of the class, which could be done separately). I'll look at the refactored tests and do some more testing next week. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Fri, 29 Oct 2021 14:31:02 GMT, Michael Strauß wrote: > Perhaps it would also be a good idea to move these files to the user's > temporary directory, so that they can be cleaned up by the system. Otherwise > they probably won't be removed in most cases when the user removes the > application that created them. Maybe. If so, this would need to be done as part of a separate bug fix. - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
> This PR fixes an issue with mnemonic parsing by removing the restriction that > a mnemonic symbol must be a letter. Now, it can be any character except > whitespace. Michael Strauß has updated the pull request incrementally with four additional commits since the last revision: - revert rename - revert rename - test for KeyCombination - renamed TextBinding to MnemonicParser, refactored tests - Changes: - all: https://git.openjdk.java.net/jfx/pull/647/files - new: https://git.openjdk.java.net/jfx/pull/647/files/c4012056..5b5d720d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=647&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=647&range=02-03 Stats: 224 lines in 3 files changed: 146 ins; 72 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/647.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647 PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 14:12:17 GMT, Jeanette Winzenburg wrote: > there is no need for the class rename, is there? > > Even though it's formally internal api, I think we shouldn't do code breaking > changes except if there's a very compelling reason - there are too many apps > out in the wild that use internal api. I don't know how `TextBinding` describes in any way what this class is doing. Naming is important, and I think `MnemonicParser`, on the other hand, describes exactly what is going on. I would disagree that treating internal APIs as if they were public APIs is a good place to be. Cleaning up and refactoring internal APIs is important for the long-term health of a codebase. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Tue, 26 Oct 2021 12:09:19 GMT, Marius Hanl wrote: > This problem can happen when using multiple instances with different jfx > early access (ea) versions. > > Example: > Instance 1 uses 18-ea+4 and Instance 2 uses 18-ea+1. > Instance 1 is started (and will cache and use libraries), then instance 2. > Now instance 2 detects via a hash comparison that the cached libraries are > not the same as the supplied ones. > With this information instance 2 tries to delete the old libraries but fails > while doing so (as instance 1 uses them currently) and will terminate right > after. > > The problem here is that instance 1 and 2 are using the same cache folder: > **18-ea**. This is because the `NativeLibLoader` uses the `javafx.version` > property for determining the folder name, which in case of an ea version will > always be **18-ea** (for all ea versions starting with 18 obviously). > > Fix as also mentioned in the ticket is to use the `javafx.runtime.version` > property instead. > With this the folders will be named 18-ea+1 and 18-ea+4. Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac [v7]
On Mon, 18 Oct 2021 08:20:22 GMT, Florian Kirmaier
wrote:
>> When using Swing it's possible to generate a Deadlock.
>> It's related to the nested eventloop started in enterFullScreenExitingLoop
>> - and the RenderLock aquired when using setView in Scene.
>> Sample Programm and Threaddump are added to the ticket.
>>
>> Removing the nested loop fixes the Problem.
>> I hope this doesn't have any side effect - so far i don't know of any.
>
> Florian Kirmaier has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8273485
> Fixing toggle fullscreen!
What version of macOS did you test on? I was running on 10.15.7 (Catalina). I
just tried it on macOS 11.5 (Big Sur) and the exception does not happen, and on
10.14.6 (Mojave) where is does. To summarize, when calling `Platform.exit`
while in full-screen mode:
macOS 10.14.6 : exception
macOS 10.15.7 : exception
macOS 11.5 : OK
modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 1345:
> 1343: if (self->nativeFullScreenModeWindow)
> 1344: {
> 1345: [[self->nsView window] toggleFullScreen:self];
I see you added the call to `toggleFullScreen` back in, but you now pass `self`
to the method, where the previous code passed `nil`. Unless there is a
compelling reason why you need to change it, I recommend to restore _exactly_
the former code, such that there are no diffs:
[self->nativeFullScreenModeWindow
performSelector:@selector(toggleFullScreen:) withObject:nil];
-
PR: https://git.openjdk.java.net/jfx/pull/622
Re: RFR: 8275911: Keyboard doesn't show when tapping inside an iOS text input control
On Tue, 26 Oct 2021 11:07:50 GMT, Jose Pereda wrote: > After [JDK-8245053](https://bugs.openjdk.java.net/browse/JDK-8245053) for > Android, this PR applies the same approach on iOS: tapping on a text input > control on iOS shows the keyboard, which hides after the control loses the > focus. > > Now, both platforms have the same behaviour. > > Tested on an iOS device. Changes look OK to me. Since they are confined to iOS-specific files, I'll leave it to @johanvos to review. - PR: https://git.openjdk.java.net/jfx/pull/653
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v2]
On Thu, 22 Jul 2021 11:50:11 GMT, Florian Kirmaier wrote: >> After thinking about this issue for some time, I've now got a solution. >> I know put the scene in the state it is, before is was shown, when the >> dirtyNodes are unset, the whole scene is basically considered dirty. >> This has the drawback of rerendering, whenever a window is "reshown", but it >> restores sanity about memory behaviour, which should be considered more >> important. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8269907 > The bug is now fixed in a new way. Toolkit now supports registering > CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks. I took a closer look, and the current PR has at least one problem. The call to `synchronizeSceneNode` must only be done with the `renderLock` held. If you are using this in production, then it likely means you aren't hitting the cases where this would break. I recommend either my earlier suggestion of hooking into the existing listener, or else you will need to acquire the `renderLock`. If the latter, then one thing you will want to avoid is running both a regular sync pass and a cleanup pass in the same pulse. I see that you try to ensure that by only adding the listener if the scene is not showing, but since you might have to also check in the listener itself (not sure). - PR: https://git.openjdk.java.net/jfx/pull/584
Re: RFR: 8276179 removing unused font code - isInstalledFont
On Fri, 29 Oct 2021 15:10:35 GMT, Florian Kirmaier wrote: > removing dead code. > When looking into the font code, I've noticed that this code is no longer > used, so I thought I should make a PR with a minor cleanup. > [JDK-8276179](https://bugs.openjdk.java.net/browse/JDK-8276179): > PrismFontFile.isInstalledFont is dead code and should be removed ⚠️ Title > mismatch between PR and JBS. @FlorianKirmaier except for the above problem with the title of this PR, it is ready to be integrated. Once you update this PR's title, Skara will mark it as ready. - PR: https://git.openjdk.java.net/jfx/pull/658
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport
On Sat, 30 Oct 2021 10:56:40 GMT, Florian Kirmaier wrote: > This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. > To fix it, I've made the Value of the WeakHashMap also weak. > We only keep this value to remove it as a listener later on. Therefore there > shouldn't be issues by making this value weak. > > > I've seen this Bug very very often, in the last weeks. Most of the > applications I've seen are somehow affected by this bug. > > It basically **breaks every application with menu bars and multiple stages** > - which is the majority of enterprise applications. It's especially annoying > because it takes some time until the application gets unstable. > > Therefore **I would recommend** after this fix is approved, **to make a new > version for JavaFX17** with this fix because this bug is so severe. > * [JDK-8274022](https://bugs.openjdk.java.net/browse/JDK-8274022): > Additional Memory Leak in ControlAcceleratorSupport ⚠️ Title mismatch between > PR and JBS. @FlorianKirmaier as a reminder, the PR title must match the JBS title. Note that I modified the JBS title slightly, so when you do fix this, you will need to grab the updated title: 8274022: Additional Memory Leak in ControlAcceleratorSupport - PR: https://git.openjdk.java.net/jfx/pull/659
RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport
This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. To fix it, I've made the Value of the WeakHashMap also weak. We only keep this value to remove it as a listener later on. Therefore there shouldn't be issues by making this value weak. I've seen this Bug very very often, in the last weeks. Most of the applications I've seen are somehow affected by this bug. It basically **breaks every application with menu bars and multiple stages** - which is the majority of enterprise applications. It's especially annoying because it takes some time until the application gets unstable. Therefore **I would recommend** after this fix is approved, **to make a new version for JavaFX17** with this fix because this bug is so severe. - Commit messages: - 8274022 fixing memory leak in the ControlAcceleratorSupport Changes: https://git.openjdk.java.net/jfx/pull/659/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274022 Stats: 39 lines in 2 files changed: 30 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 09:46:13 GMT, Jeanette Winzenburg wrote: >> I don't see an easy way to do that, and I'm not in favor of making private >> implementation details package-public just to test some internal state. Of >> course, mnemonic support should have been designed in a way that is more >> easily testable, but this PR is not the place to do that. > > in the skin test, it could be tested indirectly, though not in isolation: > > - access the actual mnenomic via accessibleAttribute > - test whether labelFor/action is working as expected when firing an > alt-mnemonic onto the scene > > Just noticed that there is no test of TextBinding .. that's where the correct > working of the basics should be tested, shouldn't it? Just noticed @kleopatra that you suggested the same thing, I definitely agree that the place to test this is in a test for `TextBinding`. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Sat, 30 Oct 2021 09:46:13 GMT, Jeanette Winzenburg
wrote:
>> I don't see an easy way to do that, and I'm not in favor of making private
>> implementation details package-public just to test some internal state. Of
>> course, mnemonic support should have been designed in a way that is more
>> easily testable, but this PR is not the place to do that.
>
> in the skin test, it could be tested indirectly, though not in isolation:
>
> - access the actual mnenomic via accessibleAttribute
> - test whether labelFor/action is working as expected when firing an
> alt-mnemonic onto the scene
>
> Just noticed that there is no test of TextBinding .. that's where the correct
> working of the basics should be tested, shouldn't it?
How about adding a JUnit test for TextBinding instead? The class is
sufficiently complicated to warrant one, and it has quite a few branches to
cover. I think testing it through a `Control` is a bit too high level.
Something like:
TextBinding tb = new TextBinding("complicated_mnemonic__example_(s)__");
assertEquals("m", tb.getMnemonic());
assertEquals(KeyCombination.M, tb.getMnemonicKeyCombination());
assertEquals(12, tb.getMnemonicIndex());
-
PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 23:14:07 GMT, Michael Strauß wrote:
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
>> line 2162:
>>
>>> 2160: label.autosize();
>>> 2161: skin.updateDisplayedText();
>>> 2162: assertEquals("foo _bar _qux",
>>> LabelSkinBaseShim.getText(label).getText());
>>
>> Can you verify here which letter / index is picked as mnemonic? (Also in the
>> other tests).
>
> I don't see an easy way to do that, and I'm not in favor of making private
> implementation details package-public just to test some internal state. Of
> course, mnemonic support should have been designed in a way that is more
> easily testable, but this PR is not the place to do that.
could be tested indirectly, though not in isolation:
- access the actual mnenomic via accessibleAttribute
- test whether labelFor/action is working as expected when firing an
alt-mnemonic onto the scene
-
PR: https://git.openjdk.java.net/jfx/pull/647
