Skip to content

Bugfix for ErrorDialogWithToggle storing the "Don't tell me again" checkbox selection state. #809

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bugfix for ErrorDialogWithToggle storing the "Don't tell me again" checkbox selection state. #809

wants to merge 1 commit into from
+469 −1

Conversation

@mehmet-karaman
Copy link

@mehmet-karaman mehmet-karaman commented Nov 16, 2025 •
edited
Loading

Fixed the java breakpoints no line attribute error dialog regarding the "Don't tell me again" checkbox. The value wasn't stored in the preference store due to a bug in ErrorDialogWithToggle.

Added also a testcase to easily reproduce the dialog opening and checking its functionality

fixes ticket #811

@mehmet-karaman mehmet-karaman changed the title Fixed the java breakpoints no line attribute error dialog regarding the Bugfix for ErrorDialogWithToggle was wasn't storing the "Don't tell me again" checkbox selection state. Nov 16, 2025
@mehmet-karaman mehmet-karaman changed the title Bugfix for ErrorDialogWithToggle was wasn't storing the "Don't tell me again" checkbox selection state. Bugfix for ErrorDialogWithToggle storing the "Don't tell me again" checkbox selection state. Nov 16, 2025
SougandhS
SougandhS approved these changes Nov 17, 2025
View reviewed changes
Copy link
Member

@SougandhS SougandhS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM 👍

@mehmet-karaman
Copy link
Author

mehmet-karaman commented Nov 17, 2025

Thanks, LGTM 👍

Thank you for revieweing..

iloveeclipse
iloveeclipse requested changes Nov 18, 2025
View reviewed changes
Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding current commit message:

Fixed the java breakpoints no line attribute error dialog regarding the
"Don't tell me again" checkbox. The value wasn't stored in the
preference store due to a bug in ErrorDialogWithToggle.

Added also a testcase to easily reproduce the dialog opening and
checking its functionality

fixes ticket https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/2648

It is almost OK except:

  1. commit title is too long, it is not a title but the commit explanation. It should be ideally under 80 characters, so it is easy to understand in git history
  2. fixes ticket <ticket url> is wrong, ticket breaks automated github linking to the ticket.
  3. The actual problem is not further analyzed. This is a fix for a regression introduced with commit 24b1795.

See example how it should be https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md#commit-message-recommendations

Also note that EGit indicates you all these problems above:

Image
  • I prefer to avoid mocking if possible, as it usually harder to maintain and refactor mocked tests, so I fixed your test to avoid mocking.
  • I've also added extra code that actually opens the dialog, for two reasons: 1) to make sure the test really test what it is supposed to test and 2) to get maintainers a chance to see the dialog and its state.
  • I wil push all fixes now.

@iloveeclipse
Copy link
Member

iloveeclipse commented Nov 18, 2025

Additional request: since the change on ErrorDialogWithToggle affects the second user of that class, we should add tests for JavaHotCodeReplaceListener which opens HotCodeReplaceErrorDialog. I haven't checked whether there are some that test this toggle, I guess not, but this is an important one.

@iloveeclipse iloveeclipse linked an issue Nov 18, 2025 that may be closed by this pull request
NoLineAttributesStatusHandler doesn't saves the "Don't tell me again" selection. #811
Open
@iloveeclipse
Copy link
Member

iloveeclipse commented Nov 18, 2025

The regression is very old, the change affects two different debug dialogs, so I would prefer we would fix it in 4.39 M1.

@mehmet-karaman
Copy link
Author

mehmet-karaman commented Nov 19, 2025 •
edited
Loading

Found this dialog.. Seems that my change causes a StackOverflowException

image

..
at org.eclipse.jdt.internal.debug.ui.ErrorDialogWithToggle.buttonPressed(ErrorDialogWithToggle.java:108)
at org.eclipse.jdt.internal.debug.ui.HotCodeReplaceErrorDialog.buttonPressed(HotCodeReplaceErrorDialog.java:155)
at org.eclipse.jdt.internal.debug.ui.ErrorDialogWithToggle.buttonPressed(ErrorDialogWithToggle.java:108)
at org.eclipse.jdt.internal.debug.ui.HotCodeReplaceErrorDialog.buttonPressed(HotCodeReplaceErrorDialog.java:155)
at org.eclipse.jdt.internal.debug.ui.ErrorDialogWithToggle.buttonPressed(ErrorDialogWithToggle.java:108)
at org.eclipse.jdt.internal.debug.ui.HotCodeReplaceErrorDialog.buttonPressed(HotCodeReplaceErrorDialog.java:155)
..

@mehmet-karaman
Fixed old regression in ErrorDialogWithToggle from commit 24b179
1198069 
The change in commit 24b1795 reverted
the logic of the "don't tell me again" toggle button and only updated
JavaHotCodeReplaceListener (which opens HotCodeReplaceErrorDialog).
However NoLineNumberAttributesStatusHandler also used
ErrorDialogWithToggle but it wasn't updated.

This commit fixes "No line attributes error" dialog on breakpoints
opened by NoLineNumberAttributesStatusHandler regarding the "Don't tell
me again" checkbox behavior. After commit above the value wasn't stored
in the preference store.

Added regression test to easily reproduce the dialog opening and
checking its functionality.

Fixes https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/2648
@mehmet-karaman
Copy link
Author

mehmet-karaman commented Nov 20, 2025

squashed two commits.. because I wanted to see if the tests were successfull.. locally I had test problems with the AutomatedSuite.
After seeing a sucessfull built, it was better to squash them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iloveeclipse iloveeclipse iloveeclipse requested changes

@SougandhS SougandhS SougandhS approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NoLineAttributesStatusHandler doesn't saves the "Don't tell me again" selection.

3 participants

@mehmet-karaman @iloveeclipse @SougandhS