Improve workspace lock error dialog. #2347

raghucssit · 2024-10-01T11:24:33Z

Write workspace lock info like user, host, java process id, display properties onto .lock file if the lock was successful. Read the current lock data in case of lock was unsuccessful and shown it in error dialog.
If the .lock file has no info then nothing is shown. For older eclipse versions.

see #2343

raghucssit · 2024-10-01T11:24:51Z

@iloveeclipse FYI

BeckerWdf · 2024-10-01T11:39:12Z

Can you pls. provide a screenshot showing the new dialog?

laeubi · 2024-10-01T12:01:02Z

By the way if on the lock dialog is worked, it would be great to have a cancel button!

Currently one can only Retry or Choose where on the second one can then finally cancel the startup of Eclipse. Instead I have most of the time not the case that another user locked the workspace but I have just an instance already running. In that case a early Cancel would be great.

raghucssit · 2024-10-01T12:01:26Z

Can you pls. provide a screenshot showing the new dialog?

Dialog looks like this with this fix.

github-actions · 2024-10-01T12:49:28Z

Test Results

1 818 files + 3 1 818 suites +3 1h 33m 2s ⏱️ + 8m 40s
7 709 tests + 45 7 481 ✅ +155 228 💤 ± 0 0 ❌ - 1
24 288 runs +128 23 541 ✅ +441 747 💤 +22 0 ❌ - 1

Results for commit 03c222e. ± Comparison against base commit 1eead54.

♻️ This comment has been updated with latest results.

BeckerWdf · 2024-10-01T13:07:12Z

Can you pls. provide a screenshot showing the new dialog?

Dialog looks like this with this fix.

Maybe User, Host, etc. should be written in with an uppercase first letter (because they are some kind of label)

tomaswolf · 2024-10-01T14:32:07Z

And these display strings must be localizeable.

iloveeclipse

Unfortunately it looks like writing to the locked file unlocks it again!

I'm able to start multiple Eclipse instances on same workspace now :-(

...g.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEApplication.java

raghucssit · 2024-10-07T07:29:18Z

Fixed all the review comments. Committed as separate commit to help the review.
Later I will squash them as single commit.

raghucssit · 2024-10-07T07:32:53Z

Unfortunately it looks like writing to the locked file unlocks it again!

I'm able to start multiple Eclipse instances on same workspace now :-(

I have changed the approach now.
First we will check if the workspace is locked, If not then write the data who is going to lock it.(Also backup .lock file data)
Now initiate actual lock, If we are successful all good Otherwise write back what was there before onto .lock file.

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/messages.properties

bundles/org.eclipse.ui.ide.application/META-INF/MANIFEST.MF

...g.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEApplication.java

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/IDEWorkbenchMessages.java

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/messages.properties

iloveeclipse · 2024-10-08T13:09:28Z

Also please rebase on latest master before next push

raghucssit · 2024-10-08T13:53:03Z

Also please rebase on latest master before next push

I went through comments. I will refactor the code with .loc_info file approach. which will not have any risks. If file exists we show owner info otherwise old error dialog.

raghucssit · 2024-10-09T14:15:00Z

By the way if on the lock dialog is worked, it would be great to have a cancel button!

Currently one can only Retry or Choose where on the second one can then finally cancel the startup of Eclipse. Instead I have most of the time not the case that another user locked the workspace but I have just an instance already running. In that case a early Cancel would be great.

I will do this change as separate PR. As we are only improving error message here. Other suggestions are fixed.

raghucssit · 2024-10-09T14:15:15Z

All the review suggestions are fixed.

BeckerWdf · 2024-10-10T08:50:53Z

All the review suggestions are fixed.

How does the UI look now?

raghucssit · 2024-10-10T09:54:32Z

All the review suggestions are fixed.

How does the UI look now?

Now it looks like this.

laeubi · 2024-10-10T09:59:31Z

Now it looks like this.

The labels are still not aligned, it should be something like

User:    abcd
Host:    234.567.7
Display: :1

BeckerWdf · 2024-10-10T10:06:08Z

Display: :1

Especially this looks strange because of the two ":". I knot that's they displays are called. Maybe we should put all the values into single or double quotes. So that is would look like

User:    "abcd"
Host:    "234.567.7"
Display: ":1"

laeubi · 2024-10-10T10:15:14Z

Especially this looks strange because of the two ":". I knot that's they displays are called.

Display has format <hostname>:<sequence number>.<screen number> so the format can potentially be longer.

BeckerWdf · 2024-10-10T10:16:43Z

Especially this looks strange because of the two ":". I knot that's they displays are called.

Display has format <hostname>:<sequence number>.<screen number> so the format can potentially be longer.

Sure. But this does not contradict my proposal...

raghucssit · 2024-10-10T12:46:04Z

Especially this looks strange because of the two ":". I knot that's they displays are called.

Display has format <hostname>:<sequence number>.<screen number> so the format can potentially be longer.

I am using System.getenv("DISPLAY"); to read display number. I use 2 monitors it always returns value as shown above. like :1 for vnc display 1.

raghucssit · 2024-10-13T20:55:36Z

Display: :1

Especially this looks strange because of the two ":". I knot that's they displays are called. Maybe we should put all the values into single or double quotes. So that is would look like
User:    "abcd"
Host:    "234.567.7"
Display: ":1"

I have introduced tab spacing and single quotes for the lock owner details. Now the dialog looks like this.

BeckerWdf · 2024-10-14T05:32:09Z

I have introduced tab spacing and single quotes for the lock owner details. Now the dialog looks like this.

That looks nice now. Thanks

laeubi · 2024-10-14T06:56:09Z

