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

FIX: Toggling a breakpoint by clicking on the linenumber in debugger not possible #1367

Closed

Conversation

jonasPoehler
Copy link
Contributor

This is a regression introduced through #1343.
With that pull request getModifiers() was switched to getModifiersEx(). However these two methods are not as easily interchangeable (especially when using the mouse-release-event) as it might seem from the deprecation notice. In this particular case the MouseEvent is checked on mouseRelease. However when looking at the documentation of getModifiersEx() (e.g. https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/java.desktop/share/classes/java/awt/event/InputEvent.java#L506C10-L506C10) it gets clear that at that stage, we cannot detect the button click with the help of the modifiers. I did some debugging and at the point of button release getModifiersEx() just returns 0, meaning that no bit is set at all. A possible solution to this is to check which button triggered the release-event with the use of getButton() like I did here. This behaviour was also discovered by someone on stackoverflow a while back: https://stackoverflow.com/questions/60518621/mouseevent-getmodifiersex-call-does-not-work-as-expected-or-as-documented.
Note: The PR #1343 touched another spot where getModifiers() was replaced. Since I am fairly new to this project, I was not sure if the changed code is correct at this point either: https://github.com/mozilla/rhino/pull/1343/files#diff-5d4e8db22e19b87ef92bbee2473a342f75f2e166ff04d18e07cbc02e46c91514L2477. I only noticed that the enclosing method seemingly is never used, so I can't say where the MouseEvent is coming from hence I am unable to tell if that code has to be changed as well.

@p-bakker p-bakker requested a review from gbrail August 8, 2023 14:23
@gbrail
Copy link
Collaborator

gbrail commented Aug 11, 2023

Thanks a ton for finding and fixing this!

Since there are additional regressions introduced in #1343, I'm going to:

  1. Close this PR
  2. Add your commit to a new PR: Fix regressions introduced to debugger #1369
  3. If everything works out, merge them together and not squash so that you are in the commit history

Thanks for contributing and please test out the other PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants