Remove "default_host" profile by vadi2 · Pull Request #2950 · Mudlet/Mudlet

vadi2 · 2019-08-07T19:21:25Z

Brief overview of PR changes/additions

Remove "default_host" profile.

Motivation for adding to Mudlet

200ms faster startup because of no extra host getting made + 10ms saved from creating the debug area only when you need it, less messy code workarounds to ignore the "default_host" and no messages like this in stdout which don't mean anything:

cTelnet::~cTelnet() Instance being destroyed before it could display some messages,
messages are:
------------
[  OK  ]  - Lua module rex_pcre loaded.
------------
[  OK  ]  - Lua module zip loaded.
------------
[  OK  ]  - Lua module lfs loaded.
------------
[  OK  ]  - Lua module sqlite3 loaded.
------------
[  OK  ]  - Lua module utf8 loaded.
------------
[  OK  ]  - Lua module yajl loaded.
------------
[ INFO ]  - Map audit starting...
------------
[  OK  ]  - Auditing of map completed (0.00s). Enjoy your game...
------------
[  OK  ]  - Map loaded successfully (0s).
------------

Other info (issues closed, discussion etc)

Opening IRC is now not possible until at least one profile open - but I don't think anyone seriously uses Mudlet as an IRC-only client. We previously kept this functionality was the only way to get help, but with Discord, this isn't needed.

Closes #1352, closes #1351, closes #736.

…-host

add-deployment-links · 2019-08-07T19:21:26Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

linux: (download pending, check back soon!)
osx: (download pending, check back soon!)
windows: https://make.mudlet.org/snapshots/716c00/Mudlet-4.0.3-testing-PR2950-a783d32b-windows.zip

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

Kebap · 2019-08-08T07:33:15Z

src/dlgProfilePreferences.cpp

