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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.net.InetAddress;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.UnknownHostException;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Properties;
Expand All @@ -49,7 +51,11 @@
import org.eclipse.osgi.service.datalocation.Location;
import org.eclipse.osgi.util.NLS;
import org.eclipse.swt.SWT;
import org.eclipse.swt.layout.FillLayout;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Control;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Label;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.ui.IWorkbench;
import org.eclipse.ui.PlatformUI;
Expand Down Expand Up @@ -79,6 +85,20 @@ public class IDEApplication implements IApplication, IExecutableExtension {

private static final String VERSION_FILENAME = "version.ini"; //$NON-NLS-1$

private static final String LOCK_FILENAME = ".lock"; //$NON-NLS-1$

private static final String DISPLAY_VAR = "DISPLAY"; //$NON-NLS-1$

private static final String PROCESS_ID = "process-id"; //$NON-NLS-1$

private static final String DISPLAY = "display"; //$NON-NLS-1$

private static final String HOST = "host"; //$NON-NLS-1$

private static final String USER = "user"; //$NON-NLS-1$

private static final String USER_NAME = "user.name"; //$NON-NLS-1$

// Use the branding plug-in of the platform feature since this is most likely
// to change on an update of the IDE.
private static final String WORKSPACE_CHECK_REFERENCE_BUNDLE_NAME = "org.eclipse.platform"; //$NON-NLS-1$
Expand Down Expand Up @@ -225,6 +245,7 @@ protected Object checkInstanceLocation(Shell shell, Map applicationArguments) {
try {
if (instanceLoc.lock()) {
writeWorkspaceVersion();
writeWsLockInfo();
return null;
}

Expand Down Expand Up @@ -313,6 +334,7 @@ protected Object checkInstanceLocation(Shell shell, Map applicationArguments) {
if (instanceLoc.set(workspaceUrl, true)) {
launchData.writePersistedData();
writeWorkspaceVersion();
writeWsLockInfo();
return null;
}
} catch (IllegalStateException e) {
Expand All @@ -332,17 +354,164 @@ protected Object checkInstanceLocation(Shell shell, Map applicationArguments) {

// by this point it has been determined that the workspace is
// already in use -- force the user to choose again

String lockInfo = getWorkspaceLockInfo(workspaceUrl);

MessageDialog dialog = new MessageDialog(null, IDEWorkbenchMessages.IDEApplication_workspaceInUseTitle,
null, NLS.bind(IDEWorkbenchMessages.IDEApplication_workspaceInUseMessage, workspaceUrl.getFile()),
MessageDialog.ERROR, 1, IDEWorkbenchMessages.IDEApplication_workspaceInUse_Retry,
IDEWorkbenchMessages.IDEApplication_workspaceInUse_Choose);
IDEWorkbenchMessages.IDEApplication_workspaceInUse_Choose) {
@Override
protected Control createCustomArea(Composite parent) {
if (lockInfo == null || lockInfo.isBlank()) {
return null;
}

String displayText = "Workspace lock is held by the following owner:\n".concat(lockInfo); //$NON-NLS-1$
Copy link
Member

Choose a reason for hiding this comment

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

Please use IDEWorkbenchMessages here


Composite container = new Composite(parent, SWT.NONE);
container.setLayout(new FillLayout());

Label multiLineText = new Label(container, SWT.NONE);
multiLineText.setText(displayText);

return container;
}
};
// the return value influences the next loop's iteration
returnValue = dialog.open();
// Remember the locked workspace as recent workspace
launchData.writePersistedData();
}
}

/**
* Read workspace lock file and parse all the properties present. Based on the
* eclipse version and operating system some or all the properties may not
* present. In such scenario it will return empty string.
*
* @return Previous lock owner details.
*/
private String getWorkspaceLockInfo(URL workspaceUrl) {
Copy link
Member

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.

File lockFile = getLockFile(workspaceUrl);
Copy link
Member

Choose a reason for hiding this comment

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

lockFile can be null.

StringBuilder sb = new StringBuilder();
Properties props = new Properties();
try (FileInputStream is = new FileInputStream(lockFile)) {
props.load(is);
String prop = props.getProperty(USER);
if (prop != null) {
sb.append("user: ").append(prop).append(System.lineSeparator()); //$NON-NLS-1$
Copy link
Member

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.

}
prop = props.getProperty(HOST);
if (prop != null) {
sb.append("host: ").append(prop).append(System.lineSeparator()); //$NON-NLS-1$
}
prop = props.getProperty(DISPLAY);
if (prop != null) {
sb.append("display: ").append(prop).append(System.lineSeparator()); //$NON-NLS-1$
}
prop = props.getProperty(PROCESS_ID);
if (prop != null) {
sb.append("process-id: ").append(prop).append(System.lineSeparator()); //$NON-NLS-1$
}
return sb.toString();
} catch (Exception e) {
IDEWorkbenchPlugin.log("Could not read lock file: ", e); //$NON-NLS-1$
}
return null;
}

/**
* Write lock owner details onto workspace lock file. Data includes user, host,
* display and current java process id.
*/
private void writeWsLockInfo() {
Location instanceLoc = Platform.getInstanceLocation();
Copy link
Member

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.

if (instanceLoc == null || instanceLoc.isReadOnly()) {
return;
}

File lockFile = getLockFile(instanceLoc.getURL());
if (lockFile == null) {
return;
}

Properties props = new Properties();

String user = System.getProperty(USER_NAME);
if (user != null) {
props.setProperty(USER, user);
}
String host = getHostName();
if (host != null) {
props.setProperty(HOST, host);
}
String display = getDisplay();
if (display != null) {
props.setProperty(DISPLAY, display);
}
String pid = getProcessId();
if (pid != null) {
props.setProperty(PROCESS_ID, pid);
}

try (OutputStream output = new FileOutputStream(lockFile)) {
props.store(output, null);
} catch (IOException e) {
IDEWorkbenchPlugin.log("Could not modify lock file", e); //$NON-NLS-1$
}
}

private String getDisplay() {
String displayEnv = null;
try {
displayEnv = System.getenv(DISPLAY_VAR);
} catch (Exception e) {
IDEWorkbenchPlugin.log("Failed to read DISPLAY variable.", e); //$NON-NLS-1$
}
return displayEnv;
}

private String getProcessId() {
Long pid = null;
try {
pid = ProcessHandle.current().pid();
} catch (Exception e) {
IDEWorkbenchPlugin.log("Failed to read Java process id.", e); //$NON-NLS-1$
}
return pid != null ? pid.toString() : null;
}

private String getHostName() {
String hostName = null;
try {
hostName = InetAddress.getLocalHost().getHostName();
Copy link
Member

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.

See https://stackoverflow.com/questions/6050011/how-do-i-get-the-local-hostname-if-unresolvable-through-dns-in-java

} catch (UnknownHostException e) {
Copy link
Member

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.

IDEWorkbenchPlugin.log("Failed to read host name.", e); //$NON-NLS-1$
}
return hostName;
}

private File getLockFile(URL workspaceUrl) {
if (workspaceUrl == null) {
return null;
}

// make sure the directory exists
File metaDir = new File(workspaceUrl.getPath(), METADATA_FOLDER);
if (!metaDir.exists()) {
Copy link
Member

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 null;
}

// make sure the file exists
File lockFile = new File(metaDir, LOCK_FILENAME);
if (!lockFile.exists()) {
return null;
}

return lockFile;
}

@SuppressWarnings("rawtypes")
private static boolean isDevLaunchMode(Map args) {
// see org.eclipse.pde.internal.core.PluginPathFinder.isDevLaunchMode()
Expand Down
Loading