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

Fixes #1294: WebRoot folder can now be set when debugging a URL #1306

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

Si-So
Copy link
Contributor

@Si-So Si-So commented Aug 11, 2023

Design considerations

Since both the firefox debugger (https://github.com/firefox-devtools/vscode-firefox-debug#launch) and the chrome debugger (https://github.com/microsoft/vscode-chrome-debug#launch) use the webroot parameter, I added the functionality to AbstractHTMLDebugDelegate.java and AbstractRunHTMLDebugTab.java. The webroot parameter was already set in FirefoxRunDABDebugDelegate.java, but it was set to the project directory of the currently selected project in the workspace. Since I tend to have multiple projects in a workspace, the currently selected project is not always the correct project for a Chrome Debug session. Thus, it is better to give the user the option to explicitly set (and inspect) the webroot parameter.

@Si-So
Copy link
Contributor Author

Si-So commented Aug 11, 2023

Error: Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.0:compare-version-with-baselines (compare-version-with-baseline) on project org.eclipse.wildwebdeveloper: Only qualifier changed for (org.eclipse.wildwebdeveloper/1.1.1.202308110939). Expected to have bigger x.y.z than what is available in baseline (1.1.1.202308101949) -> [Help 1]

It seems to me that I am not responsible for the test failure, but that some mvn plugin is misconfigured.

Is this correct?

@vrubezhny
Copy link
Contributor

@Si-So At the moment - no. We've just released and WWD needs to bump versions for the next development cycle.
You can add the required version bumpings right to your PR or wait for me making the required changes.

@Si-So
Copy link
Contributor Author

Si-So commented Aug 11, 2023

It seems to me that it makes more sense to wait for the version bump performed by you...

When will you bump the version?

If it makes more sense to bump the version myself in this PR -> What file(s) do I need to touch?

@vrubezhny
Copy link
Contributor

It seems to me that it makes more sense to wait for the version bump performed by you...

When will you bump the version?

Give me few minutes, please

Copy link
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

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

Validation is to be fixed. Please see the inline comment

Copy link
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

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

The Revert breaks at least radio buttons selections, see how does it looks like after I clicked the Revert button:

Screenshot from 2023-08-11 17-01-41

@vrubezhny
Copy link
Contributor

@Si-So Please re-base your PR on top of master branch

@wesleybl
Copy link

@Si-So I didn't understand why I managed to debug a react app even without this PR. Works even without filling Working directory. It will be have projects that need the Working directory and others that don't?

@Si-So
Copy link
Contributor Author

Si-So commented Aug 14, 2023

Introduction

After looking into the behavior of the "Revert" button, I discovered more bugs. These bugs were not caused by me, but I will fix them anyway.

Bug 1: Both radio buttons are selected at the same time

The Revert breaks at least radio buttons selections, see how does it looks like after I clicked the Revert button:

Screenshot from 2023-08-11 17-01-41

As stated above: This bug was not caused by this PR, but I will fix it anyway. Here is how to reproduce this bug in versions of wwd that don't have my changes:

  1. Create a new "Chrome Debug" configuration
  2. Enter something for "File"
  3. Press the "Apply"-Button
  4. Select the radio button for "URL:"
  5. Press the "Revert"-Button
  6. Now both the "File" and the "URL" radio buttons are selected.

This bug was caused by a missing statement in initializeFrom(ILaunchConfiguration configuration)

Bug 2: The debug dialog deletes user input when reverting

Steps to reproduce:

  1. Create a new "Chrome Debug" configuration
  2. Enter something for "File"
  3. Press the "Apply" button
  4. Select the radio button for "URL:"
  5. Enter something for "URL"
  6. Press the "Apply" button
  7. Select the radio button for "File"
  8. Press the "Revert"-Button
  9. Now the text in the "File" textbox is gone and there is no way to recover it.

Expected behavior: Pressing the "Revert" Button does not delete user input

Bug 3: User input is deleted when re-opening the dialog

Steps to reproduce:

  1. Create a new "Chrome Debug" configuration
  2. Enter something for "File"
  3. Press the "Apply" button
  4. Select the radio button for "URL:"
  5. Enter something for "URL"
  6. Press the "Apply" button
  7. Press the "Close" button
  8. Open the dialog again.
  9. Now the text in the "File" textbox is gone and there is no way to recover it.

Expected behavior: Closing and re-opening the debug configuration does not delete user input

Si-So added 2 commits August 14, 2023 13:25
1. It is now impossible to select both radio buttons at the same time
2. The debug dialog no longer deletes user input when reverting
3. The debug dialog no longer deletes user input when re-opening

See: eclipse-wildwebdeveloper#1306 (comment)
@Si-So
Copy link
Contributor Author

Si-So commented Aug 14, 2023

@Si-So Please re-base your PR on top of master branch

I merged with master, because I didn't want to force push.

@Si-So Si-So closed this Aug 14, 2023
@Si-So Si-So reopened this Aug 14, 2023
@Si-So
Copy link
Contributor Author

Si-So commented Aug 14, 2023

@Si-So I didn't understand why I managed to debug a react app even without this PR. Works even without filling Working directory. It will be have projects that need the Working directory and others that don't?

I am not sure why it worked with your react app. Maybe because react is a javaccript framework and angular is a typescript framework?

I must admit that the working directory option in "Chrome Debug" confuses me. I looked into the source code of the debugger (https://github.com/microsoft/vscode-chrome-debug/blob/master/src/chromeDebugAdapter.ts) and as far as I understand the directory is passed to chrome as "cwd". I then wanted to know how chrome handles this option and looked at this list of the cmd-line options for chrome (https://peter.sh/experiments/chromium-command-line-switches/) and couldn't find the cwd-option. I then gave up to understand what this option does. Please note that the documentation for the debugger (https://github.com/microsoft/vscode-chrome-debug/tree/master) describes cwd as optional whereas the webRoot-option is mandatory when setting an URL.

@Si-So Si-So requested a review from vrubezhny August 14, 2023 14:38
@Si-So
Copy link
Contributor Author

Si-So commented Aug 15, 2023

@vrubezhny: Regarding the failed test: I believe that my changes did not cause the regression. Maybe the test is flaky? Can you re-run the tests?

However, if I am responsible, I will fix the test.

Also: For some reason I could only close one of your requested changes. I feel like I have sufficiently addressed the issues that you raised.

Copy link
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@vrubezhny
Copy link
Contributor

@Si-So Thanks for the PR update. The failing test is not your fault - it fails sporadically for some reason.

Thank you for the contribution.

@vrubezhny vrubezhny merged commit 4dbd75b into eclipse-wildwebdeveloper:master Aug 15, 2023
vrubezhny pushed a commit that referenced this pull request Aug 15, 2023
1. It is now impossible to select both radio buttons at the same time
2. The debug dialog no longer deletes user input when reverting
3. The debug dialog no longer deletes user input when re-opening

See: #1306 (comment)
@Si-So
Copy link
Contributor Author

Si-So commented Aug 16, 2023

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants