Allow disabling the public-server-list

[TimMThomas]

This implements #4070 and makes it possible to hide the public-server-list from the connect dialog.
The save location was left under ui for backwards compatibility.

This is a follow-up on #4305

[Popkornium18]

Not entirely sure but I think your changes are conflicting with mine which introduced some usages of the bool you renamed. I think you need to rebase from master and rename these too.

[TimMThomas]

Yea, saw that, im on it

[TimMThomas]

I think this should be it. However when i check the option and reopen the connection menu, mumble crashes. The Warning that the public list is disabled is printed. Can anyone replicate this?

[Popkornium18]

My guess is that since qlPublicServers is static it still has items once you reopen the connect dialog.

mumble/src/mumble/ConnectDialog.cpp

Lines 1418 to 1423 in b1e8910

	foreach(QTreeWidgetItem *qtwi, qlNew) {
	ServerItem *si = static_cast<ServerItem *>(qtwi);
	qtwServers->siPublic->addServerItem(si);
	filterServer(si);
	startDns(si);
	}

Then this gets executed because there are public servers in the list. But since siPublic has been set to NULL this leads to a crash.

Disabling the public list should probably also clear qlPublicServers.

Edit: Eh, probably not exactly that since that function shouldn't be called but it smells like that null pointer is dereferenced.

[TimMThomas]

Jep, its a segmentation fault.

mumble/src/mumble/ConnectDialog.cpp

Lines 972 to 986 in b1e8910

	if (bAllowFilters) {
	switch (g.s.ssFilter) {
	case Settings::ShowPopulated:
	qcbFilter->setCurrentText(tr("Show Populated"));
	break;
	case Settings::ShowAll:
	qcbFilter->setCurrentText(tr("Show All"));
	break;
	default:
	qcbFilter->setCurrentText(tr("Show Reachable"));
	break;
	}
	} else {
	qcbFilter->setEnabled(false);
	}

if i add an "&& !g.s.bDisablePublicList" to the if it works fine. Would that also be solved by clearing the qlPublicServers? (Besides it being a good idea anyways)

[Popkornium18]

The text change is probably calling this slot.

mumble/src/mumble/ConnectDialog.cpp

Lines 1822 to 1833 in b1e8910

	void ConnectDialog::on_qcbFilter_currentIndexChanged(int filterIndex) {
	const QString filter = qcbFilter->itemText(filterIndex);
	if (filter == tr("Show All")) {
	g.s.ssFilter = Settings::ShowAll;
	} else if (filter == tr("Show Reachable")) {
	g.s.ssFilter = Settings::ShowReachable;
	} else if (filter == tr("Show Populated")) {
	g.s.ssFilter = Settings::ShowPopulated;
	}
	filterPublicServerList();
	}

Which is then trying to filter the public server list and dereferencing the nullpointer here.

mumble/src/mumble/ConnectDialog.cpp

Lines 1523 to 1525 in b1e8910

	foreach(ServerItem * const si, qtwServers->siPublic->qlChildren) {
	filterServer(si);
	}

Also it looks like I used spaces for tabs there 🤦

[TimMThomas]

So, i dont really know what the best approach would be. I could add a toggled function to the checkbox, or i think it could work if i clear it in the NetworkConfig::save(). I dont really feel comfortable with both, any suggestions?

[Popkornium18]

I would check if the public list is disabled before calling filterPublicServerList() which solves the crashing issue. You could clear qlPublicServers when the box is checked. It's not strictly necessary since nothing is done with it if the public server list is disabled, but why keep it if it's not needed.

[TimMThomas]

alright, added the check, since im new to qt im still struggling with adding the action to the checkbox, I will try to do this on the weekend :)

[TimMThomas]

After messing a bit around i came to the conclusion that qlPublicServers should be anyway destroyed after the ConnectDialog closes/gets destroyed. So i think it shouldnt be necessary to do that. Then the pr should be ready like it is?

[Krzmbrzl]

Oh and for your commit message, I think this slightly altered version would be better (plus I fixed a few typos):

FEAT(settings): Allow disabling of the public server list

This commit makes it possible to hide the public server list from the connect dialog.
The setting's group was left to be "ui" for backwards compatibility (even though the
actual setting is part of the network settings in the UI).

Implements #4070

[Popkornium18]

After messing a bit around i came to the conclusion that qlPublicServers should be anyway destroyed after the ConnectDialog closes/gets destroyed. So i think it shouldnt be necessary to do that. Then the pr should be ready like it is?

It shouldn't be cleared when the dialog gets closed as that would require the server list to be downloaded every time the connect dialog is opened, which is unneccessary. But as I said, it isn't a big deal. It's no privacy risk, it's just some memory that could be freed. But once the client is restarted there will be no difference as the list will not get populated in the first place.

[Krzmbrzl]

I guess the saved memory is not noticeable on any system though, so I think that's okay :)

[TimMThomas]

Added the changes you requested. And yep, i feel like im blind, didnt saw that the list is static 😅
On another note: thank you guys for your help :)

Edit: The CI for macOS failed when setting up the build environment, i dont think the changes i made should break anything hmm

[Krzmbrzl]

We had some issues with the macOS CI (see #4349) - I rebased your branch against the current master brach and now all CIs should succeed. After that I think we can merge this PR :)

[Krzmbrzl]

(I also fixed some whitespace issue while I was on it - you had mixed indentation with tabs and with spaces)

[Krzmbrzl]

Thank you for your contribution! :)

[TimMThomas]

Alright, thank you all for you help! :)

[Krzmbrzl]

You're welcome :)