-    // count will be 2 in the case we particularly want to handle (adding the
-    // first real Host instance):
-    if (!mpHost && pHost && count < 3) {
+    if (!mpHost && pHost && count < 1) {


There were <3 before, you remove default_host, now it should be <2, right?

Thanks, fixed.

Kebap · 2019-08-08T07:33:28Z

src/Host.cpp

-    QMutexLocker locker(& mLock);
-    bool isChanged = false;
+    QMutexLocker locker(&mLock);
+    bool dictionaryChanged {};


Never seen this syntax before

We've used it in a few places in Mudlet now. It creates dictionaryChanged and sets it to a default value you'd expect. If you just do bool dictionaryChanged; the value could be random because one wasn't given (it basically doesn't do anything at all to set one).

Technical term is "braced intializer", unfortunately all resources I could find from a quick search weren't an easy read.

It uses the default constructor IIRC - in the same manner as it would be for a member in a class's constructor's initialization list, e.g. if dictionaryChanged was a member then this would do the same as:

, dictionaryChanged()

would do in the initialization list.

Arguments can also be provided for the constructor of more complex items - and even bools can be initialized to the non-default value true with:

bool dictionaryChanged {true};

vadi2 · 2019-08-08T11:32:47Z

@wiploo are you able to test if this breaks anything? It is a pretty fundamental change.

vadi2 · 2019-08-08T11:36:36Z

src/mudlet.cpp


+bool mudlet::addHost(const QString& hostname, const QString& port, const QString& login, const QString& pass)
+{
+    auto success = mHostManager.addHost(hostname, port, login, pass);


We now have HostManager::addHost() and mudlet::addHost() and the intention is that it's only the latter that should be called, because it wraps some necessary initialisation (the central debug area) around the method once we have our first host.

I'm not sure how to properly describe it using C++ semantics.

Make only the latter callable?

The mudlet class is overriding the addHost method of the HostManager class maybe? 🤓

In retrospect, this design was bad. Fixed it to a more logical one - you call HostManager.addHost as you did before now.

SlySven · 2019-08-08T22:18:29Z

We previously kept this functionality was the only way to get help, but with Discord, this isn't needed.

Not everyone has/uses/can use (Chinese users?) Discord.

vadi2 · 2019-08-08T22:21:07Z

That is true, and they have their own QQ group, so IRC is not relevant to them.

SlySven · 2019-08-08T22:37:47Z

src/mudlet.cpp

-      ||(mMenuBarVisibility & visibleMaskNormally)) {
-
+    // Are there any profiles loaded?
+    if ((mHostManager.getHostCount() < 1 && mMenuBarVisibility & visibleAlways)


The < 1 is redundant... 😉

True, fixed.

SlySven · 2019-08-08T22:40:12Z

src/mudlet.cpp

-    // as the first one
-    if ((mHostManager.getHostCount() < 2 && mToolbarVisibility & visibleAlways) || (mToolbarVisibility & visibleMaskNormally)) {
+    // Are there any profiles loaded?
+    if ((mHostManager.getHostCount() < 1 && mToolbarVisibility & visibleAlways)


As above, so below...

SlySven · 2019-08-08T22:44:31Z

src/mudlet.cpp

+
+    if (success && !mpDebugArea) {
+        mpDebugArea = new QMainWindow(nullptr);
+        mpDefaultHost = mHostManager.getHost(hostname);


⚠️ Um, Huston, we may have a problem - it looks like this and the next line will make the Central Debug Console belong to the first profile open - so what happens to the CDC when multi-playing and the first profile is closed - does that make the CDC die as well...?

Eh, yes, you raise a valid point. I'll have it migrate to the next available profile (if any) then.

Hm, tough. We could just also close the debug area and re-open it with the next available host, but then the user experience is not so nice: everything that was in the debug area will get lost. On the other hand, implementing host migration for an existing TConsole is a fair bit of work just for this one usecase.

Does it have to be a Host pointer - or is it only for the QObject::parent property so that if the parent is destroyed it causes all the QObject derived children of that parent to commit suicide? If that is the case then perhaps the CDC can be given the mudlet singleton as a parent - then things ought to be fine.

I haven't got the code open in front of me right now (😮) but I will take a look over the weekend if I can make sense of this...

On the other hand, implementing host migration for an existing TConsole is a fair bit of work just for this one usecase.

That one TConsole instance is an important use case - and we (I certainly) have suggested to users to have the debug console opened from a dummy profile before the one with a start-up problem is initiated - so that early debug output can be observed in the latter - so it is important that the CDC is shared equally between all open profiles. {Fair enough there is also a long-standing need for a means to show which profile produces which lines in the CDC!}

It does need a valid Host, yeah.

SlySven · 2019-08-08T22:45:49Z

src/mudlet.cpp

@@ -3685,7 +3690,7 @@ void mudlet::doAutoLogin(const QString& profile_name)

    // load an old profile if there is any
    // PLACEMARKER: Host creation (2) - autoload case


Please revise the number down now that // PLACEMARKER: Host creation (1) ... has gone away.

I still don't see the point of these placemakers. Revised, though.

They were/are marking all the places where a profile/Host can be created (and there are some for destruction) - which is useful to check that all of them are passed through/tested in some situations (as we have had cases where one execution path showed a bug that another one didn't) and putting breakpoints on them is one technique I have used to debug things in those cases.

Modern IDEs have a "find all usages of this function" feature which does the same, but better: it never forgets to update comments 😁

I just like the Qt TODO plugin as it makes it very quick and simple to jump to previously commented/marked places in the whole project...

Yes but, doing these things requires manual work to keep it up to date - which can be wrong! So it has a low reliability score - thus I'd ignore it and only trust the source...

SlySven · 2019-08-08T22:46:39Z

src/dlgConnectionProfiles.cpp

@@ -2028,7 +2021,7 @@ void dlgConnectionProfiles::loadProfile(bool alsoConnect)
    }
    // load an old profile if there is any
    // PLACEMARKER: Host creation (3) - normal case


Please revise the number down now that // PLACEMARKER: Host creation (1) ... has gone away.

SlySven · 2019-08-08T22:49:56Z

src/mudlet.cpp


-    // For the first real host created the getHostCount() will return 2 because
-    // there is already a "default_host"
    signal_hostCreated(pHost, mHostManager.getHostCount());


If you haven't already done so, may I suggest checking any slots connected to this signal to ensure that they respond correctly given that the number passed will be one less now...?

Yes, the only place it is used in is the settings window, and that works OK.

Also, this is a strange way to emit a signal...

🤔 Humm, yeah - where has the emit gone?

It was like that from the beginning 92f61bb#diff-84e2883dc57a4e7b034fbac3fd4668b5R2633 - I'll fix it.

SlySven · 2019-08-08T22:55:52Z

src/mudlet.cpp


+bool mudlet::addHost(const QString& hostname, const QString& port, const QString& login, const QString& pass)
+{
+    auto success = mHostManager.addHost(hostname, port, login, pass);


The mudlet class is overriding the addHost method of the HostManager class maybe? 🤓

…-host

vadi2 · 2019-08-10T09:51:21Z

Ready for review.

SlySven · 2019-08-10T13:31:33Z

src/mudlet.cpp

    mpMainToolBar->actions()[14]->setEnabled(false);

    mpActionReplay->setEnabled(false);
+    mpActionIRC->setEnabled(false);


Is this the best place for this line of code - IMHO it would be better placed before or after the pair of lines it is sat between which both act on mpActionReplay rather than splitting them...?

Sure, applied.

…-host

demonnic

launches, deleted default_host, still launches, test profile isn't erroring. Does load faster.

It seems, *I think* that the removal of the "default host" profile (in Mudlet#2950) broke the logic that detected that any "real" profiles were loaded and prevented both the main menu bar and the main toolbar from being hidden when no such profiles were active. As it is now it is not possible to hide both the main menu and the main toolbar AFTER a profile is loaded - even though we provide the settings that are suppose to allow that. The original `enum` used to encode the wanted settings for the visibility of those two items used a cunning combination of bit patterns (and some application of DeMorgan's laws) to work out what the visibility settings are and what state the application is in (number of profiles active) so as to show or hide the main menu and main toolbars. Unfortunately I've lost track of how I put it together when the single "default host" profile was included in the count so had to rewrite the logic tests without changing the `enum` values. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

It seems, *I think* that the removal of the "default host" profile (in #2950) broke the logic that detected that any "real" profiles were loaded and prevented both the main menu bar and the main toolbar from being hidden when no such profiles were active. As it is now it is not possible to hide both the main menu and the main toolbar AFTER a profile is loaded - even though we provide the settings that are suppose to allow that. The original `enum` used to encode the wanted settings for the visibility of those two items used a cunning combination of bit patterns (and some application of DeMorgan's laws) to work out what the visibility settings are and what state the application is in (number of profiles active) so as to show or hide the main menu and main toolbars. Unfortunately I've lost track of how I put it together when the single "default host" profile was included in the count so had to rewrite the logic tests without changing the `enum` values. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 added 5 commits August 6, 2019 12:20

Wrap addHost into mudlet::addHost()

96a23b4

Clean up comments

df67e26

Merge remote-tracking branch 'origin/development' into remove-default…

77e8713

…-host

Pruning of default_profile usage - wip

b5193b0

Update "first Mudlet launch ever" random profile selection

b41ee73

vadi2 added this to the 4.1.0 milestone Aug 7, 2019

vadi2 requested review from SlySven and itsTheFae August 7, 2019 19:21

vadi2 requested a review from a team as a code owner August 7, 2019 19:21

vadi2 requested a review from a team August 7, 2019 19:21

Kebap reviewed Aug 8, 2019

View reviewed changes

Typo fix

fbfb807

vadi2 commented Aug 8, 2019

View reviewed changes

SlySven reviewed Aug 8, 2019

View reviewed changes

vadi2 added 6 commits August 9, 2019 08:14

Review feedback

11b95b4

Merge remote-tracking branch 'origin/development' into remove-default…

bc5ea65

…-host

Review feedback

083927b

Emit signal properly

b6445a1

Fix crash when re-opening central debug console from another profile

bb823ac

Improve function definition

908ced4

SlySven reviewed Aug 10, 2019

View reviewed changes

Fix crash

bd05cef

Merge remote-tracking branch 'origin/development' into remove-default…

b5a9afa

…-host

demonnic approved these changes Aug 15, 2019

View reviewed changes

vadi2 merged commit 6b29b60 into Mudlet:development Aug 15, 2019

vadi2 deleted the remove-default-host branch August 15, 2019 16:43

atari2600tim mentioned this pull request Jun 23, 2020

Registering the sysIrcMessage event sometimes doesn't call the associated function #3704

Closed

SlySven mentioned this pull request Feb 14, 2021

Provide a means to open the Central Debug Console before a profile is opened #4807

Open

SlySven mentioned this pull request Sep 23, 2021

BugFix: repair "prevent all controls from being hidden" feature #5445

Merged

		@@ -3685,7 +3690,7 @@ void mudlet::doAutoLogin(const QString& profile_name)

		// load an old profile if there is any
		// PLACEMARKER: Host creation (2) - autoload case

Uh oh!

Conversation

vadi2 commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Uh oh!

add-deployment-links bot commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven commented Aug 8, 2019

Uh oh!

vadi2 commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Aug 7, 2019 •

edited

Loading

add-deployment-links bot commented Aug 7, 2019 •

edited

Loading

vadi2 Aug 10, 2019 •

edited

Loading