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.

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

Bug 578871 - use ISchedulableOperation #19

Merged
merged 1 commit into from
May 2, 2022
Merged

Bug 578871 - use ISchedulableOperation #19

merged 1 commit into from
May 2, 2022

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Apr 19, 2022 •
edited by iloveeclipse
Loading

to prevent UI freeze during UNDO

needs
eclipse-platform/eclipse.platform.runtime#30

@jukzi jukzi mentioned this pull request Apr 19, 2022
Merged
@jukzi
Copy link
Contributor Author

jukzi commented Apr 21, 2022

please rebuild.

@jukzi
Copy link
Contributor Author

jukzi commented Apr 25, 2022

I think I covered all requested changes but the review is still -1, what do you think is missing?

@ktatavarthi ktatavarthi removed their request for review April 25, 2022 09:36
iloveeclipse
iloveeclipse requested changes Apr 25, 2022
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.

Jörg: I see there are two commits now.

Ideally that should be one (no need here for multiple commits), with appropriate commit message. I fear, github own "squash and merge" will generate some stupid commit message, so ideally you could locally squash your two commits into one (using proper commit message) & push again.

...factoring/src/org/eclipse/ltk/internal/core/refactoring/UndoableOperation2ChangeAdapter.java Show resolved Hide resolved
@jukzi
Copy link
Contributor Author

jukzi commented Apr 25, 2022

I have no influence on what you will put in the commit message.:
When you press squash then you get an msg editor that you have to edit. The message header will be the first line in the commit message and the multilinetext below ist line 3+ in the commit message. It defaults to a combination of all commit messages plus some additional lines if any. Feel free to change it to anything you understand.

@jarthana
Copy link
Member

I have no influence on what you will put in the commit message.

That's what we (I believe @iloveeclipse too) are trying to avoid, I suppose. Besides, I think it also makes it easier to review if the changes are in one commit, although I have no personal preference.

@jukzi jukzi mentioned this pull request Apr 26, 2022
Closed
@EcljpseB0T
Bug 578871 - UndoableOperation2ChangeAdapter use ISchedulableOperation
279cf8b 
Give the caller of the UndoableOperation2ChangeAdapter.execute a hint
that this operation aquires workspace.root as ISchedulingRule.
It freezed the eclipse UI as long as any other Thread (for example
autobuild) had any lock in the workspace.
With the hint a UI can prevent to freeze while waiting for the rule.

The original call to ResourcesPlugin.getWorkspace().run(runnable, pm);
is equivalent to the call of Workspace.run(IWorkspaceRunnable,
IProgressMonitor), which is implemented internally as
run((ICoreRunnable) action, defaultRoot, IWorkspace.AVOID_UPDATE,
monitor); - means, changing the original code to proposed one is safe
and doesn't change any behavior because the executed code it is
identical at the end. It only gives a hint
about the lock that is used anyway.

Workspace.run(IWorkspaceRunnable, IProgressMonitor) was inlined to
give a uniform pattern where the lock is aquired.
@iloveeclipse
Copy link
Member

@jukzi : I've tried to undo method rename wit all patches applied, but none operations I get in org.eclipse.ui.operations.OperationHistoryActionHandler.run() are UndoableOperation2ChangeAdapter.

Can you provide exact steps to reproduce the original issue described in https://bugs.eclipse.org/bugs/show_bug.cgi?id=578871?

@jukzi
Copy link
Contributor Author

jukzi commented Apr 26, 2022

@jukzi : I've tried to undo method rename

In eclipse ide in a java editor select a java method name, press ALT+SHIFT+R to rename, edit name, press ENTER.
Undo with CTRL+Z

@iloveeclipse
Copy link
Member

In eclipse ide in a java editor select a java method name, press ALT+SHIFT+R to rename, edit name, press ENTER.
Undo with CTRL+Z

I'm doing exact this and still get no UndoableOperation2ChangeAdapter

Debugging that I see this eclipse-platform/eclipse.platform.ui#18 was not complete, see my debugger screenshot that explains why:
image

@jukzi
Copy link
Contributor Author

jukzi commented Apr 26, 2022

Debugging that I see this eclipse-platform/eclipse.platform.ui#18 was not complete,

Then we can let TriggeredOperations forward the ISchedulableOperation.

@iloveeclipse
Copy link
Member

Then we can let TriggeredOperations forward the ISchedulableOperation.

Would you provide a PR?

Reminder for myself: draft commit message here (assuming no other changes needed)

Bug 578871 - UndoableOperation2ChangeAdapter use ISchedulableOperation

Give the caller of the UndoableOperation2ChangeAdapter.execute a hint
that this operation acquires workspace root as ISchedulingRule.
Undo operation freezes UI if any other thread (for example
autobuild job) holds the workspace lock during undo.

With the given hint UI (org.eclipse.ui.operations.OperationHistoryActionHandler) 
can prevent the freeze in RefactoringExecutionHelper.perform() while waiting 
for the workspace rule.

This change does not introduce any additional locking, since original code
already implicitly acquired workspace lock by using workspace root as a rule.

@jukzi
Copy link
Contributor Author

jukzi commented Apr 26, 2022

Would you provide a PR?

eclipse-platform/eclipse.platform.ui#37

@iloveeclipse iloveeclipse merged commit 72c4c91 into eclipse-jdt:master May 2, 2022
merks pushed a commit to merks/eclipse.jdt.ui that referenced this pull request Oct 11, 2023
@SDawley
POM and product version changes for 4.25 release (eclipse-jdt#19)
ef8e7fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gayanper gayanper gayanper left review comments

@iloveeclipse iloveeclipse iloveeclipse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jukzi @jarthana @iloveeclipse @gayanper @EcljpseB0T