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

[SHELL32] brfolder.cpp: Code formatting and refactoring #6909

Merged
merged 3 commits into from
May 24, 2024

Conversation

katahiromz
Copy link
Contributor

@katahiromz katahiromz commented May 19, 2024

Purpose

Modernize source code. Improve readability and reduce confusing.
JIRA issue: CORE-17340

Proposed change

  • Code formatting.
  • Rename brsfolder.cpp as brfolder.cpp.
  • Rename browse_info as BrFolder.
  • Rename TV_ITEMDATA as BrItemData.
  • Rename InsertTreeViewItem as BrFolder_InsertItem.
  • Rename InitializeTreeView as BrFolder_InitTreeView.
  • Rename GetIcon as BrFolder_GetIcon.
  • Rename GetNormalAndSelectedIcons as BrFolder_GetIconPair.
  • Rename GetName as BrFolder_GetName.
  • Fix some confusing and/or non-standard variable names.
  • Use ATL to simplify logics.

TODO

  • Do build.
  • Do check.

@github-actions github-actions bot added the shell All PR's related to the shell (and shell extensions) label May 19, 2024
@katahiromz katahiromz added the refactoring For refactoring changes. label May 19, 2024
@katahiromz katahiromz changed the title [SHELL32] brsfolder.cpp: Code formatting only [SHELL32] brsfolder.cpp: Code formatting May 19, 2024
dll/win32/shell32/brsfolder.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/brsfolder.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/brsfolder.cpp Outdated Show resolved Hide resolved
@whindsaks

This comment was marked as outdated.

@katahiromz

This comment was marked as outdated.

@HBelusca HBelusca self-requested a review May 19, 2024 18:38
@HBelusca HBelusca added the review pending For PRs undergoing review. label May 19, 2024
@HBelusca
Copy link
Contributor

I'm going to review this during this week.

@katahiromz katahiromz changed the title [SHELL32] brsfolder.cpp: Code formatting [SHELL32] brsfolder.cpp: Code formatting and refactoring May 19, 2024
Copy link
Contributor

@HBelusca HBelusca left a comment

Choose a reason for hiding this comment

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

I don't see any obvious problems here but I would strongly suggest the following, in order to ease diffs and review:

  1. in the first commit, do all the whitespace/comments/brackets/code-style changes ONLY;
  2. in the second commit, do all the refactoring (renamings of structs, variables, and usage of COM/ATL helpers etc.)

Modernize source code. Improve readability and reduce confusing.
JIRA issue: CORE-17340
- Rename browse_info as BrsFolder.
- Rename TV_ITEMDATA as BrsItemData.
- Rename InsertTreeViewItem as BrsFolder_InsertItem.
- Rename InitializeTreeView as BrsFolder_InitTreeView.
- Rename GetIcon as BrsFolder_GetIcon.
- Rename GetNormalAndSelectedIcons as BrsFolder_GetIconPair.
- Rename GetName as BrsFolder_GetName.
- Fix some confusing and/or non-standard variable names.
- Use ATL to simplify logics.
@katahiromz
Copy link
Contributor Author

@HBelusca Done.

and rename BrsItemData as BrItemData to reduce binary size of
Debug version.
JIRA issue: CORE-17340
@katahiromz katahiromz changed the title [SHELL32] brsfolder.cpp: Code formatting and refactoring [SHELL32] brfolder.cpp: Code formatting and refactoring May 23, 2024
Copy link
Contributor

@JoachimHenze JoachimHenze left a comment

Choose a reason for hiding this comment

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

Approved. Please merge the 3 commits in unsquashed form. Makes sense to wait for Hermes review to complete.

@HBelusca HBelusca added no squash merge Author has no full name in GitHub profile, either merge by rebase or manually and removed review pending For PRs undergoing review. labels May 24, 2024
Copy link
Contributor

@HBelusca HBelusca left a comment

Choose a reason for hiding this comment

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

Looks nice this way. The commits need to be merged separately (NOT squashed)

@katahiromz katahiromz merged commit ed7c23f into reactos:master May 24, 2024
34 checks passed
@katahiromz katahiromz deleted the brsfolder-formatting branch May 24, 2024 13:54
katahiromz added a commit that referenced this pull request May 25, 2024
Follow-up of #6909 (af03438). Fix assertions and exceptions of
CComPtr and CComHeapPtr.
JIRA issue: CORE-17340
- Don't use AddRef, Release, and Attach methods against CComPtr
  template class. Simply use assignment.
- Fix the 3rd parameter of three IShellFolder::GetAttributesOf
  method calls. It had referenced CHeapPtr::operator&.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no squash merge Author has no full name in GitHub profile, either merge by rebase or manually refactoring For refactoring changes. shell All PR's related to the shell (and shell extensions)
Projects
None yet
4 participants