Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8087980: Add property to disable Monocle cursor #5

Closed

Conversation

@dellgreen
Copy link
Contributor

dellgreen commented Oct 3, 2019

Often on embedded systems a cursor is not a valid input modality. On some of these systems, when the javafx toolkit initialises the native hardware cursor, it produces artefacts which can be seen on screen (in the framebuffer for example). This change adds a system property "monocle.cursor.enabled" that can disable the creation of a native cursor in each of the Monocle NativePlatform implementations in favour of a NullCursor which is a no-op implementation of the NativeCursor abstract class that all native cursors have to implement.

NullCursor class already existed and was being returned for some implementations like AndroidPlatform and HeadlessPlatform. This change builds upon that and conditionally returns NullCursor for all platforms.

A system property "monocle.debugcursor" has also been added to turn on logging of which NativeCursor has been selected when the toolkit initialises.

Progress

Issue

JDK-8087980: Add property to disable Monocle cursor

Approvers

  • jgneff (no known github.com user name / role)
  • Johan Vos (jvos - Reviewer)
@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 3, 2019

@bridgekeeper bridgekeeper bot added the oca label Oct 3, 2019
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 3, 2019

Hi dellgreen, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user dellgreen" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 3, 2019

/signed
/covered

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 3, 2019

https://www.oracle.com/technetwork/community/oca-486395.html#i

Ideaworks Ltd. - OpenJDK OpenJFX - dellgreen

@dellgreen dellgreen changed the title JDK-8087980: Add property to disable Monocle cursor 8087980: Add property to disable Monocle cursor Oct 4, 2019
@bridgekeeper bridgekeeper bot removed the oca label Oct 7, 2019
@openjdk
Copy link

openjdk bot commented Oct 7, 2019

@dellgreen This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8087980: Add property to disable Monocle cursor

Reviewed-by: jvos
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 70 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Oct 7, 2019
@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 7, 2019

/integrate

@openjdk openjdk bot added the sponsor label Oct 7, 2019
@openjdk
Copy link

openjdk bot commented Oct 7, 2019

@dellgreen
Your change (at version cfbbc7d) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 7, 2019

Hold on a minute. There is clearly some bug in the Skara tooling, since this change has not yet been reviewed, and is therefore not ready to integrate.

Copy link
Member

kevinrushforth left a comment

This has not yet been reviewed. It will need at least one reviewer with a Reviewer role in the project.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 8, 2019

You are already a known contributor!

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 8, 2019

am going to temporarily change the title in an attempt to force the jcheck bot to run again. I'll change it back once done. Failing this, I will ask the Skara admins to rerun the check if possible. If this doesn't work, we will need to close this PR and have you open a new one.

@kevinrushforth kevinrushforth changed the title 8087980: Add property to disable Monocle cursor WIP: 8087980: Add property to disable Monocle cursor Oct 8, 2019
@openjdk openjdk bot removed the ready label Oct 8, 2019
@kevinrushforth kevinrushforth changed the title WIP: 8087980: Add property to disable Monocle cursor 8087980: Add property to disable Monocle cursor Oct 8, 2019
@openjdk openjdk bot added the rfr label Oct 8, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 8, 2019

Webrevs

@johanvos
Copy link
Collaborator

johanvos commented Oct 29, 2019

Is this PR open for review now? Or will a new PR be created?

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 29, 2019

this is ready for review from my perspective. :)

@kevinrushforth kevinrushforth self-requested a review Oct 29, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 29, 2019

The Skara tooling bug in question has been fixed, so yes this is ready for review.

@kevinrushforth kevinrushforth requested review from johanvos and removed request for kevinrushforth Nov 13, 2019
@kevinrushforth kevinrushforth removed their request for review Nov 13, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 13, 2019

I have no particular issue one way or the other on this. I will defer the review to @johanvos

@dellgreen
Copy link
Contributor Author

dellgreen commented Nov 13, 2019

As a follow up, if its desirable for javafx to be used in IOT/embedded devices, we should fix this issue, otherwise it doesn't look as slick on startup when i put it next to competing devices. :)

