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

[RAPPS] Rework application handling #5070

Closed
wants to merge 0 commits into from

Conversation

learn-more
Copy link
Member

Previously, there would be function duplication between installed and available applications. Now this is handled with polymorphism, which allows to re-use a lot of code. Additionally, toolbar buttons are properly disabled now. The mutex used to guard rapps' single instance is renamed, so that the 'new' and old rapps can be run at the same time for testing.

* PURPOSE: Classes for working with available applications
* COPYRIGHT: Copyright 2017 Alexander Shaposhnikov (sanchaez@reactos.org)
* Copyright 2020 He Yang (1160386205@qq.com)
* Copyright 2021-2023 Mark Jansen <mark.jansen@reactos.org>
Copy link
Contributor

Choose a reason for hiding this comment

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

the space indentation between Alex and his email, and He and his email should be stripped. Especially since you already decided to not follow that nonsense-padding-attempt with your own email.
Some other newly created files are also affected. I will only mention that once.

InstallLocalTime.wDay = wcstol(m_szInstallDate.Mid(6, 2).GetString(), NULL, 10);
}
}
// It might be a DWORD (Unix timestamp). try again.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is very good regarding its content, but its positioning (before the else) is awful. Please move it!

@@ -1410,7 +1252,7 @@ BOOL CAppsListView::SetDisplayAppType(APPLICATION_VIEW_TYPE AppType)
SetCheckboxesVisible(TRUE);
break;

case AppViewTypeEmpty:
//case AppViewTypeEmpty:
default:
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reasoning behind keeping that commented line?

Copy link
Member Author

Choose a reason for hiding this comment

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

forgot to remove it.

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.

LGTM. Aside my minor style-nitpicks.

I especially appreciate the commenting within these files (not sure whether all of that was yours.) Not too much, not too little. Very helpful!

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.

👍 Code looks cleaner indeed now.

base/applications/rapps/applicationdb.cpp Outdated Show resolved Hide resolved
base/applications/rapps/applicationdb.cpp Outdated Show resolved Hide resolved
base/applications/rapps/applicationdb.cpp Outdated Show resolved Hide resolved
base/applications/rapps/applicationdb.cpp Outdated Show resolved Hide resolved
base/applications/rapps/settingsdlg.cpp Outdated Show resolved Hide resolved
base/applications/rapps/settingsdlg.cpp Outdated Show resolved Hide resolved
base/applications/rapps/settingsdlg.cpp Outdated Show resolved Hide resolved
base/applications/rapps/applicationdb.cpp Outdated Show resolved Hide resolved
@learn-more
Copy link
Member Author

Because formatting was so important to @HBelusca and @JoachimHenze that they left almost 30 individual reviews with one comment per review,
I have formatted every file in rapps that I touched using the .clang-format file in our repo.

If the style is still not to your liking, feel free to fix the .clang-format file or provide a better way to do this automatically.

@learn-more learn-more closed this Feb 20, 2023
@learn-more learn-more deleted the rapps_cleanup branch February 20, 2023 18:31
learn-more referenced this pull request Feb 20, 2023
Previously, there would be function duplication between installed and available applications.
Now this is handled with polymorphism, which allows to re-use a lot of code.
Additionally, toolbar buttons are properly disabled now.
The mutex used to guard rapps' single instance is renamed,
so that the 'new' and old rapps can be run at the same time for testing.

CORE-18459
@binarymaster binarymaster added the refactoring For refactoring changes. label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring For refactoring changes.
Projects
None yet
4 participants