Refactor: use a single enum to classify all TConsole instances #2138

SlySven · 2018-12-17T00:36:56Z

By replacing the following somewhat adhoc collection of flags it is easier to refactor code that is dependent on the particular type of TConsole:

(bool) TConsole::mIsDebugConsole
(bool) TConsole::mIsSubConsole
(bool) TConsole::mUserConsole - flag for a subConsole/Buffer/UserWindow

Also removed the unused:
* (bool) TConsole::mWindowIsHidden

This has revealed that there are numerous cases where the Buffer type of TConsole is including code that was likely only intended for the main one for each profile...!

Signed-off-by: Stephen Lyons slysven@virginmedia.com

By replacing the following somewhat adhoc collection of flags it is easier to refactor code that is dependent on the particular type of `TConsole`: * (bool) TConsole::mIsDebugConsole * (bool) TConsole::mIsSubConsole * (bool) TConsole::mUserConsole - flag for a subConsole/Buffer/UserWindow Also removed the unused: * (bool) TConsole::mWindowIsHidden This has revealed that there are numerous cases where the `Buffer` type of `TConsole` is including code that was likely only intended for the main one for each profile...! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2018-12-17T00:38:38Z

Suggestions for changes are welcome - as I say: some of the conditional code parts now look wrong...

SlySven · 2018-12-17T00:41:37Z

src/TConsole.cpp