@BeckerWdf do you think the quotes are required with the new layout? They look a bit misplaced from my point of view.

merks · 2024-10-14T07:40:48Z

@BeckerWdf do you think the quotes are required with the new layout? They look a bit misplaced from my point of view.

Yes, unless one expects there might be trailing or leading whitespace that needs to be called out because that relevant, then the quotes seem like merely noise. And I believe in this case there is no interesting whitespace.

BeckerWdf · 2024-10-14T08:49:46Z

Yes. In the new layout we can get rid of them from my point of view.

...g.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEApplication.java

Write workspace lock info like user, host, java process id, display properties onto a new file .lock_info if the lock was successful. Read the current lock data in case of lock was unsuccessful and show it in error dialog. If the .lock_info does not exist or the file has no info then nothing is shown. For older eclipse versions. see eclipse-platform#2343

raghucssit · 2024-10-14T21:15:47Z

Fixed review comments. Now dialog looks like this.

iloveeclipse

Thanks Raghu! Looks good now.

iloveeclipse · 2024-10-15T05:56:28Z

...g.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEApplication.java

+ private File createLockInfoFile(URL workspaceUrl) throws Exception {
+ File lockInfoFile = getLockInfoFile(workspaceUrl);
+
+ if (lockInfoFile.exists())


Next time please don't forget to put "if" in a block.

raghucssit force-pushed the impr_ws_lock_dlg_2343 branch from 10fa592 to f34d747 Compare October 1, 2024 12:02

iloveeclipse requested changes Oct 1, 2024

View reviewed changes

raghucssit requested a review from iloveeclipse October 7, 2024 07:33

laeubi reviewed Oct 7, 2024

View reviewed changes

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/messages.properties Outdated Show resolved Hide resolved

laeubi reviewed Oct 7, 2024

View reviewed changes

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/messages.properties Outdated Show resolved Hide resolved

iloveeclipse requested changes Oct 8, 2024

View reviewed changes

raghucssit force-pushed the impr_ws_lock_dlg_2343 branch 6 times, most recently from f8f7892 to 1c35e4f Compare October 9, 2024 14:11

raghucssit requested review from iloveeclipse and laeubi October 9, 2024 14:15

raghucssit force-pushed the impr_ws_lock_dlg_2343 branch 2 times, most recently from 9f3f9a9 to 987ceca Compare October 13, 2024 20:54

iloveeclipse requested changes Oct 14, 2024

View reviewed changes

raghucssit force-pushed the impr_ws_lock_dlg_2343 branch from 987ceca to 03c222e Compare October 14, 2024 21:13

raghucssit requested a review from iloveeclipse October 14, 2024 21:15

iloveeclipse approved these changes Oct 15, 2024

View reviewed changes

iloveeclipse merged commit 52d1a35 into eclipse-platform:master Oct 15, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve workspace lock error dialog. #2347

Improve workspace lock error dialog. #2347

raghucssit commented Oct 1, 2024

raghucssit commented Oct 1, 2024

BeckerWdf commented Oct 1, 2024

laeubi commented Oct 1, 2024

raghucssit commented Oct 1, 2024

github-actions bot commented Oct 1, 2024 •

edited

Loading

BeckerWdf commented Oct 1, 2024

tomaswolf commented Oct 1, 2024

iloveeclipse left a comment

raghucssit commented Oct 7, 2024

raghucssit commented Oct 7, 2024

iloveeclipse commented Oct 8, 2024

raghucssit commented Oct 8, 2024

raghucssit commented Oct 9, 2024

raghucssit commented Oct 9, 2024

BeckerWdf commented Oct 10, 2024

raghucssit commented Oct 10, 2024

laeubi commented Oct 10, 2024

BeckerWdf commented Oct 10, 2024

laeubi commented Oct 10, 2024

BeckerWdf commented Oct 10, 2024

raghucssit commented Oct 10, 2024

raghucssit commented Oct 13, 2024

BeckerWdf commented Oct 14, 2024

laeubi commented Oct 14, 2024

merks commented Oct 14, 2024

BeckerWdf commented Oct 14, 2024

raghucssit commented Oct 14, 2024

iloveeclipse left a comment

iloveeclipse Oct 15, 2024

Improve workspace lock error dialog. #2347

Improve workspace lock error dialog. #2347

Conversation

raghucssit commented Oct 1, 2024

raghucssit commented Oct 1, 2024

BeckerWdf commented Oct 1, 2024

laeubi commented Oct 1, 2024

raghucssit commented Oct 1, 2024

github-actions bot commented Oct 1, 2024 • edited Loading

Test Results

BeckerWdf commented Oct 1, 2024

tomaswolf commented Oct 1, 2024

iloveeclipse left a comment

Choose a reason for hiding this comment

raghucssit commented Oct 7, 2024

raghucssit commented Oct 7, 2024

iloveeclipse commented Oct 8, 2024

raghucssit commented Oct 8, 2024

raghucssit commented Oct 9, 2024

raghucssit commented Oct 9, 2024

BeckerWdf commented Oct 10, 2024

raghucssit commented Oct 10, 2024

laeubi commented Oct 10, 2024

BeckerWdf commented Oct 10, 2024

laeubi commented Oct 10, 2024

BeckerWdf commented Oct 10, 2024

raghucssit commented Oct 10, 2024

raghucssit commented Oct 13, 2024

BeckerWdf commented Oct 14, 2024

laeubi commented Oct 14, 2024

merks commented Oct 14, 2024

BeckerWdf commented Oct 14, 2024

raghucssit commented Oct 14, 2024

iloveeclipse left a comment

Choose a reason for hiding this comment

iloveeclipse Oct 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 1, 2024 •

edited

Loading