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

Remove "default_host" profile #2950

Merged
merged 14 commits into from Aug 15, 2019

Conversation

@vadi2
Copy link
Member

commented Aug 7, 2019

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.

@vadi2 vadi2 added this to the 4.1.0 milestone Aug 7, 2019

@vadi2 vadi2 requested review from SlySven and itsTheFae Aug 7, 2019

@vadi2 vadi2 requested review from Mudlet/core-cpp as code owners Aug 7, 2019

@add-deployment-links

This comment has been minimized.

Copy link

commented Aug 7, 2019

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

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

// 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) {

This comment has been minimized.

Copy link
@Kebap

Kebap Aug 8, 2019

Contributor

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 8, 2019

Author Member

Thanks, fixed.

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

This comment has been minimized.

Copy link
@Kebap

Kebap Aug 8, 2019

Contributor

Never seen this syntax before

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 8, 2019

Author Member

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.

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

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};

:neckbeard:

@vadi2

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

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

@@ -3671,6 +3646,36 @@ void mudlet::startAutoLogin()
}
}

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 8, 2019

Author Member

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.

This comment has been minimized.

Copy link
@Kebap

Kebap Aug 8, 2019

Contributor

Make only the latter callable?

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 9, 2019

Author Member

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

@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

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

||(mMenuBarVisibility & visibleMaskNormally)) {

// Are there any profiles loaded?
if ((mHostManager.getHostCount() < 1 && mMenuBarVisibility & visibleAlways)

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

The < 1 is redundant... 😉

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 9, 2019

Author Member

True, fixed.

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

As above, so below...

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 9, 2019

Author Member

Fixed.


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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

⚠️ 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...?

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 8, 2019

Author Member

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 9, 2019

Author Member

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.

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 9, 2019

Member

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!}

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 10, 2019

Author Member

It does need a valid Host, yeah.

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 10, 2019

Author Member

Fixed.

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 9, 2019

Author Member

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 9, 2019

Member

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.

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 10, 2019

Author Member

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 12, 2019

Member

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

Screenshot_20190812_164548

Screenshot_20190812_164718

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 12, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 9, 2019

Author Member

Fixed.

@@ -3717,8 +3722,6 @@ void mudlet::doAutoLogin(const QString& profile_name)
// save the setting is on profile loading:
pHost->mTelnet.setEncoding(readProfileData(profile_name, QLatin1String("encoding")), false);

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 9, 2019

Author Member

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 9, 2019

Member

🤔 Humm, yeah - where has the emit gone?

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 10, 2019

Author Member

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

@@ -3671,6 +3646,36 @@ void mudlet::startAutoLogin()
}
}

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 8, 2019

Member

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

vadi2 added 6 commits Aug 9, 2019
@vadi2

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2019

Ready for review.

@@ -1729,6 +1742,7 @@ void mudlet::disableToolbarButtons()
mpMainToolBar->actions()[14]->setEnabled(false);

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

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 10, 2019

Member

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

This comment has been minimized.

Copy link
@vadi2

vadi2 Aug 10, 2019

Author Member

Sure, applied.

@demonnic
Copy link
Member

left a comment

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

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

3 of 4 checks passed

automerge automerge
Details
CodeFactor 7 issues fixed. 1 issue found.
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@vadi2 vadi2 deleted the vadi2:remove-default-host branch Aug 15, 2019

wsdmatty added a commit to wsdmatty/Mudlet that referenced this pull request Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.