@@ -122,21 +120,27 @@ TConsole::TConsole(Host* pH, bool isDebugConsole, QWidget* parent)
        mStandardFormat.flags &= ~(TCHAR_STRIKEOUT);
    } else {
        setWindowTitle(tr("Non Debug Console"));
-        if (parent) {
-            mIsSubConsole = true;
+        if (mType & (ErrorConsole|SubConsole|UserWindow)) {


Not sure that the ErrorConsole type should be here as I do not think it should be added to the "main" console's mSubConsoleList though give that that is not used anywhere it is probably due for removal anyhow.

I don't think it matters then - we can leave it be

SlySven · 2018-12-17T00:44:40Z

src/TConsole.cpp

            mpHost->mpConsole->mSubConsoleList.append(this);
            mMainFrameTopHeight = 0;
            mMainFrameBottomHeight = 0;
            mMainFrameLeftWidth = 0;
            mMainFrameRightWidth = 0;
-        } else {
-            mIsSubConsole = false;
+        } else if (mType & (MainConsole|Buffer)) {


The Buffer type should not be here I think. in fact, in the light of the previous comment I guess the ErrorConsole and Buffer types could/should be move to a new else if with just:

mMainFrameTopHeight = 0; mMainFrameBottomHeight = 0; mMainFrameLeftWidth = 0; mMainFrameRightWidth = 0;

SlySven · 2018-12-17T00:46:26Z

src/TConsole.cpp

@@ -567,7 +571,7 @@ TConsole::TConsole(Host* pH, bool isDebugConsole, QWidget* parent)
    layerCommandLine->setPalette(__pal);

    changeColors();
-    if (!mIsSubConsole && !mIsDebugConsole) {
+    if (mType & (MainConsole|Buffer)) {


Should, I think, be just MainConsole...

SlySven · 2018-12-17T00:48:26Z

src/TConsole.cpp

@@ -603,7 +607,7 @@ void TConsole::setLabelStyleSheet(std::string& buf, std::string& sh)

 void TConsole::resizeEvent(QResizeEvent* event)
 {
-    if (!mIsDebugConsole && !mIsSubConsole) {
+    if (mType & (MainConsole|Buffer)) {


Just MainConsole here also?

SlySven · 2018-12-17T00:49:53Z

src/TConsole.cpp

@@ -613,7 +617,7 @@ void TConsole::resizeEvent(QResizeEvent* event)
    int y = event->size().height();


-    if (!mIsSubConsole && !mIsDebugConsole) {
+    if (mType & (MainConsole|Buffer)) {


I think this and the previous can be run together with the two event->size() lines being done beforehand.

SlySven · 2018-12-17T00:51:39Z

src/TConsole.cpp

@@ -626,7 +630,7 @@ void TConsole::resizeEvent(QResizeEvent* event)
    }
    mpMainDisplay->move(mMainFrameLeftWidth, mMainFrameTopHeight);

-    if (mIsSubConsole || mIsDebugConsole) {
+    if (mType & (CentralDebugConsole|ErrorConsole|SubConsole|UserWindow)) {


I think the logic needs to be inverted so that the former else code is then only used for the MainConsole case...

SlySven · 2018-12-17T00:53:07Z

src/TConsole.cpp

@@ -635,7 +639,7 @@ void TConsole::resizeEvent(QResizeEvent* event)

    QWidget::resizeEvent(event);

-    if (!mIsDebugConsole && !mIsSubConsole) {
+    if (mType & (MainConsole|Buffer)) {


We do not need a WindowResizeEvent for the Buffer type but since it is not visible we probably have not been seeing the code being run for them!

SlySven · 2018-12-17T00:54:38Z

src/TConsole.cpp

@@ -656,7 +660,7 @@ void TConsole::resizeEvent(QResizeEvent* event)

 void TConsole::refresh()
 {
-    if (!mIsDebugConsole) {
+    if (mType & (ErrorConsole|MainConsole|SubConsole|UserWindow|Buffer)) {


This looks odd - being run on everything except the CentralDebugConsole ???

SlySven · 2018-12-17T00:55:46Z

src/TConsole.cpp

@@ -820,7 +824,7 @@ int TConsole::getButtonState()

 void TConsole::toggleLogging(bool isMessageEnabled)
 {
-    if (mIsDebugConsole || mIsSubConsole) {
+    if (mType & (CentralDebugConsole|ErrorConsole|SubConsole|UserWindow)) {


Technically, it should include the Buffer type as well.

SlySven · 2018-12-17T00:56:18Z

src/TConsole.cpp

@@ -1030,7 +1034,7 @@ void TConsole::toggleLogging(bool isMessageEnabled)
 // generated for Lua user control by the Lua subsystem.
 void TConsole::slot_toggleLogging()
 {
-    if (mIsDebugConsole || mIsSubConsole) {
+    if (mType & (CentralDebugConsole|ErrorConsole|SubConsole|UserWindow)) {


As previous - should include Buffer as well.

SlySven · 2018-12-17T00:57:25Z

src/TConsole.cpp

@@ -1040,7 +1044,7 @@ void TConsole::slot_toggleLogging()

 void TConsole::slot_toggleReplayRecording()
 {
-    if (mIsDebugConsole) {
+    if (mType & CentralDebugConsole) {


Surely this should include everything apart from the MainConsole so that only that type actually does anything here.

SlySven · 2018-12-17T00:59:25Z

src/TConsole.cpp

@@ -1329,7 +1335,7 @@ void TConsole::deselect()

 void TConsole::showEvent(QShowEvent* event)
 {
-    if (!mIsDebugConsole && !mIsSubConsole) {
+    if (mType & (MainConsole|Buffer)) {


Given that the Buffer type is never visible I guess it shouldn't be here.

SlySven · 2018-12-17T00:59:46Z

src/TConsole.cpp

@@ -1339,7 +1345,7 @@ void TConsole::showEvent(QShowEvent* event)

 void TConsole::hideEvent(QHideEvent* event)
 {
-    if (!mIsDebugConsole && !mIsSubConsole) {
+    if (mType & (MainConsole|Buffer)) {


Same as showEvent.

SlySven · 2018-12-17T01:01:19Z

src/TTextEdit.cpp

@@ -1061,7 +1061,7 @@ void TTextEdit::slot_popupMenu()

 void TTextEdit::mousePressEvent(QMouseEvent* event)
 {
-    if (!mpConsole->mIsSubConsole && !mpConsole->mIsDebugConsole) {
+    if (mpConsole->getType() & (TConsole::MainConsole|TConsole::Buffer)) {


Impossible for the Buffer type!

SlySven · 2018-12-17T01:01:40Z

src/TTextEdit.cpp

@@ -1610,7 +1610,7 @@ void TTextEdit::mouseReleaseEvent(QMouseEvent* event)
        mCtrlSelecting = false;
    }

-    if (!mpConsole->mIsSubConsole && !mpConsole->mIsDebugConsole) {
+    if (mpConsole->getType() & (TConsole::MainConsole|TConsole::Buffer)) {


Again, impossible for the Buffer type!

SlySven · 2018-12-29T02:22:59Z

@vadi2 This is marked as a W.I.P. because I want feed-back on the types that each if (...) is working on in the commented places. The current selection reproduces the current code situation as far as I can tell - I just want input from other sets of eyeballs before making any functional changes.

Kebap · 2018-12-29T12:57:42Z

My 0.02$ is: don't let the PR grow into an untameable mess again. Merge the refractoring which reproduces the current behavior as is. Move the discussion about functional changes into their own issues, which can then spawn seperated PRs.

vadi2

I agree - lets merge this in now so we have the improvement, and then look into cleanup as a separate PR. I've read through your comments @SlySven and they seem to make sense. Let's also merge early so people can help test!

SlySven · 2019-01-01T18:31:50Z

Okay, I'll raise an issue to link back to the points I've raised here after merging it in.

SlySven added the 0 - Work in progress label Dec 17, 2018

SlySven assigned vadi2 and SlySven Dec 17, 2018

SlySven requested a review from a team December 17, 2018 00:36

SlySven requested a review from a team as a code owner December 17, 2018 00:36

SlySven commented Dec 17, 2018

View reviewed changes

SlySven mentioned this pull request Dec 27, 2018

Enable adding custom input lines #1897

Closed

vadi2 approved these changes Dec 30, 2018

View reviewed changes

SlySven added 2 - Working and removed 0 - Work in progress labels Jan 1, 2019

SlySven merged commit 11f46b3 into Mudlet:development Jan 1, 2019

SlySven mentioned this pull request Jan 1, 2019

Review code assignment to different TConsole::mType values #2169

Open

15 tasks

SlySven deleted the Refactor_useASingleEnumToClassifyAllTConsoleInstances branch January 1, 2019 19:11

SlySven restored the Refactor_useASingleEnumToClassifyAllTConsoleInstances branch June 22, 2020 17:58

SlySven deleted the Refactor_useASingleEnumToClassifyAllTConsoleInstances branch June 22, 2020 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: use a single enum to classify all TConsole instances #2138

Refactor: use a single enum to classify all TConsole instances #2138

SlySven commented Dec 17, 2018

SlySven commented Dec 17, 2018

SlySven Dec 17, 2018 •

edited

vadi2 Jan 3, 2019

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018 •

edited

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven Dec 17, 2018

SlySven commented Dec 29, 2018

Kebap commented Dec 29, 2018

vadi2 left a comment

SlySven commented Jan 1, 2019

Refactor: use a single enum to classify all TConsole instances #2138

Refactor: use a single enum to classify all TConsole instances #2138

Conversation

SlySven commented Dec 17, 2018

SlySven commented Dec 17, 2018

SlySven Dec 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlySven Dec 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlySven commented Dec 29, 2018

Kebap commented Dec 29, 2018

vadi2 left a comment

Choose a reason for hiding this comment

SlySven commented Jan 1, 2019

SlySven Dec 17, 2018 •

edited

SlySven Dec 17, 2018 •

edited