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

Chrome Debugger Refactorings #256

Closed
wants to merge 1 commit into from
Closed

Chrome Debugger Refactorings #256

wants to merge 1 commit into from

Conversation

AObuchow
Copy link
Contributor

Refactorings for debugTab's and DebugDelegates

Signed-off-by: Andrew Obuchowicz aobuchow@redhat.com

@AObuchow AObuchow changed the title Chrome Debuger Refactorings Chrome Debbuger Refactorings Sep 17, 2019
@AObuchow AObuchow changed the title Chrome Debbuger Refactorings Chrome Debugger Refactorings Sep 17, 2019
AObuchow
AObuchow commented Sep 17, 2019
View reviewed changes
org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/debug/AbstractDebugDelegate.java Outdated

DSPLaunchDelegateLaunchBuilder builder = new DSPLaunchDelegateLaunchBuilder(configuration, mode, launch,
monitor);
builder.setLaunchDebugAdapter(InitializeLaunchConfigurations.getNodeJsLocation(), debugCmdArgs);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized this line is calling InitializeLaunchConfigurations.getNodeJsLocation() - it should probably call a function to get the delegates debug adapter location instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind.. you need node to run the debug adapter 🤦‍♂️

AObuchow
AObuchow commented Sep 17, 2019
View reviewed changes
org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/debug/AbstractDebugDelegate.java Outdated
import org.eclipse.wildwebdeveloper.Activator;
import org.eclipse.wildwebdeveloper.InitializeLaunchConfigurations;

public class AbstractDebugDelegate extends DSPLaunchDelegate{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be public abstract class AbstractDebugDelegate

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, and also, as it's only for HTML, AbstractHTMLDebugDelegate.

@mickaelistria
Copy link
Contributor

I let you do the self-review and publish new version of the commit. Ping me when you think it's good to merge.

@AObuchow
Copy link
Contributor Author

I let you do the self-review and publish new version of the commit. Ping me when you think it's good to merge.

Sounds good :)

@mickaelistria mickaelistria mentioned this pull request Sep 18, 2019
Closed
AObuchow
AObuchow commented Sep 18, 2019
View reviewed changes
Copy link
Contributor Author

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

need to remove garbage files

@AObuchow AObuchow force-pushed the Refactor_chrome branch 2 times, most recently from 9315008 to c191d9c Compare September 18, 2019 14:54
mickaelistria
mickaelistria requested changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

That seems good. See some comments and questions inline.

org.eclipse.wildwebdeveloper/build.properties Outdated
@@ -4,7 +4,8 @@ bin.includes = META-INF/,\
.,\
plugin.xml,\
language-servers/package-lock.json,\
language-servers/node_modules/,\
language-servers/chrome-debug-adapter/extension/node_modules/,\
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems erroneous, the build.properties doesn't need to change.

org.eclipse.wildwebdeveloper/pom.xml
@@ -132,6 +132,9 @@
</execution>
</executions>
</plugin>

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for extra lines here. You can reset to previous state to simplify the change (and the review)

org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/debug/AbstractDebugDelegate.java Outdated
public static final String ENV = ILaunchManager.ATTR_ENVIRONMENT_VARIABLES;
public static final String SOURCE_MAPS = "sourceMaps";
public static final String PORT = "port"; //$NON-NLS-1$
public static final String REQUEST = "request"; //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all those attributes the same for the 3 debug adapters?

org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/debug/RunHTMLDebugTab.java Outdated
import org.eclipse.wildwebdeveloper.Activator;
import org.eclipse.wildwebdeveloper.debug.chrome.ChromeRunDebugLaunchShortcut;

public class RunHTMLDebugTab extends AbstractLaunchConfigurationTab {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this class isn't meant to be instantiated directly, make it abstract and call it AbstractRunHTMLDebugTab

org.eclipse.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/debug/AbstractDebugDelegate.java Outdated
import org.eclipse.wildwebdeveloper.Activator;
import org.eclipse.wildwebdeveloper.InitializeLaunchConfigurations;

public class AbstractDebugDelegate extends DSPLaunchDelegate{
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, and also, as it's only for HTML, AbstractHTMLDebugDelegate.

...se.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/debug/node/NodeRunDAPDebugDelegate.java Outdated
@@ -96,7 +96,12 @@ public void launch(ILaunchConfiguration configuration, String mode, ILaunch laun
} catch (IOException e) {
IStatus errorStatus = new Status(IStatus.ERROR, Activator.PLUGIN_ID, e.getMessage(), e);
Activator.getDefault().getLog().log(errorStatus);
ErrorDialog.openError(Display.getDefault().getActiveShell(), "Debug error", e.getMessage(), errorStatus); //$NON-NLS-1$
Display.getDefault().asyncExec(new Runnable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

use a lambda here instead of explicit new Runnable()

Display.getDefault().asyncExec(() -> ErrorDialog.openError(Display.getDefault().getActiveShell(), "Debug error", e.getMessage(), errorStatus)); //$NON-NLS-1$

...se.wildwebdeveloper/src/org/eclipse/wildwebdeveloper/debug/node/NodeAttachDebugDelegate.java Outdated
@@ -66,7 +66,12 @@ public void launch(ILaunchConfiguration configuration, String mode, ILaunch laun
} catch (IOException e) {
IStatus errorStatus = new Status(IStatus.ERROR, Activator.PLUGIN_ID, e.getMessage(), e);
Activator.getDefault().getLog().log(errorStatus);
ErrorDialog.openError(Display.getDefault().getActiveShell(), "Debug error", e.getMessage(), errorStatus); //$NON-NLS-1$
Display.getDefault().asyncExec(new Runnable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

use a lambda here instead of explicit new Runnable()

Display.getDefault().asyncExec(() -> ErrorDialog.openError(Display.getDefault().getActiveShell(), "Debug error", e.getMessage(), errorStatus)); //$NON-NLS-1$

@AObuchow
Chrome Debuger Refactorings
5f4f004 
Refactorings for DebugTabs, DebugLaunchShortcuts and DebugDelegates

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
@mickaelistria
Copy link
Contributor

Build is failing because .target reference older platform repo. I'll try to fix it tomorrow.

@AObuchow
Copy link
Contributor Author

Build is failing because .target reference older platform repo. I'll try to fix it tomorrow.

Ok no problem

@mickaelistria
Copy link
Contributor

Patch works perfectly (for what I tried), and makes code simpler and easier to navigate.
Merged as cbb485a
Thanks!

@AObuchow
Copy link
Contributor Author

Glad to hear! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickaelistria mickaelistria mickaelistria requested changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@AObuchow @mickaelistria