Skip to content

Conversation

@laeubi
Copy link
Contributor

@laeubi laeubi commented Jul 5, 2025

Currently the BackgroundThread is an inner class of the AbstractReconciler and is tightly coupled with it what makes it quite hard to understand the interaction between both and to properly update a reconciler on document changes or refactor the code.

This is a first attempt to decouple the things by extracting the inner class in an own (package private) type to allow further refactoring operations and proper encapsulation.

Whats next:

Currently the change of the viewer initiates an exchange of the reconcile queue, this might or might not overlap with the background thread and the Job itself is not changed making it also not possible to identify the actual document. Instead the viewer and the queue should be passed in the constructor, the old job should be canceled and a new one must be scheduled to make this properly works. With that it would even be possible to find out the document providing input and set the name accordingly making the whole access to the shared data more threadsafe and easier to understand.

Then at the moment the job uses delays to wait for more things to do and sleeping otherwise unless notified. This notification for new things could be replaced by a scheduling of the job instead making much less threads running when many documents are open. Currently for each open Editor one thread is running forever unless the user closes the document.

@laeubi laeubi requested review from iloveeclipse and merks July 5, 2025 05:32
Currently the BackgroundThread is an inner class of the
AbstractReconciler and is tightly coupled with it what makes it quite
hard to understand the interaction between both and to properly update a
reconciler on document changes or refactor the code.

This is a first attempt to decouple the things by extracting the inner
class in an own (package private) type to allow further refactoring
operations and proper encapsulation.
@laeubi laeubi force-pushed the extract_Reconciler_job branch from e6cb1a4 to 6faa8bd Compare July 5, 2025 05:49
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2025

Test Results

 1 852 files   -   926   1 852 suites   - 926   1h 15m 35s ⏱️ - 31m 2s
 7 930 tests ±    0   7 695 ✅  -     7  233 💤 +  5  1 ❌ +1  1 🔥 +1 
15 567 runs   - 7 778  15 052 ✅  - 7 547  513 💤  - 233  1 ❌ +1  1 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit 6faa8bd. ± Comparison against base commit d2177cf.

This pull request skips 5 tests.
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testEnabledWhenHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testMultipleHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testProblemHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testSingleHover
org.eclipse.ui.genericeditor.tests.ShowInformationTest ‑ testInformationControl

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

It all seems fine in principle, though I imagine the same could be accomplished by leaving it where it was and simply making the class static.

@laeubi
Copy link
Contributor Author

laeubi commented Jul 7, 2025

Merging as no further concerns are raised.

@laeubi laeubi merged commit 4b0fa03 into eclipse-platform:master Jul 7, 2025
15 of 18 checks passed
@iloveeclipse
Copy link
Member

I had no time to review because I've spent time on original problem.
Please check eclipse-jdt/eclipse.jdt.ui#2323 (comment).

@iloveeclipse
Copy link
Member

@laeubi : also if you request reviews, ideally you would give more then one working day time. I'm trying to have some private life on weekends, so you can't expect review to happen on same working day.

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.

3 participants