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

Some fixes and update french language #46

Merged
merged 3 commits into from
Dec 24, 2024

Conversation

shadow2560
Copy link
Contributor

No description provided.

…king up it and logging it correctly, update french language.

Signed-off-by: shadow2560 <[email protected]>
sphaira_path = "/switch/sphaira.nro";
rc = nro_get_nacp(sphaira_path, sphaira_nacp);
}

// found sphaira, now lets get compare version
if (R_SUCCEEDED(rc) && !std::strcmp(sphaira_nacp.lang[0].name, "sphaira")) {
if (R_SUCCEEDED(rc) && !std::strcmp(sphaira_nacp.lang[0].name, "sphaira") == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

That's wrong. Doing !cond inverts the value, meaning that if the value is zero is becomes a 1 (true), otherwise it becomes a 0 (false).

So you're checking if the inverted value is false, which isn't what we want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's wrong. Doing !cond inverts the value, meaning that if the value is zero is becomes a 1 (true), otherwise it becomes a 0 (false).

So you're checking if the inverted value is false, which isn't what we want

Oups, this "!" shouldn't be here, it wasn't my intention.

For the std::strcmp condition I have had somme problems when I tried to work on my pull request #41 when no explicitely set " == 0" or " != 0" during conditions, that's why I made it explicit.

OK I will revert the change to backup every hbmenu except if it's Sphaira.

@@ -1075,13 +1075,13 @@ App::~App() {
Result rc;

rc = nro_get_nacp(sphaira_path, sphaira_nacp);
if (R_FAILED(rc) || std::strcmp(sphaira_nacp.lang[0].name, "sphaira")) {
if (R_FAILED(rc) || std::strcmp(sphaira_nacp.lang[0].name, "sphaira") != 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

It's not needed, doing if(cond) checks if the value is none zero, adding != 0 is just being verbose ;)

@@ -1052,10 +1052,10 @@ App::~App() {
if (App::GetReplaceHbmenuEnable() && !IsHbmenu()) {
NacpStruct nacp;
fs::FsNativeSd fs;
if (R_SUCCEEDED(nro_get_nacp("/hbmenu.nro", nacp)) && std::strcmp(nacp.lang[0].name, "sphaira")) {
if (R_SUCCEEDED(nro_get_nacp("/hbmenu.nro", nacp)) && std::strcmp(nacp.lang[0].name, "nx-hbmenu") == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

/hbmenu.nro could be anything, iirc there's a few hbmenu replacements out there, and even more forks that change the same. For this reason, I simple check that it's not sphaira, and if so, I back it up - regardless what it may be.

Explicitly checking for hbmenu would mean it wouldn't back up https://github.com/Chrscool8/Homebrew-Details for example.

@@ -317,7 +317,7 @@ void Menu::Sort() {
fs::FsPath star_path;
for (auto& p : m_entries) {
p.has_star = fs.FileExists(GenerateStarPath(p.path));
if (p.has_star) {
if (p.has_star == true) {
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch! I would do p.has_star.value() for consistency sake, as that's what I do throughout the code when using std::optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I would do p.has_star.value() for consistency sake, as that's what I do throughout the code when using std::optional

For this one I prefer to let you do it cause I'm not sure to understand what you exactly want to do. Is it just modifying the condition like that:
if (p.has_star.value()) {

or I need to do something else?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep that's it. std::optional has a method called value, which returns the underlying value, so in this example it would return true or false, see here

if (lhs.has_star.value() && !rhs.has_star.value()) {

Doing if (v == true) if the same as if (v.value()), it's just that I use .value() everywhere else, so I'm trying to be consistent :)

…every homebrews except Sphaira will be backuped. Also fix an error witch make a mal-formed condition.

Signed-off-by: shadow2560 <[email protected]>
@ITotalJustice
Copy link
Owner

Fwiw I'm not really a fan of changing something unless it needs fixing. If you can revert the changes around strcmp then I can merge this. If (strcmp) and if (!strcmp) is far more readable to me as I can tell of it's the checking for equality by looking at the start of the function (does it have ! or not), rather than look at the end.

If (0 == strcmp) works but that's not only longer but it looks weird 😄

@shadow2560
Copy link
Contributor Author

Fwiw I'm not really a fan of changing something unless it needs fixing. If you can revert the changes around strcmp then I can merge this. If (strcmp) and if (!strcmp) is far more readable to me as I can tell of it's the checking for equality by looking at the start of the function (does it have ! or not), rather than look at the end.

If (0 == strcmp) works but that's not only longer but it looks weird 😄

It's a big difference when working with screen reader, this type of writing is not the simplest to read (I often not use my braille reader, with it this is less a problem).

@shadow2560 shadow2560 closed this Dec 24, 2024
@shadow2560 shadow2560 reopened this Dec 24, 2024
@ITotalJustice ITotalJustice merged commit a2c9b63 into ITotalJustice:master Dec 24, 2024
8 checks passed
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.

2 participants