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

Modernize code (Qt5 connects + init of params) #474

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

Conversation

francescmm
Copy link
Contributor

In this PR I've applied two main changes:

  1. Use of Qt5 connects that will cause compilation error if the signal/slot doesn't exist or has a mismatch. This prevents connection problems in runtime, quite hard to debug/spot.
  2. Move the initialization of member parameters to the header file. The reason for this is that in the case of having multiple constructors, one might forget to initialize some of the parameters, or if not forgotten, it will cause a lot of code duplication.

These should be harmless modifications since they don't change any implementation.

I've postponed some real changes like using smart pointers, modern iterators, etc. until this PR goes in and clang-format is in place. I don't think it's recommendable to modify everything in one single PR.

Last thing: I've kept one old-style connect because it would make the code use qoverload. I know that this was an issue in some distros as ArchLinux and since I'm not sure QTermWidget is ship there or not, I decided to keep it.

Copy link
Member

@yan12125 yan12125 left a comment

Choose a reason for hiding this comment

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

I have only finished Session.cpp and Session.h, but there are already several points. Let's fix a little at a time instead of fixing everything in the end.

ptySlaveFd = _shellProcess->pty()->slaveFd();

//create emulation backend
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this comment is gone. It's better to keep comments after refactoring.


QObject::connect( widget ,SIGNAL(destroyed(QObject *)) , this ,
SLOT(viewDestroyed(QObject *)) );
//slot for close
Copy link
Member

Choose a reason for hiding this comment

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

ditto


connect(_emulation, &Emulation::imageResizeRequest, this, &Session::onEmulationSizeChange);
connect(_emulation, &Emulation::imageSizeChanged, this, &Session::onViewSizeChange);
connect(_emulation, &Emulation::cursorChanged,
Copy link
Member

Choose a reason for hiding this comment

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

Good catch! cursorChanged is indeed a signal of Emulation, not Vt102Emulation.

// or KPty::login() which uses a QProcess to start /usr/bin/utempter
, _flowControl(true)
, _fullScripting(false)
, _sessionId(0)
Copy link
Member

Choose a reason for hiding this comment

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

Initialization for _flowControl, _flowControl, _sessionId are gone?

, _sessionId(0)
// , _zmodemBusy(false)
// , _zmodemProc(0)
// , _zmodemProgress(0)
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to also move initialization of zmodem-related variables to Session.h, so that we don't forget initialization in case the zmodem feature is added back.

// , _zmodemBusy(false)
// , _zmodemProc(0)
// , _zmodemProgress(0)
, _hasDarkBackground(false)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, initialization gone

@@ -491,7 +491,11 @@ public slots:
private slots:
void done(int);

// void fireZModemDetected();
Copy link
Member

Choose a reason for hiding this comment

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

Although technical functionaltiy should not be affected, it's better to avoid changing slots into ordinary functions.

// void zmodemReadStatus();
// void zmodemReadAndSendBlock();
// void zmodemRcvBlock(const char *data, int len);
// void zmodemFinished();
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, keep these in slots.

@yan12125
Copy link
Member

yan12125 commented Jun 4, 2023

Last thing: I've kept one old-style connect because it would make the code use qoverload. I know that this was an issue in some distros as ArchLinux and since I'm not sure QTermWidget is ship there or not, I decided to keep it.

We can move to that. qOverload needs Qt 5.7 and C++ 14, and LXQt now requires Qt 5.15 and C++ 17: lxqt/lxqt#1974, lxqt/lxqt-build-tools#83

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