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

SCM - Add commands to focus each resource group #213255

Closed
wants to merge 4 commits into from

Conversation

lszomoru
Copy link
Member

Related #204300

@lszomoru lszomoru self-assigned this May 22, 2024
@lszomoru lszomoru requested a review from joaomoreno May 22, 2024 19:50
@lszomoru lszomoru added the scm General SCM compound issues label May 22, 2024
@lszomoru lszomoru added this to the May 2024 milestone May 22, 2024
@admsev
Copy link

admsev commented May 26, 2024

@gooroodev please review

@gooroodev
Copy link

Thanks for the tag, @admsev!

Summary of Changes

This pull request introduces new commands to focus on each Source Control Resource Group (SCM Resource Group). The major changes include:

  1. New Dependencies and Imports:

    • Added ReferenceCollection, DisposableMap, and toDisposable from vs/base/common/lifecycle.
    • Added VIEW_PANE_ID from vs/workbench/contrib/scm/common/scm.
    • Added Action2, registerAction2 from vs/platform/actions/common/actions.
    • Added ServicesAccessor from vs/platform/instantiation/common/instantiation.
    • Added localize2 from vs/nls.
    • Added IViewsService from vs/workbench/services/views/common/viewsService.
    • Added SCMViewPane from vs/workbench/contrib/scm/browser/scmViewPane.
  2. New Class SCMResourceGroupActionReferenceCollection:

    • Manages references to newly registered actions for focusing on resource groups.
    • Uses ReferenceCollection to manage the lifecycle of these actions.
    • Registers actions with IDs in the format workbench.scm.action.resourceGroup.focus.<group_id>.
  3. Modifications in MainThreadSCM:

    • Introduced _resourceGroupActions as an instance of SCMResourceGroupActionReferenceCollection.
    • Introduced _resourceGroupDisposables to manage disposables for resource groups.
    • Modified $registerGroups to acquire references for new resource groups and $unregisterGroup to dispose of these references.
  4. Updates in SCMViewPane:

    • Added a new method focusResourceGroup to handle focusing on a specific resource group by its ID.
    • Modified the title of an existing action to better align with the new functionality.

Issues, Bugs, or Typos

  1. Typo in Method Name:

    • The method localize2 should be replaced with localize as localize2 is not a standard or recognized method in the vs/nls module.

    Proposed Change:

    - import { localize2 } from 'vs/nls';
    + import { localize } from 'vs/nls';
    - title: { ...localize2('focusResourceGroup', "Focus on {0} Source Control Resource Group", args[0] as string) },
    + title: { ...localize('focusResourceGroup', "Focus on {0} Source Control Resource Group", args[0] as string) },
    - title: { ...localize2('focusInput', "Focus on Source Control Input") },
    + title: { ...localize('focusInput', "Focus on Source Control Input") },
  2. Improper Disposal Handling in MainThreadSCM:

    • Ensure that _resourceGroupDisposables.get(handle) is not undefined before calling dispose().

    Proposed Change:

    - this._resourceGroupDisposables.get(handle)!.dispose();
    + this._resourceGroupDisposables.get(handle)?.dispose();
    - this._resourceGroupDisposables.get(sourceControlHandle)!.dispose();
    + this._resourceGroupDisposables.get(sourceControlHandle)?.dispose();
  3. Typo in Variable Naming:

    • The variable resourceGroupDisposables should be renamed to resourceGroupDisposablesMap for clarity.

    Proposed Change:

    - const resourceGroupDisposables = this._resourceGroupDisposables.get(sourceControlHandle);
    + const resourceGroupDisposablesMap = this._resourceGroupDisposables.get(sourceControlHandle);
    - if (!resourceGroupDisposables) {
    + if (!resourceGroupDisposablesMap) {
    - resourceGroupDisposables.set(group[0], toDisposable(ref.dispose));
    + resourceGroupDisposablesMap.set(group[0], toDisposable(ref.dispose));

General Review of Code Quality and Style

  • Code Structure: The code is well-structured and follows a clear modular approach by introducing new classes and methods to handle the new functionality.
  • Naming Conventions: The naming conventions are consistent and descriptive, making the code easy to understand.
  • Comments and Documentation: The code lacks comments explaining the purpose and functionality of new methods and classes. Adding comments would improve code readability.
  • Error Handling: The code could benefit from more robust error handling, especially when dealing with disposables.
  • Performance Considerations: The use of Throttler and Sequencer in SCMViewPane shows consideration for performance and avoiding race conditions.

Overall, the code is well-written and integrates seamlessly with the existing codebase, following the established patterns and practices.

Yours, Gooroo.dev. I'd love to hear from you! React or reply.

@lszomoru lszomoru closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scm General SCM compound issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants