-
Notifications
You must be signed in to change notification settings - Fork 168
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.
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
Improve workspace lock error dialog. #2347
base: master
Are you sure you want to change the base?
Improve workspace lock error dialog. #2347
Conversation
@iloveeclipse FYI |
Can you pls. provide a screenshot showing the new dialog? |
By the way if on the lock dialog is worked, it would be great to have a cancel button! Currently one can only |
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 eclipse-platform#2343
10fa592
to
f34d747
Compare
Test Results 1 815 files ±0 1 815 suites ±0 1h 34m 54s ⏱️ + 1m 12s For more details on these failures and errors, see this check. Results for commit f34d747. ± Comparison against base commit c9b34e4. |
And these display strings must be localizeable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it looks like writing to the locked file unlocks it again!
I'm able to start multiple Eclipse instances on same workspace now :-(
private String getHostName() { | ||
String hostName = null; | ||
try { | ||
hostName = InetAddress.getLocalHost().getHostName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to check environment variable HOSTNAME
first, because it doesn't do any network lookups and so is cheap on Linux.
props.load(is); | ||
String prop = props.getProperty(USER); | ||
if (prop != null) { | ||
sb.append("user: ").append(prop).append(System.lineSeparator()); //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please use IDEWorkbenchMessages so the strings could be translated if needed.
return null; | ||
} | ||
|
||
String displayText = "Workspace lock is held by the following owner:\n".concat(lockInfo); //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use IDEWorkbenchMessages here
* | ||
* @return Previous lock owner details. | ||
*/ | ||
private String getWorkspaceLockInfo(URL workspaceUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please make this method and writeWsLockInfo() protected? This is not API, but we override parts of the class internally and these two methods most likely could need some customization sooner or later.
|
||
// make sure the directory exists | ||
File metaDir = new File(workspaceUrl.getPath(), METADATA_FOLDER); | ||
if (!metaDir.exists()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for extra folder check, file check below will do the job too
* @return Previous lock owner details. | ||
*/ | ||
private String getWorkspaceLockInfo(URL workspaceUrl) { | ||
File lockFile = getLockFile(workspaceUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lockFile can be null.
* display and current java process id. | ||
*/ | ||
private void writeWsLockInfo() { | ||
Location instanceLoc = Platform.getInstanceLocation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please first create properties & fill with the data, and only if the properties are not empty, open file for writing.
String hostName = null; | ||
try { | ||
hostName = InetAddress.getLocalHost().getHostName(); | ||
} catch (UnknownHostException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be consistent, I would also catch here generic Exception, just in case.
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