protected static final boolean debugCursor =
AccessController.doPrivileged((PrivilegedAction<Boolean>) () -> {
final String str =
System.getProperty("monocle.debugcursor", "");

This comment has been minimized.

Copy link
@jgneff

jgneff Nov 13, 2019

Contributor

Just a nit, but why monocle.debugcursor rather than monocle.cursor.debug (my preference), or at least monocle.debugCursor? Below is the full list, for comparison, including the two added by this pull request.

monocle.cursor.enabled
monocle.debugcursor
monocle.epd.bitsPerPixel
monocle.epd.enableInversion
monocle.epd.forceMonochrome
monocle.epd.noWait
monocle.epd.rotate
monocle.epd.useDitheringY1
monocle.epd.useDitheringY4
monocle.epd.waveformMode
monocle.epd.y8inverted
monocle.input.<product>.flipXY
monocle.input.<product>.maxX
monocle.input.<product>.maxY
monocle.input.<product>.minX
monocle.input.<product>.minY
monocle.input.<product>.touchFilters
monocle.input.touchFilters
monocle.input.touchRadius
monocle.input.traceEvents
monocle.input.traceEvents.verbose
monocle.maliSignedStruct
monocle.platform
monocle.platform.traceConfig
monocle.screen.fb
monocle.stackSize

I'm nervous about our hidden API of system properties, and I'm as guilty as anyone with the nine properties I added for Monocle EPD. I think it might be okay as long as the code gets the property values only during class initialization. That should restrict their use to startup scripts and keep them out of application code trying to modify them on-the-fly at run time.

This comment has been minimized.

Copy link
@dellgreen

dellgreen Nov 13, 2019

Author Contributor

if i recall i originally started with the format you recommend as it made more sense, and when looking for other debug logging across the javafx stack I picked up on somewhat of a loose existing convention so changed to match it. I guess it can be whatever everyone agrees upon. :)

This comment has been minimized.

Copy link
@jgneff

jgneff Nov 16, 2019

Contributor

On second thought, let's remove monocle.debugcursor and use a PlatformLogger. The JavaFX loggers are available from Logging. In retrospect, that's how I managed to avoid any new debug properties for Monocle EPD even though it's packed with debugging and trace messages. For examples, see the variable logger in EPDFrameBuffer, where logger.fine is called for messages printed once per run, while logger.finer is called for messages printed once per rendered frame.

protected static final boolean debugCursor =
AccessController.doPrivileged((PrivilegedAction<Boolean>) () -> {
final String str =
System.getProperty("monocle.debugcursor", "");

This comment has been minimized.

Copy link
@jgneff

jgneff Nov 16, 2019

Contributor

On second thought, let's remove monocle.debugcursor and use a PlatformLogger. The JavaFX loggers are available from Logging. In retrospect, that's how I managed to avoid any new debug properties for Monocle EPD even though it's packed with debugging and trace messages. For examples, see the variable logger in EPDFrameBuffer, where logger.fine is called for messages printed once per run, while logger.finer is called for messages printed once per rendered frame.

if (debugCursor) {
final String name = cursor == null ? null : cursor.getClass().getSimpleName();
System.err.println("Using native cursor: " + name);
}

This comment has been minimized.

Copy link
@jgneff

jgneff Nov 16, 2019

Contributor

Here you would call PlatformLogger.fine unconditionally, for example, instead of the conditional call to System.err.println.

@dellgreen
Copy link
Contributor Author

dellgreen commented Nov 16, 2019

OK, that looks a better way to go. I'll sort out out an additional pull request when back in work Monday. Off the top of your head can you remember the syntax to change the logger level from command line when starting the JVM?

@jgneff
Copy link
Contributor

jgneff commented Nov 16, 2019

Off the top of your head can you remember the syntax to change the logger level from command line when starting the JVM?

Here's how I use it. My Bash run script is bin/run.sh:

#!/bin/bash
# Runs the EPD JavaFX Animator program
JAVA_HOME=$HOME/opt/jdk-13.0.1+9
JAVAFX_LIB=$HOME/lib/armv6hf-sdk/lib

app_dir=$HOME/src/epd-javafx
app_jar=$app_dir/dist/epd-javafx.jar
logging=$app_dir/conf/logging.properties

$JAVA_HOME/bin/java -Djava.util.logging.config.file=$logging \
    --add-modules=javafx.graphics --module-path=$JAVAFX_LIB \
    -Dglass.platform=Monocle -Dmonocle.platform=EPD -Dprism.order=sw \
    -Dmonocle.input.18/0/0/0.maxX=800 -Dmonocle.input.18/0/0/0.maxY=600 \
    -Dmonocle.epd.waveformMode=4 -jar $app_jar $@

The logging properties are defined in conf/logging.properties:

# Global properties
handlers = java.util.logging.ConsoleHandler
.level = INFO

# Handler specific properties
java.util.logging.ConsoleHandler.level = ALL
java.util.logging.ConsoleHandler.formatter = java.util.logging.SimpleFormatter
java.util.logging.SimpleFormatter.format=%4$s: %5$s%n

# Facility specific properties
javafx.level = FINE

The output looks like the following:

$ sudo bin/run.sh --pattern=3 --loops=0
FINE: EPD system properties: {monocle.epd.waveformMode=4}
FINE: Frame buffer geometry: 800 600 800 640 32
FINE: Frame buffer rgba: 8/16,8/8,8/0,8/24
FINE: Frame buffer grayscale: 0
FINE: Mapping frame buffer: 1,920,000 bytes
Frame rate: 80 frames in 11.93 s =  6.70 fps (149 ms/frame)
Frame rate: 80 frames in  9.97 s =  8.03 fps (125 ms/frame)
Frame rate: 80 frames in  9.96 s =  8.03 fps (125 ms/frame)

You can just add a new commit to this pull request branch. (I think that's what you meant, but just in case.) The commits get squashed into a single commit when they are integrated into the target branch.

@dellgreen
Copy link
Contributor Author

dellgreen commented Nov 17, 2019

cool thanks, apologies i meant commit. :)

@dellgreen
Copy link
Contributor Author

dellgreen commented Nov 18, 2019

system property has now been changed to use platform logger as advised. has been tested successfully on my imx6 board.

@jgneff
jgneff approved these changes Nov 20, 2019
Copy link
Contributor

jgneff left a comment

Yeah—one less system property! Thanks, Dell.

@dellgreen
Copy link
Contributor Author

dellgreen commented Nov 20, 2019

no probs, thanks for approving :)

Copy link
Collaborator

johanvos left a comment

LGTM

@openjdk openjdk bot added ready and removed rfr labels Dec 18, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 2, 2020

@dellgreen this is ready for you to integrate. It will need a sponsor, so perhaps @johanvos would be willing?

@openjdk openjdk bot added ready and removed ready labels Jan 2, 2020
@johanvos
Copy link
Collaborator

johanvos commented Jan 2, 2020

/sponsor

@openjdk
Copy link

openjdk bot commented Jan 2, 2020

@johanvos The PR has been updated since the change author (@dellgreen) issued the integrate command - the author must perform this command again.

@dellgreen
Copy link
Contributor Author

dellgreen commented Jan 2, 2020

/integrate

@openjdk openjdk bot added the sponsor label Jan 2, 2020
@openjdk
Copy link

openjdk bot commented Jan 2, 2020

@dellgreen
Your change (at version 1f6e282) is now ready to be sponsored by a Committer.

@johanvos
Copy link
Collaborator

johanvos commented Jan 2, 2020

/sponsor

@openjdk openjdk bot closed this Jan 2, 2020
@openjdk openjdk bot added integrated and removed sponsor ready labels Jan 2, 2020
@openjdk
Copy link

openjdk bot commented Jan 2, 2020

@johanvos @dellgreen The following commits have been pushed to master since your change was applied:

  • 3bbcbfb: 8225571: Port DND source to use GTK instead of GDK
  • 1952606: 8232811: Dialog's preferred size no longer accommodates multi-line strings
  • 4c6ebfb: 8236484: Compile error in monocle dispman
  • 8367e1a: 8130738: Add tabSize property to Text and TextFlow
  • 69e4ef3: 8235627: Blank stages when running JavaFX app in a macOS virtual machine
  • 5e0fb91: 8235364: Update copyright header for files modified in 2019
  • 935e99d: 8207957: TableSkinUtils should not contain actual code implementation
  • 4e005e4: 8227808: Make GTK3 libraries mandatory for building on Linux
  • 1140d34: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
  • d2d44b4: 8207759: VK_ENTER not consumed by TextField when default Button exists
  • fc539b5: 8223296: NullPointerException in GlassScene.java at line 325
  • 71ca899: 8220722: ProgressBarSkin: adds strong listener to control's width property
  • 07af89a: 8221334: TableViewSkin: must initialize flow's cellCount in constructor
  • 4ddf554: 8234916: [macos 10.15] Garbled text running with native-image
  • 46338d0: 8232524: SynchronizedObservableMap cannot be be protected for copying/iterating
  • a68347c: 8235150: IosApplication does not pass the required object in _leaveNestedEventLoopImpl
  • 1c27fbd: 8210955: DOMTest::testEventListenerCascade fails
  • 2d4096a: 8235151: Nonexistent notifyQuit method referred from iOS GlassHelper.m
  • 98035cb: 8211308: Support HTTP/2 in WebView
  • 6892fa1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
  • 1d670f1: 8200224: Multiple press event when JFXPanel gains focus
  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md
  • 4d3c723: 8234593: Mark LeakTest.testGarbageCollectability as unstable
  • 5a39824: 8234056: Upgrade to libxslt 1.1.34
  • 8bea7b7: 8229472: Deprecate for removal JavaBeanXxxPropertyBuilders constructors
  • aad1720: 8233420: Upgrade to gcc 8.3 on Linux
  • 42040c4: 8232063: Upgrade gradle to version 6.0
  • aab07a4: 8234239: [TEST_BUG] Reenable few ignored web tests
  • 95ad601: 8233421: Upgrade to Visual Studio 2017 version 15.9.16
  • 3e0557a: 8234303: [TEST_BUG] Correct ignore tag in graphics unit tests
  • e37cb37: 8234150: Address ignored tests in ComboBoxTest, LabeledTest, HyperLinkTest and TextInputControlTest
  • 4f496d4: 8234194: [TEST_BUG] Reenable few graphics unit tests
  • 927fc8a: 8234174: Change IDEA VCS mapping to Git
  • 3d0cb49: 8234189: [TEST_BUG] Remove ignored and invalid graphics unit tests
  • dc01309: 8234110: SwingFXUtilsTest is unsuitable for unit test framework
  • 5b96ee4: 8231188: Update SQLite to version 3.30.1
  • d46dcae: 8233338: FX javadoc headings are out of sequence
  • 94bcf3f: 8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene
  • 286d1b5: 8230492: font-family not set in HTMLEditor if font name has a number in it
  • f74f3af: 8233040: ComboBoxPopupControl: remove eventFilter for F4
  • a1cc4ab: 8232210: Update Mesa 3-D Headers to version 19.2.1
  • dca8df4: 8232943: Gesture support is not initialized on iOS
  • 5a70b0c: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
  • ac71396: 8232929: Duplicate symbols when building static libraries
  • ab6ea3b: 8232158: [macOS] Fallback to command line tools if xcode is missing
  • 2ae171a: 8232687: No static JNI loader for libprism-sw
  • a09a0fa: 8232522: FX: Update copyright year in docs, readme files to 2020
  • 582d999: Merge
  • b6e53f4: 8218640: Update ICU4C to version 64.2
  • bada612: Merge
  • 64aaeb8: 8226754: FX build fails using gradle 5.6+ or 6
  • 63fe66c: 8231870: CrossLibs script for armv6hf toolchain download fails
  • a4bc22d: Merge
  • c6eb091: 8231854: Change Mercurial to git in various README files
  • 0a0d34a: 8231590: Update location of jfx repo to GitHub in third-party legal files
  • f595cc1: 8231735: gradle checkrepo is obsolete and doesn't work with git
  • 2593dea: Merge
  • 003128b: 8231310: Add .jcheck/conf to jfx git repo
  • 29906fd: 8092352: Skip dispatch if there are no handlers/filters
  • 9a6b69d: 8231326: Update README and CONTRIBUTING docs for Skara
  • 9df20c5: Merge
  • a1aa38a: Merge
  • 0ff02bb: Merge
  • a433bf2: Merge
  • 561153d: Merge
  • 75d439e: Merge
  • 758252f: Merge
  • 35e0cae: 8227402: Improve XSLT processing

Your commit was automatically rebased without conflicts.

Pushed as commit 580a2a9.

@dellgreen dellgreen deleted the dellgreen:dpg/8087980/optionalMonocleCursor branch Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.