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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Typo fixes #318

wants to merge 4 commits into from

Conversation

isf63
Copy link

@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
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.

lxqtapplication.h Outdated Show resolved Hide resolved
@@ -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
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
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.

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