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

issue reporter refactor (web prep) #212762

Merged
merged 5 commits into from
May 16, 2024

Conversation

justschen
Copy link
Contributor

refactoring and moving around some stuff for issue reporter in preparation for web.

issue.ts contains BaseIssueReporterService.

Web issue reporter will use browser/issue/issueReporterService.ts with IssueReporterWeb extends BaseIssueReporterService

Normal issue reporter will use electron-sandbox/issue/issueReporterServiceNew.ts with IssueReporterNew extends BaseIssueReporterService

in issueMainService, we change to use the new IssueReporterNew object.

can be easily reverted simply by changing instantiation back to IssueReporter instead of IssueReporterNew

Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Good! Just a few very general comments, considering I'm not familiar with the code in this area.

src/vs/code/browser/issue/issueReporterService.ts Outdated Show resolved Hide resolved
Comment on lines +68 to +75
this.addEventListener('disableExtensions', 'click', () => {
this.issueMainService.$reloadWithExtensionsDisabled();
});

this.addEventListener('extensionBugsLink', 'click', (e: Event) => {
const url = (<HTMLElement>e.target).innerText;
windowOpenNoOpener(url);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

unsure whether any of these need a debounce. Could someone click multiple times and fire multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also unsure, will double check with Tyler on this 😢 personally haven't seen any issues on this tho (window focus is changed in either cases)

src/vs/code/browser/issue/issue.ts Show resolved Hide resolved
src/vs/code/browser/issue/issue.ts Outdated Show resolved Hide resolved
src/vs/code/browser/issue/issue.ts Show resolved Hide resolved
@justschen justschen marked this pull request as ready for review May 16, 2024 17:42
@VSCodeTriageBot VSCodeTriageBot added this to the May 2024 milestone May 16, 2024
@justschen justschen merged commit 60d035d into microsoft:main May 16, 2024
6 checks passed
@bpasero
Copy link
Member

bpasero commented May 16, 2024

Just fyi for some layering issues you might encounter: nothing can depend on vs/code, its our main entry point. So if you want to open issue reporter from vs/workbench, that would actually be a layer violation. The issue reporter needs to move into vs/workbench or a base/platform layer if we end up keeping vs/code for desktop.

@justschen
Copy link
Contributor Author

justschen commented May 16, 2024

@bpasero thanks for those points!

atm, I'm opening via a new issueMainService thru vs/platform/issue/browser/issueMainService which I see does have some layering issues so I'll take a look to see where that should actually be moved to

@bpasero
Copy link
Member

bpasero commented May 17, 2024

@justschen I think https://github.com/microsoft/vscode/wiki/Source-Code-Organization is a good read about this topic, feel free to reach out in case of questions.

As for the issueMainService, some remarks:

  • I introduced xyMainService for services that run in the electron-main process, but in your case its a service that will be created from the workbench, so I suggest issueService or if you want a new one issueFormService or issueWindowService
  • if the service ends up being used only by workbench I would move it into vs/workbench/contrib/issue, the platform layer is meant for things that are shared between vs/code, vs/workbench and vs/editor

Again, my feeling is things would be much easier if both desktop and web would use the same issue window, opened as aux window.

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.

None yet

4 participants