Skip to content

Code modernizations#88

Open
krf wants to merge 7 commits intomasterfrom
hotfix/code-modernization
Open

Code modernizations#88
krf wants to merge 7 commits intomasterfrom
hotfix/code-modernization

Conversation

@krf
Copy link
Copy Markdown
Member

@krf krf commented Apr 27, 2026

No description provided.

Kevin Funk added 7 commits March 7, 2026 21:23
Fixes:

```
icemon(253199)/qt.core.qobject.connect: unknown(0): QObject::disconnect: Unexpected nullptr parameter @ ?icemon?|?icemon?|?icemon?|?icemon?|?libQt6Core.so.6?|...
```
Changes:
- Create Monitor & HostInfoManager in main() directly
- MainWindow is just an observer of those
- Setup Monitor directly in main (without pass-through methods)
- Remove duplicated logic in HostInfoManager from Monitor
- Remove unnecessary QString<->QByteArray conversions, make this more
  explicit

Leads to much cleaner code
Also makes at least one implicit enum->bool conversion more explicit.
Used to allow setting --testmode in CLI options.
@krf krf requested a review from HenryMiller1 April 28, 2026 07:04
FinishedJobs::iterator it = m_finishedJobs.begin();
for (const FinishedJobs::iterator itEnd = m_finishedJobs.end(); it != itEnd; ++it) {
auto it = m_finishedJobs.begin();
for (const auto itEnd = m_finishedJobs.end(); it != itEnd; ++it) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

general practice it is to move the end() call out of the loop, and leave begin() in. Begin is called once either way, while end() is called on each iteration - which is only needed if end() is expected to change inside the loop (and this implies other iteration invalidation issues).

I'm including to suggest range based for here as well.

Comment thread src/views/starview.cc
if (hostInfo) {
QToolTip::showText(gp + QPoint(10, 10), hostInfo->toolTip(), this, itemRect);
} else {
} else if (const auto monitor = m_starView->monitor()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like assigning variables inside an if. It works and is probably right in this case, but people expect to see == not =.

Comment thread src/fakemonitor.cc

const int MAX_JOB_COUNT = 10;
const int MAX_HOST_COUNT = 40;
const int MAX_HOST_COUNT = 20;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

consider constexpr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants