eclipse.platform.ui icon indicating copy to clipboard operation
eclipse.platform.ui copied to clipboard

Improve workspace lock error dialog.

Open raghucssit opened this issue 1 year ago • 11 comments

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 https://github.com/eclipse-platform/eclipse.platform.ui/issues/2343

raghucssit avatar Oct 01 '24 11:10 raghucssit

@iloveeclipse FYI

raghucssit avatar Oct 01 '24 11:10 raghucssit

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

BeckerWdf avatar Oct 01 '24 11:10 BeckerWdf

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.

laeubi avatar Oct 01 '24 12:10 laeubi

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

Dialog looks like this with this fix. image

raghucssit avatar Oct 01 '24 12:10 raghucssit

Test Results

 1 818 files  +  3   1 818 suites  +3   1h 33m 2s :stopwatch: + 8m 40s  7 709 tests + 45   7 481 :white_check_mark: +155  228 :zzz: ± 0  0 :x:  - 1  24 288 runs  +128  23 541 :white_check_mark: +441  747 :zzz: +22  0 :x:  - 1 

Results for commit 03c222e5. ± Comparison against base commit 1eead54a.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Oct 01 '24 12:10 github-actions[bot]

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

Dialog looks like this with this fix. image

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

BeckerWdf avatar Oct 01 '24 13:10 BeckerWdf

And these display strings must be localizeable.

tomaswolf avatar Oct 01 '24 14:10 tomaswolf

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

raghucssit avatar Oct 07 '24 07:10 raghucssit

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.

raghucssit avatar Oct 07 '24 07:10 raghucssit

Also please rebase on latest master before next push

iloveeclipse avatar Oct 08 '24 13:10 iloveeclipse

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 avatar Oct 08 '24 13:10 raghucssit

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 avatar Oct 09 '24 14:10 raghucssit

All the review suggestions are fixed.

raghucssit avatar Oct 09 '24 14:10 raghucssit

All the review suggestions are fixed.

How does the UI look now?

BeckerWdf avatar Oct 10 '24 08:10 BeckerWdf

All the review suggestions are fixed.

How does the UI look now?

Now it looks like this. image

raghucssit avatar Oct 10 '24 09:10 raghucssit

Now it looks like this.

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

User:    abcd
Host:    234.567.7
Display: :1

laeubi avatar Oct 10 '24 09:10 laeubi

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"

BeckerWdf avatar Oct 10 '24 10:10 BeckerWdf

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.

laeubi avatar Oct 10 '24 10:10 laeubi

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...

BeckerWdf avatar Oct 10 '24 10:10 BeckerWdf

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 avatar Oct 10 '24 12:10 raghucssit

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. image

raghucssit avatar Oct 13 '24 20:10 raghucssit

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

That looks nice now. Thanks

BeckerWdf avatar Oct 14 '24 05:10 BeckerWdf

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

laeubi avatar Oct 14 '24 06:10 laeubi

@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.

merks avatar Oct 14 '24 07:10 merks

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

BeckerWdf avatar Oct 14 '24 08:10 BeckerWdf

Fixed review comments. Now dialog looks like this. image

raghucssit avatar Oct 14 '24 21:10 raghucssit

This change seems to have introduced a regression documented in #2423

jonahgraham avatar Oct 18 '24 23:10 jonahgraham