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

Typo fixes #318

Merged
merged 6 commits into from
Jan 12, 2025
Merged

Typo fixes #318

merged 6 commits into from
Jan 12, 2025

Conversation

isf63
Copy link
Contributor

@isf63 isf63 commented Feb 16, 2023

Typo fixes to comments/text and identifiers. Half of the changes are to CHANGELOG.

Affected files:

  • CHANGELOG
  • CMakeLists.txt
  • lxqtapplication.h
  • lxqtnotification.cpp
  • lxqtnotification.h
  • lxqtnotification_p.h
  • lxqtsettings.cpp
  • cmake/lxqt-config.cmake.in

@isf63
Copy link
Contributor Author

isf63 commented Aug 9, 2023

@tsujan, could you review this old PR - asking because you merged most of my other typo fixes. Also I have two .ui/XML tab-order PRs, in lxqt-session and lxqt-config.

@tsujan
Copy link
Member

tsujan commented Aug 10, 2023

@isf63, thanks for the reminder.

Real life problems may interfere with reviewing (and coding, in general). Reminders like this are appreciated, when a PR was made before the previous release.

@@ -142,7 +142,7 @@ namespace
QScopedPointer<SignalHandler> SignalHandler::instance;
}

void Application::listenToUnixSignals(QList<int> const & signoList)
void Application::listenToUnixSignals(QList<int> const & signalList)
Copy link
Member

Choose a reason for hiding this comment

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

Also, in the body of the function (this cannot be compiled).

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw. this wasn't any typo, but intentional name (signal numbers list) :-)

Copy link
Member

@tsujan tsujan Aug 11, 2023

Choose a reason for hiding this comment

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

Btw. this wasn't any typo

The comment of the function said ... defined in \param signalList....

"signolList" wasn't indicative of a number either. I think @isf63 was right to treat it as a typo, although a harmless one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change the identifier and comment to signoList as that is used elsewhere in lxqtapplication.cpp/h.

Copy link
Member

Choose a reason for hiding this comment

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

I'll change the identifier and comment to signoList as that is used elsewhere in lxqtapplication.cpp/h.

Oh, this is getting complicated for no good reason. That method is public and has a comment; "signoList" has no meaning as a name either. Your previous change was good, and I was going to merge it after a test....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

signoList has a well-defined meaning - 'signo' in Unix/Linux means 'signal number'. Running grep -o -i signo lxqtapplication.cpp there were 13 instances of signo before the recent commit, including another signoList method param.

I agree, getting complicated, but I think it's done.

Copy link
Member

Choose a reason for hiding this comment

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

signoList has a well-defined meaning...

It may have been used by some devs and then copied. "Linux signo" leads to "Linux signal" in Google and Wikipedia.

there were 13 instances of signo

That's why I said, "That method is public and has a comment....". Anyway, I resigned from reviewing it. Hopefully, another dev will do it.

@isf63
Copy link
Contributor Author

isf63 commented Jan 12, 2025

Mostly typo corrections to comments.

Double checked and partially reviewed by @tsujan. Merging...

@isf63 isf63 merged commit d2c0d2e into lxqt:master Jan 12, 2025
@tsujan
Copy link
Member

tsujan commented Jan 12, 2025

Please NEVER merge a PR that isn't approved by the reviewer(s).

@tsujan
Copy link
Member

tsujan commented Jan 12, 2025

Particularly in this case, which is about a fundamental library, merging without asking others could create bad headaches later. I reverted it in 7b66ae5

@tsujan
Copy link
Member

tsujan commented Jan 12, 2025

A suggestion: You might want to make another PR that includes only the typos that are NOT inside the codes.

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.

3 participants